Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for nested tenant config override #920

Merged

Conversation

rootgog
Copy link
Contributor

@rootgog rootgog commented Aug 19, 2022

This PR adds the ability to assign from nested JSON values on the Tenant model while staying backwards compatible with the existing implementation.

Essentially eloquent's model getAttribute is replaced with the Laravel helper function Arr::get when the values are extracted from the tenent model which allows for dot notated paths to be used to map nested JSON values.

For example this allows implementations such as the following

\Stancl\Tenancy\Features\TenantConfig::$storageToConfigMap = [
    'whitelabel.config.theme' => 'whitelabel.theme',
];

@stancl
Copy link
Member

stancl commented Aug 21, 2022

Hey, thanks for the PR! I was trying to find a past discussion about this because I think there was a convo about how this part of the code works, so I wanted to check if there'd be any problem with using Arr::get(). But it seems like this will work fine.

I found this issue #412 and that may have been it (I think there was some other discussion too but this looks similar enough). Essentially, "supporting arrays" is a thing on both sides of the foo => bar mapping here. I believe the commit which fixed #412 6dad6ce added support for mapping a single tenant value to multiple config values.

And this PR will support reading from arrays on the tenant side. Is that correct?

The implementation looks good and so does the test. Though I think I'll change the target to 4.x. We're focusing on v4 development now so we prefer that new features are added there, and 3.x changes are only things like urgent bug fixes.

If I change the target branch, there will likely be a conflict in the test file, since we migrated to pest on the master branch. You can try resolving that on your own, or @abrardev99 will fix it on Monday and we'll get this merged 👍🏻

@stancl stancl changed the base branch from 3.x to master August 21, 2022 14:09
@stancl
Copy link
Member

stancl commented Aug 21, 2022

Ah hmm, seems like your branch is based on 3.x which has some additional changes that can't be easily merged. I guess we'll merge this into 3.x then and merge 3.x into master

@stancl stancl changed the base branch from master to 3.x August 21, 2022 14:42
@stancl stancl merged commit 1ea3cef into archtechx:3.x Aug 21, 2022
@stancl
Copy link
Member

stancl commented Aug 21, 2022

Thanks a lot for the PR!

stancl added a commit that referenced this pull request Aug 22, 2022
* exclude master from CI

* Add space after 'up' in 'docker-compose up-d' (#900)

* Fix ArgumentCountError on the TenantAssetsController (#894)

* Fix ArgumentCount exception on the TenantAssetsController when no `$path` is provided

* CS

* CS

* Handle null case explicitly

* code style

Co-authored-by: Bram Wubs <bram@sibi.nl>
Co-authored-by: Samuel Štancl <samuel@archte.ch>

* Add support for nested tenant config override (#920)

* feat: add support for nested tenant config override

* test: ensure nested tenant values are mapped

* Update TenantConfigTest.php

Co-authored-by: lukinovec <lukinovec@gmail.com>
Co-authored-by: Bram Wubs <megawubs@users.noreply.github.com>
Co-authored-by: Bram Wubs <bram@sibi.nl>
Co-authored-by: George Bishop <email.georgebishop@gmail.com>
Co-authored-by: Abrar Ahmad <abrar.dev99@gmail.com>
stancl added a commit that referenced this pull request Feb 20, 2023
* exclude master from CI

* Add space after 'up' in 'docker-compose up-d' (#900)

* Fix ArgumentCountError on the TenantAssetsController (#894)

* Fix ArgumentCount exception on the TenantAssetsController when no `$path` is provided

* CS

* CS

* Handle null case explicitly

* code style

Co-authored-by: Bram Wubs <bram@sibi.nl>
Co-authored-by: Samuel Štancl <samuel@archte.ch>

* Add support for nested tenant config override (#920)

* feat: add support for nested tenant config override

* test: ensure nested tenant values are mapped

* fix: typo mistake (#954)

* [3.x] Add Vite helper for tenancy (#956)

* Add Vite helper for tenancy

* Move Vite bundler to an Optional Feature

* Rename to foundation vite

* Add ViteBundlerTest

* Add missing end of file

* Update tests

* remove unnecessary end() call

Co-authored-by: Samuel Štancl <samuel@archte.ch>

* rewrite ViteBundlerTest to phpunit syntax

* skip vite test in Laravel < 9

* convert ViteBundler to PHP 7 syntax

* remove import of nonexistent class in older Laravel versions

* remove import of Foundation\Vite in tests

* try to exclude Vite.php from coverage report

* remove typehint

* update channel name

* Cache crash fix (#1048)

* Don't prevent accessing missing Tenant attributes. (#1045)

* [3.x] L10 compatibility (#1065)

* Bump dependencies for Laravel 10

* Update GitHub Actions for Laravel 10

* ci: do not test L10 using PHP 7.3

* drop < L9 support

* use `dispatch_sync` instead of `dispatch_now`

* migrate phpunit configuration

* Update ci.yml

* drop laravel < 9 support

* misc L10 fixes, new docker image

* specify odbc version

* wip

* properly list php versions as strings

* minor changes

* Add `getValue($queryGrammar)` to raw query

* Clean up `isVersion8` code

* rewrite hasFailed assertion

* phpunit schema update

* Upgrade `doctrine/dbal`

---------

Co-authored-by: Samuel Štancl <samuel@archte.ch>
Co-authored-by: Samuel Štancl <samuel.stancl@gmail.com>
Co-authored-by: lukinovec <lukinovec@gmail.com>

* Update ci.yml

* Fix code style (php-cs-fixer)

* Update dependencies

* Change invade version

* Delete ViteBundlerTest

* Fix PHPStan error

* Delete PHPStan error ignore

* Fix CONTRIBUTING.md

* Delete ViteBundler remains

* Bring back ViteBundler

* Convert ViteBundlerTest to Pest

* Update ci.yml

---------

Co-authored-by: Samuel Štancl <samuel.stancl@gmail.com>
Co-authored-by: Bram Wubs <megawubs@users.noreply.github.com>
Co-authored-by: Bram Wubs <bram@sibi.nl>
Co-authored-by: Samuel Štancl <samuel@archte.ch>
Co-authored-by: George Bishop <email.georgebishop@gmail.com>
Co-authored-by: Anbuselvan Rocky <15264938+anburocky3@users.noreply.github.com>
Co-authored-by: Wilsen Hernández <13445515+wilsenhc@users.noreply.github.com>
Co-authored-by: Joel Stein <joel@mediatrix.digital>
Co-authored-by: Guilherme Saade <saade@outlook.com.br>
Co-authored-by: PHP CS Fixer <phpcsfixer@example.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants