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 fallback for Config\Cookie #4625

Merged

Conversation

paulbalandan
Copy link
Member

Description
Fixes #4619

Checklist:

  • Securely signed commits

@paulbalandan paulbalandan merged commit 8a1f8be into codeigniter4:develop May 1, 2021
@paulbalandan paulbalandan deleted the cookie-config-bugfix branch May 1, 2021 11:48
@sfadschm
Copy link
Contributor

sfadschm commented Sep 30, 2021

Dumb question which might be related:
If someone sees this in Config\App :

* @deprecated use Config\Cookie::$prefix property instead.

and decides to use Config\Cookie instead as suggested then goes on and deletes the property from App\Config, he will get an error from HTTP\Response:
$this->cookiePrefix = $config->cookiePrefix;
$this->cookieDomain = $config->cookieDomain;
$this->cookiePath = $config->cookiePath;
$this->cookieSecure = $config->cookieSecure;

Should these assignements not try to read the (deprecated) property from Config\App and if it is not there, get it from Config\Cookie? Like done here:
$cookiePrefix = $cookie->prefix ?? $config->cookiePrefix;

@paulbalandan
Copy link
Member Author

IMO, it is not the framework's problem. Those properties are only deprecated, so technically they are still supported until the next major. If anyone deletes them intentionally then they are inflicting upon themselves their own breaking changes. The cost of providing workarounds for possible removal of deprecated members in userland then removing those workarounds in next major version is not worth the benefit.

@paulbalandan
Copy link
Member Author

Those assignments in Response are practically useless as those aren't used anymore. Those are there for the sake of BC preservation for users still using them.

@sfadschm
Copy link
Contributor

Alright, I guess I misunderstood the principle of deprecations. However, still feels weird that this seems to be handled differently across the framework.

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.

Bug: Missing Cookie Config
4 participants