[2.3][SECURITY] Add remember me cookie configuration #14491
Conversation
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #14490 |
| License | MIT |
| Doc PR |
|
Apparently this build fails on Jenkins, but it appears to be on some unrelated tests. Any ideas how to proceed here? |
| 1, | ||
| $this->options['path'], | ||
| $this->options['domain'], | ||
| isset($this->options['secure']) ? $this->options['secure'] : false, |
There was a problem hiding this comment.
What about using array_merge in constructor to be sure that this options always exists?
There was a problem hiding this comment.
But what is the advantage of that?
If an array_merge should be added, it would not have to be here, but at the point of reading the the config file imo
There was a problem hiding this comment.
Big advantage for me: I see full default configuration in the constructor. So no need scroll down to line no 296 for getting default value of secure parameter. Also I see full list of options that available in this class.
There was a problem hiding this comment.
It is common practice
There was a problem hiding this comment.
Okay, I see your point @Koc.
Updated the PR!
|
So all checks passed this time, not sure what the next steps are. |
|
Can you add some tests to prevent future breakage? Status: needs work |
|
@sstok I'll look in to it. |
|
@sstok test has been added. |
|
Apply the Fabbot patch and this should be good http://fabbot.io/report/symfony/symfony/14491/78eb0fd7b5cd9554943c610e315441715c3b629f#patchcs |
|
@sstok yeah sorry about the standards, it's applied, redoing tests. |
|
What will be the necessary steps to get this on the newer versions of Symfony? (2.7, 2.8) |
|
Any idea why this happened? https://ci.appveyor.com/project/fabpot/symfony/build/1.0.1172/job/itg8ut8oqr82912s |
|
The 2.3 branch is merged into the 2.7 and eventually 2.8 and master on a weekly basis. Status: Reviewed |
|
@sstok can you fill me in on what the next steps are? Do I just wait until it gets merged in, or do I have to do something else myself? |
|
@klaascuvelier I am sorry that your pull request has been overlooked. However, your solution has the advantage that it does not fail when one of the new options is omitted. Would you mind rebasing your PR on the latest |
|
@klaascuvelier Imho you ignore those failures for the moment and just rebase your changes on the latest commit of the |
|
#16141 has been merged now. |
|
@klaascuvelier Well, it looks a bit strange that your PR contains the commit made by @nicolas-grekas. However, the diff looks right. Could you please also revert the changes to the test classes made in #14842 where the default values are set for the options? |
|
That's because you did a merge, not a rebase. We need a rebase to keep the history clean. |
|
I did a rebase from 2.3, like described here: http://symfony.com/doc/current/contributing/code/patches.html#step-3-submit-your-patch |
|
Can you tell me what exactly you want me to do, to fix this mess-up? |
|
This should work: From your branch. |
|
Okay, I did a new rebase, PR looks better now. |
|
This looks better. Could you also modify the test classes as described in #14491 (comment)? |
| $this->options['secure'], | ||
| $this->options['httponly'] | ||
| ) | ||
| ); |
|
Alright, fixed both of your remarks as well. |
|
They should be revert in |
|
Anything I can do about the failing tests? |
|
Nope, they're going to be fixed by other PRs. |
|
Thank you @klaascuvelier. |
…aascuvelier) This PR was squashed before being merged into the 2.3 branch (closes #14491). Discussion ---------- [2.3][SECURITY] Add remember me cookie configuration | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14490 | License | MIT | Doc PR | Commits ------- e8f0e5a [2.3][SECURITY] Add remember me cookie configuration