-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[HttpKernel] Make Kernel::getShareDir() nullable #62204
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
Conversation
In that case, app cache directory have to be set back to |
|
the changes from #62191 also need to be addressed |
nope: that's exactly the kind of config you want to spot early on: if you go with no share dir, your app cache pools have to be configured to use eg Redis by default (or you might be fine with using the cache-dir, but it shouldn't be our call by default) |
b3d273d to
9f74103
Compare
indeed, PR updated |
9f74103 to
a32ccf0
Compare
Ok, I understand the intention better now. The fallback on |
|
Sure. We don't want to disrupt the defaults. |
| // Returns $this->getCacheDir() for backward compatibility | ||
| return $this->getCacheDir(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this not be now return null; ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope: we want to null to be opt-in
|
|
||
| public function testGetShareDirDisabledByEnv() | ||
| { | ||
| $previous = $_SERVER['APP_SHARE_DIR'] ?? null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? PHPUnit does restore the $_SERVER array after each test if the backup globale feature isn’t disabled, doesn’t it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's quite possible 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTR, it's the opposite: #62273
|
Hey! I think this PR broke UX test-suite on based Symfony 8: https://github.com/symfony/ux/actions/runs/18986901497/job/54232436997?pr=3154
I'm not sure, but maybe a Composer |
|
@Kocal it's just that the change wasn't merged yet into 8.0. I did it now. |
|
Thanks, that's perfect now :) |
This allows specifying explicitly when the app has no share dir.
If the method returns
null, thekernel.share_dirparameter is not defined.This gives a nice way to spot where references to this parameter remain with
You have requested a non-existent parameter "kernel.share_dir"exceptions as a hint.Setting the
APP_SHARE_DIRenv var to any falsy value (off,false,0, etc as supported byfilter_var()) will lead to configuring the app with no share dir.