codeigniter4 / shield

Authentication and Authorization for CodeIgniter 4
https://shield.codeigniter.com
MIT License
366 stars 133 forks source link

`config('Auth')` or `setting('Auth...')` #190

Closed kenjis closed 1 year ago

kenjis commented 2 years ago

https://github.com/codeigniter4/shield/search?q=config%28%27Auth%27%29&type=code

Do these need to be replaced with setting()?

I'm not sure when to use config() and when to use setting().

MGatner commented 2 years ago

Anything that needs the actual Config class must use config(). Using setting() returns an individual value only, checking the database for an override first. I will defer to @lonnieezell on what we want to allow to be overridden, but my guess is "everything".

It's still on my list to make a "Registrar adapted" for the Settings library so these changes happen automatically without having to use the function call on every item.

lonnieezell commented 2 years ago

I agree mostly with what @MGatner said - if it needs the class it has to be config otherwise settings. The only part it might make sense to keep config is in the tests, though I can't see these getting ran within someone's project so it's probably fine to change them also.

Oh - and places where it's calling a method of the config class need to stay config, also, like config('Auth')->logoutRedirect().

In general with these I try to think of what types of things will be changed within the UI for a config file and make sure those are changed. However, with a big public library like this, everything that's possible should probably be using setting to allow developers the most flexibility when developing their UIs.

kenjis commented 2 years ago

Then, do you mean these should be replaced with setting()?

https://github.com/codeigniter4/shield/blob/8c507cf6f94b2537e9ed261e0ad8e7c00dc31b09/src/Authentication/Passwords/ValidationRules.php#L90-L91

https://github.com/codeigniter4/shield/blob/8c507cf6f94b2537e9ed261e0ad8e7c00dc31b09/src/Views/email_2fa_show.php#L1

lonnieezell commented 2 years ago

I'd say sure, though I didn't do anything related to views originally since I didn't see a use case for that, honestly.