Skip to content

[FrameworkBundle] Remove default value for gc_probability config option #58165

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

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? 7.2
Bug fix? no
New feature? no
Deprecations? no
Issues -
License MIT

While playing on a test app, I experienced an error related to the session GC:

Notice: SessionHandler::gc(): ps_files_cleanup_dir: opendir(/var/lib/php/sessions) failed: Permission denied (13)

This is triggered by StrictSessionHandler calling the gc() method of the native session handler.

I figured out the GC was running with 1/1440 probability so I tried increasing the probability to reproduce. I did so patching my ini settings and this did nothing, until I figured out that the corresponding option shadows the ini settings.

This was done 10 years ago in #10366 (/cc @fabpot) to fix #10349. Re-reading that issue, I think it doesn't apply anymore: by default, we now encourage storing sessions in the folder configured in the ini settings also.

Let's revert that PR.

Then, what about the error itself? It happens because the folder configured on my Ubuntu doesn't have the x permission, so that the session GC cannot list its content. This is consistent with session.gc_probability being set to 0. My host relies on cron instead of this GC. Which means there's nothing else to fix actually.

@carsonbot carsonbot added this to the 7.2 milestone Sep 3, 2024
@OskarStark OskarStark changed the title [FrameworkBundle] Remove default value for gc_probability config option [FrameworkBundle] Remove default value for gc_probability config option Sep 3, 2024
@javiereguiluz
Copy link
Member

Just asking so we can properly document this: the change means that the new default value of Symfony's session.gc_probability option (https://symfony.com/doc/current/reference/configuration/framework.html#gc-probability) is the value of the PHP's session. gc_probability INI setting (https://www.php.net/manual/en/session.configuration.php#ini.session.gc-probability)?

@fabpot
Copy link
Member

fabpot commented Sep 4, 2024

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 14e99f0 into symfony:7.2 Sep 4, 2024
8 of 10 checks passed
@nicolas-grekas nicolas-grekas deleted the native-gc_probability branch September 4, 2024 16:26
@nicolas-grekas
Copy link
Member Author

@javiereguiluz 💯 this will make it behave like the other session settings

@fabpot fabpot mentioned this pull request Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gc_probability and cache sessions directory growing endlessly
4 participants