Skip to content

[RFC] Precise session management #1734

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

Closed

Conversation

yohgaki
Copy link
Contributor

@yohgaki yohgaki commented Jan 22, 2016

This PR is for "Precise session management" RFC.
https://wiki.php.net/rfc/precise_session_management

Current Status:
All features/changes are implemented. Please report issues if there are.

TODO:
Fix test dependency issues by fundamental test updates.

Yasuo Ohgaki added 4 commits January 10, 2016 09:48
…ION__"

is in serialized data. Other than this, basic feature works.
TODO:
 - Add session internal data validation, otherwise broken data can crash PHP.
 - Remove dependency from previous tests and/or other PHP versions.
 - Use long parameter for session_destroy([long $duration_for_removing]).
@laruence laruence added the RFC label Jan 22, 2016
Yasuo Ohgaki added 25 commits January 22, 2016 19:45
…hgaki/php-src into master-rfc-precise-session-management
…er by default (already done in ini). Support disabling automatic session ID regeneration. Update php.ini-production/development.
…initialization and check in session_decode()
…t more robust fix will follow. This will be done when this is merged
RETURN_BOOL(php_session_destroy() == SUCCESS);
if (immediate) {
/* Immediate destroy may cause random lost session by server side race condition. */
php_error_docref(NULL, E_NOTICE, "Immediate session data removal may cause random lost sessions. It is advised not to delete immediately");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a total no-go. A notice for an intentionally chosen behavior (explicitly passing true).
Warn about it in docs, but definitely not force the user to use the @ operator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Let make it documentation issue.

@bwoebi
Copy link
Member

bwoebi commented Feb 14, 2016

In general, please do not convert to E_RECOVERABLE_ERROR, but use an exception, when you want them to be catchable.

@yohgaki
Copy link
Contributor Author

yohgaki commented Feb 15, 2016

It's OK for me to convert E_RECOVERABLE_ERROR to exceptions. Current session module does not have any exception. I'm not sure which one should be used. If nobody insist, I'll change them to exceptions.


if (PS(session_status) != php_session_active) {
php_error_docref(NULL, E_WARNING, "Session is not active");
RETURN_FALSE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general 'etiquette' is that when there is no data, "NULL" should be returned, and not FALSE. Especially in error conditions like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I'll change the return type.

@nikic
Copy link
Member

nikic commented Jul 10, 2016

Closing as the RFC has been declined (and is now split in parts).

@nikic nikic closed this Jul 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants