feat: Settings - Decouple from shield #1220
Conversation
|
|
||
| if (! function_exists('shieldSetting')) { | ||
| /** | ||
| * Provides a wrapper function for settings module. | ||
| * | ||
| * @param mixed $value | ||
| * | ||
| * @return array|bool|float|int|object|string|void|null | ||
| * @phpstan-return ($value is null ? array|bool|float|int|object|string|null : void) | ||
| */ | ||
| function shieldSetting(?string $key = null, $value = null) | ||
| { | ||
| /** @var AuthConfig $config */ | ||
| $config = config('Auth'); | ||
| if($config->useSettings) { | ||
| return setting($key, $value); | ||
| } | ||
|
|
||
| // Getting the value? | ||
| if (!empty($key) && count(func_get_args()) === 1) { | ||
| $parts = explode('.', $key); | ||
| if (count($parts) === 1) { | ||
| throw new InvalidArgumentException('$key must contain both the class and field name, i.e. Foo.bar'); | ||
| } | ||
| return config($parts[0])?->{$parts[1]} ?? null; | ||
| } | ||
|
|
||
| throw new InvalidArgumentException('Settings library is not being used for shield'); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the important part. With the proposed change call to setting() method will only occur here.
najdanovicivan
changed the title
datamweb
changed the title
|
Yes, it would be great to decouple Shield and Settings packages. Personally, I don't see a lot of sense of having the Settings dependency here, because CodeIgniter already has BaseConfig to handle settings. |
najdanovicivan
force-pushed
the
decouple-settings
branch
from
0326f5d to
6453789
Compare
|
@datamweb What else is needed for this one to be merged ? |
|
@najdanovicivan I'm currently on mobile and need to review this PR on my PC. I'll get back to it soon. |
|
|
||
| // Get the credentials for login | ||
| $credentials = $this->request->getJsonVar(setting('Auth.validFields')); | ||
| $credentials = $this->request->getJsonVar(shieldSetting('Auth.validFields')); |
There was a problem hiding this comment.
I believe this should be in snake case per our coding standards: shield_setting
| * @return array|bool|float|int|object|string|void|null | ||
| * @phpstan-return ($value is null ? array|bool|float|int|object|string|null : void) |
There was a problem hiding this comment.
In what instance will this return void?
| * @return array|bool|float|int|object|string|void|null | |
| * @phpstan-return ($value is null ? array|bool|float|int|object|string|null : void) | |
| * @return mixed |
| * @return array|bool|float|int|object|string|void|null | ||
| * @phpstan-return ($value is null ? array|bool|float|int|object|string|null : void) | ||
| */ | ||
| function shieldSetting(?string $key = null, $value = null) |
There was a problem hiding this comment.
| function shieldSetting(?string $key = null, $value = null) | |
| function shield_setting(?string $key = null, mixed $value = null): mixed |
| { | ||
| /** @var AuthConfig $config */ | ||
| $config = config('Auth'); | ||
| if($config->useSettings) { |
There was a problem hiding this comment.
Please add a function_exists check
| } | ||
|
|
||
| // Getting the value? | ||
| if (!empty($key) && count(func_get_args()) === 1) { |
There was a problem hiding this comment.
What's the use case for having null key? Would it be better to just require a string key?
| if (count($parts) === 1) { | ||
| throw new InvalidArgumentException('$key must contain both the class and field name, i.e. Foo.bar'); | ||
| } | ||
| return config($parts[0])?->{$parts[1]} ?? null; |
There was a problem hiding this comment.
The ?-> would return null if config($parts[0]) is not an object so the ?? null is useless.
| return config($parts[0])?->{$parts[1]} ?? null; | ||
| } | ||
|
|
||
| throw new InvalidArgumentException('Settings library is not being used for shield'); |
There was a problem hiding this comment.
This capturing exception feels off. This would be reached if:
Auth::$useSettingsis true butsettingsis not installed.$useSettingsis false but$keyis null or empty string.$useSettingsis false,$keyis non-empty-string, and $value is provided
These different scenarios should be tackled separately within each conditional above.
| "ext-curl": "Required to use the password validation rule via PwnedValidator class.", | ||
| "ext-openssl": "Required to use the JWT Authenticator." | ||
| "ext-openssl": "Required to use the JWT Authenticator.", | ||
| "codeigniter4/settings": "Required to store groups and permissions in database" |
There was a problem hiding this comment.
Add this also to require-dev
| } | ||
| } | ||
|
|
||
| if (! function_exists('shieldSetting')) { |
There was a problem hiding this comment.
Please add a test for this new function.
paulbalandan
added
tests needed
Description
Decouple settings library so it is not required.
Related to #1215
Checklist: