Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpFoundation] Fix setting session-related ini settings#27067
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
nicolas-grekas left a comment
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.
thanks for the PR, here are some minor comments
| $validOptions =array_flip(array( | ||
| 'cache_limiter','cookie_domain','cookie_httponly', | ||
| 'cache_limiter','cache_expire','cookie_domain','cookie_httponly', |
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.
can you move it after cookie_domain? would make it alpha order
| if (isset($validOptions[$key])) { | ||
| ini_set('session.'.$key,$value); | ||
| // All options starts with 'session.' except 'url_rewriter.tags' | ||
| $name ='url_rewriter.tags' !=$key ?'session.'.$key :$key; |
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 use!==
| ini_set('session.'.$key,$value); | ||
| // All options starts with 'session.' except 'url_rewriter.tags' | ||
| $name ='url_rewriter.tags' !=$key ?'session.'.$key :$key; | ||
| ini_set($name,$value); |
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.
let's inline?ini_set('url_rewriter.tags' !== $key ? 'session.'.$key : $key, $value);
| foreach ($optionsas$key =>$value) { | ||
| if (isset($validOptions[$key])) { | ||
| ini_set('session.'.$key,$value); | ||
| // All options starts with 'session.' except 'url_rewriter.tags' |
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.
I think we should remove this comment as it just duplicates what the code tells quite evidently
| $this->getStorage($options); | ||
| $this->assertEquals('a=href',ini_get('url_rewriter.tags')); | ||
| $this->assertEquals(200,ini_get('session.cache_expire')); |
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 use assertSame in both cases
| $this->getStorage($options); | ||
| $this->assertEquals('a=href',ini_get('url_rewriter.tags')); |
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.
This fails on HHVM. I suggest to skip this test on HHVM.:
if (defined('HHVM_VERSION')) {$this->markTestSkipped('HHVM is not handled in this test case.');}
e-moe commentedApr 27, 2018
@nicolas-grekas , thanks for your review! updated |
Tobion commentedApr 28, 2018
Good catch, thanks@e-moe. |
…(e-moe)This PR was merged into the 2.7 branch.Discussion----------[HttpFoundation] Fix setting session-related ini settings| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#27011| License | MIT| Doc PR | n/aAdded missed option `cache_expire`Fixed typo in `upload_progress.min_freq`Fixed ini_set name prefix of `url_rewriter.tags`Commits-------64a0f23Fix#27011: Session ini_set bug
Added missed option
cache_expireFixed typo in
upload_progress.min_freqFixed ini_set name prefix of
url_rewriter.tags