8000 [HttpFoundation] Add `use_strict_mode` in validOptions for session by sstok · Pull Request #22352 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpFoundation] Add use_strict_mode in validOptions for session #22352

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 10, 2017
Merged

[HttpFoundation] Add use_strict_mode in validOptions for session #22352

merged 1 commit into from
Apr 10, 2017

Conversation

sstok
Copy link
Contributor
@sstok sstok commented Apr 9, 2017
Q A
Branch? 3.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #9258
License MIT
Doc PR

PHP ini session.use_strict_mode was added in PHP 5.5.2 so I didn't target 2.8 as this still uses PHP 5.3. https://secure.php.net/manual/en/session.configuration.php#ini.session.use-strict-mode

@sstok sstok mentioned this pull request Apr 9, 2017
Closed
@pierredup
Copy link
Contributor

PHP ini session.use_strict_mode was added in PHP 5.5.2 so I didn't target 2.8 as this still uses PHP 5.3

You can still run SF 2.x on PHP 5.5 and want to set this option. So IMO this can go into 2.7 (earlier versions of PHP should just ignore the option if you set it)

@sstok sstok changed the base branch from 3.2 to 2.7 April 9, 2017 16:31
@sstok
Copy link
Contributor Author
sstok commented Apr 9, 2017

Rebased on 2.7.

@ghost
Copy link
ghost commented Apr 10, 2017

Is there a good reason not to enable it by default? I'm not 100% sure of the intricacies of the option, so maybe it's a bad idea.

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Apr 10, 2017
nicolas-grekas
nicolas-grekas approved these changes Apr 10, 2017
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

👍

@fabpot
Copy link
Member
fabpot commented Apr 10, 2017

Thank you @sstok.

@fabpot fabpot merged commit 130ee32 into symfony:2.7 Apr 10, 2017
fabpot added a commit that referenced this pull request Apr 10, 2017
@fabpot
bug #22352 [HttpFoundation] Add use_strict_mode in validOptions for…
63592a6 
… session (sstok)

This PR was merged into the 2.7 branch.

Discussion
----------

[HttpFoundation] Add `use_strict_mode` in validOptions for session

| Q             | A
| ------------- | ---
| Branch?       | 3.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #9258
| License       | MIT
| Doc PR        |

PHP ini `session.use_strict_mode` was added in PHP 5.5.2 so I didn't target 2.8 as this still uses PHP 5.3. https://secure.php.net/manual/en/session.configuration.php#ini.session.use-strict-mode

Commits
-------

130ee32 Add `use_strict_mode` in validOptions for session
@sstok sstok deleted the patch-2 branch April 10, 2017 15:41
This was referenced May 1, 2017
Merged
Merged
Merged
@MacDada
Copy link
Contributor
MacDada commented May 17, 2017 •
edited
Loading

How do I configure this?

I've tried:

#config.yml
framework:
    session:
        use_strict_mode: 1

But it throws InvalidConfigurationException.

And it's not defined in the config: https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L696

@MacDada
Copy link
Contributor
MacDada commented May 17, 2017

The InvalidConfigurationException is thrown, because the option (and many other) is not defined here: https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L696

Make a PR?

MacDada added a commit to MacDada/symfony that referenced this pull request May 17, 2017
@MacDada
Sessions: configurable "use_strict_mode" option for NativeSessionStorage
6b640a7 
symfony#22352 (comment)
@MacDada MacDada mentioned this pull request May 17, 2017
Merged
MacDada added a commit to MacDada/symfony that referenced this pull request Jun 16, 2017
@MacDada
Sessions: configurable "use_strict_mode" option for NativeSessionStorage
90e192e 
symfony#22352 (comment)
fabpot added a commit that referenced this pull request Jun 18, 2017
@fabpot
bug #22730 [FrameworkBundle] Sessions: configurable "use_strict_mode"…
ac9a527 
… option for NativeSessionStorage (MacDada)

This PR was merged into the 2.7 branch.

Discussion
----------

[FrameworkBundle] Sessions: configurable "use_strict_mode" option for NativeSessionStorage

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT

It is currently not possible to configure the `use_strict_mode` option for `NativeSessionStorage` in a proper manner.

The reason of this PR: #22352 (comment)

It could be considered a new feature, but I wish it wouldn't, as I don't want to do any ugly hacking to get it working.

What else could be done?

* implement more options from `NativeSessionStorage` in the config?
* get rid of duplication somehow (maybe a static method in `NativeSessionStorage` that would return the option list and could be used in `FrameworkExtension`?)
* update `FrameworkExtensionTest`?
* update `ConfigurationTest`?
* update [the docs](https://symfony.com/doc/current/reference/configuration/framework.html#session)?

I'm willing to do those if decided.

Commits
-------

90e192e Sessions: configurable "use_strict_mode" option for NativeSessionStorage
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Jun 18, 2017
@MacDada
Sessions: configurable "use_strict_mode" option for NativeSessionStorage
3edff42 
symfony/symfony#22352 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Bug HttpFoundation Status: Needs Review
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.
6 participants
@sstok @pierredup @fabpot @MacDada @nicolas-grekas @carsonbot
0