Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[2.3][HttpFoundation] PDO Session handling enhancements#7634
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
For webhosts that have ini_set restricted (for security purposes), don't just blindly use ini_set if the INI setting is already set to what we need.
The rest of the Handler class assumes that a PDOException gets thrown when there's an issue, but doesn't enforce that requirement. This change explicitly sets that attribute.
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.
please add the curly braces
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.
the caller of this code - which created the pdo connection - may not like the fact that pdo will throw exceptions after creating this handler with it..
I think it would be better and less offensive to throw an exception when the ERRMODE is not EXCEPTION - than the user may decide if he likes to switch the ERRMODE (and therfore may change his other code which relies on his pdo connection and this setting) or not.
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.
Sure thing; that change is added in now!
Don't just change the PDO object attribute without asking; throw an InvalidArgument Exception instead.Check that the ini_set() call succeeded after its called.
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.
the message should be more explicit, so a users knows what todo, e.g.
sprintf('"%s" requires PDO connection attribute PDO::ATTR_ERRMODE set to PDO::ERRMODE_EXCEPTION',__CLASS__)
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.
Sure thing; added in a more descriptive error, with class name, and code fragment to update the PDO error mode. Along the same reasoning, should the error message two lines above be updated to use the class name as well, since "a PdoSessionStorage" doesn't match the class name (PdoSessionHandler)?
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.
you should add a unit test proving the exception is thrown as expected when ERRMODE differes from EXCEPTION
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.
Good point; I added in a few more unit tests for checking that situation, and ensuring a few other runtime errors are indeed getting thrown when they should.
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.
You should use!==
When making a comparison, use the strictest comparison possible, and put the literal comparator first.
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.
Please revert this to justini_set('session.use_cookies', 1);
ghost commentedApr 15, 2013
@MidnightLightning - could you please add a CHANGELOG.md entry for this (it's in the HttpfFoundation component root. |
ghost commentedApr 20, 2013
@fabpot - this PR seems ok to merge IMO |
PdoSessionHandlerclass assumes that the PDO object is set to throw exceptions, not errors. I added a line in the constructor to set that attribute, so configuration/query errors are able to be seen and caught a lot easier.