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

[5.0] Task parameter SessionGC #41608

Closed
heelc29 opened this issue Sep 5, 2023 · 10 comments
Closed

[5.0] Task parameter SessionGC #41608

heelc29 opened this issue Sep 5, 2023 · 10 comments

Comments

@heelc29
Copy link
Contributor

heelc29 commented Sep 5, 2023

Steps to reproduce the issue

Are the parameters for Divisor and Probability still necessary after convert plugin to task (#41326) where it is possible to set execution rules?
image

@richard67
Copy link
Member

It might depend on the configured session handler, if file or database. I think for file they might still be needed because these parameters are passed to php built in methods, and php handles deletion. For database where Joomla handles deletion they might not be necessary. But I am not really sure, I just remember something like that.

@heelc29
Copy link
Contributor Author

heelc29 commented Sep 5, 2023

I only found references in the plugin itself

$probability = (int) $event->getArgument('params')->gc_probability ?? 1;
$divisor = (int) $event->getArgument('params')->gc_divisor ?? 100;
$random = $divisor * lcg_value();
if ($probability > 0 && $random < $probability) {
$this->metadataManager->deletePriorTo(time() - $this->getApplication()->getSession()->getExpire());
}

@richard67
Copy link
Member

richard67 commented Sep 5, 2023

@heelc29 I found this but don’t have the time to check in detail now:

https://www.php.net/manual/en/function.session-gc.php

$this->getApplication()->getSession()->gc();

@heelc29
Copy link
Contributor Author

heelc29 commented Sep 6, 2023

If i see it right this function ($this->getApplication()->getSession()->gc()) calls only session_gc in the session framework
https://github.com/joomla-framework/session/blob/95c74bd3c8546b73c49440c426eef734a30ea97b/src/Storage/NativeStorage.php#L134

And the task parameter have nothing to do with the php parameter
https://www.php.net/manual/en/session.configuration.php#ini.session.gc-probability

If I search in the code there is only one result: session.gc_maxlifetime

@richard67
Copy link
Member

@heelc29 From what I can see you are right. It was once implemented in Joomla 3.8.6 in that way so it behaves same as the PHP sessiongc, as far as I understand: #19687 . But the parameters are not really related to those used by PHP, and now as we have a scheduled task I also think it should not need that.

However, I am not really an expert on our session handling. I only took the task to finish the scheduler task plugins for J5 because of the beta 1 with the feature freeze being so close and the author of the PRs having been busy with his regular, daily job. So for me it would be ok if someone else takes that task and fixes that. But it should be tested with real tests, not just by code review, so it needs to write good testing instructions.

@heelc29
Copy link
Contributor Author

heelc29 commented Sep 6, 2023

I am not really an expert on our session handling

Me neither

@HLeithner
Copy link
Member

Parameters can be removed, they are only used to not run the garbage collection with each request.

@richard67
Copy link
Member

Parameters can be removed, they are only used to not run the garbage collection with each request.

@HLeithner Can the corresponding language strings be removed, too? Or shall they be deprecated?

@HLeithner
Copy link
Member

can be removed too since they are new in j5

@richard67 richard67 added the bug label Sep 7, 2023
@alikon
Copy link
Contributor

alikon commented Sep 8, 2023

i was thinking at this when i've started to write the pr #41326, but at some point i forgot, i'll do a pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants