codeigniter4 / CodeIgniter4

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
5.38k stars 1.9k forks source link

Missing config options and config options repetition #4504

Closed zoruz closed 3 years ago

zoruz commented 3 years ago

CI VERSION: V4.1.1

Issue 01

Missing DBPrefix and sessionExpiration config items on env file. Of course we can add them by ourselves. But its nice these options already included in the env file.

Issue 02

I see CSRF related config items get repeated in env file and also get repeated in app and security config files. I wonder why same config items get repeated on two places. If its due to breaking changes, Then add deprecated notice on both config files and env file.

zoruz commented 3 years ago

I know this is not a bug. But I posted on here because on the forum its hardly get attention.

kenjis commented 3 years ago

Yes, the CSRF related config items below are deprecated. https://github.com/codeigniter4/CodeIgniter4/blob/5c23a60c573835523e4011f1fd1656c5b8638252/env#L42-L48

When do we remove them in the env file?

zoruz commented 3 years ago

Need to remove from both env file config/app file. If can't be removed, add 'deprecated' notice comment on both files. So its clear we dont have to mess with them.

paulbalandan commented 3 years ago

Issue 01

Missing DBPrefix and sessionExpiration config items on env file. Of course we can add them by ourselves. But its nice these options already included in the env file.

We can have those included in the shipped env file.

Issue 02

I see CSRF related config items get repeated in env file and also get repeated in app and security config files. I wonder why same config items get repeated on two places. If its due to breaking changes, Then add deprecated notice on both config files and env file.

This repetition is caused by Config\App divesting its various configurations to their specific components in separate classes. The App version is being deprecated in favor of this new classes. For the env I do not see removing them as "breaking change" because users have their own .env (note the preceding dot). The only issue I see is when they copy env to .env then add their own values they might wonder why app.CSRFProtection is now missing.

Personally, I would vote to remove the deprecated options in the shipped env to "enforce" users to back away using the deprecated options. Maybe some docs explaining this change would be nice. Any thoughts, @MGatner ?

MGatner commented 3 years ago

Yes, absolutely we can do anything to env and should. This should always be our "ideal template": not including ever possible Config value, but some helpful suggestions (and no deprecated items).

John-Betong commented 3 years ago

On Thu, 1 Apr 2021 at 05:51 MGatner @.***> wrote:

Yes, absolutely we can do anything to env and should. This should always be our "ideal template": not including ever possible Config value, but some helpful suggestions (and no deprecated items).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/codeigniter4/CodeIgniter4/issues/4504#issuecomment-811516228, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABH5O2LYACMUHT3IAZNLD3TGORNNANCNFSM42BBMNJA .

Could an alternative ViewPath be set in the .env configuration file?

-- Sent from Gmail Mobile

MGatner commented 3 years ago

alternative ViewPath

Not as is. app/Config/Paths.php is not a CodeIgniter\Database\Config, because it is loaded pretty much before everything else. I could imagine adding some checks but at some point we need to have a "bottom line".

paulbalandan commented 3 years ago

@zoruz could you do a follow up PR to remove the deprecated items in the env so we can close this ticket?

MGatner commented 3 years ago

@npwsamarasinghe CSRF moved to the Security Config. We've left the references in App to prevent breaking changes, but all references in the framework should point to Security now. If you see tests that need updating feel free to point them out.

paulbalandan commented 3 years ago

@npwsamarasinghe can you link here the commit hash that removed that config item? We may need to check that.