shield icon indicating copy to clipboard operation
shield copied to clipboard

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

Open kenjis opened this issue 3 years ago • 4 comments

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().

kenjis avatar May 31 '22 08:05 kenjis

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.

MGatner avatar May 31 '22 11:05 MGatner

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.

lonnieezell avatar May 31 '22 13:05 lonnieezell

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

kenjis avatar May 31 '22 21:05 kenjis

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.

lonnieezell avatar Jun 01 '22 03:06 lonnieezell