Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Process] Fix broken tests for PHP 7.2#24516
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
sroze commentedOct 11, 2017
Status: needs work |
| env:deps=high | ||
| -php:7.1 | ||
| env:deps=low | ||
| -php:7.2 |
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 add another line:
- php: 7.2- php: 7.2 env: deps=low5be2f4c to1677777Comparestof commentedOct 11, 2017
you should avoid it being marked as an intermediary version though. You don't want it to be skipped. |
c891a90 tob48b20aComparesroze commentedOct 12, 2017
@stof do you think we should prevent 7.2 to be marked as intermediate for all the PRs or it was just in the context of my debugging? |
sroze commentedOct 12, 2017
Status: Needs Review |
| if (\PHP_SESSION_ACTIVE ===session_status()) { | ||
| if (!empty($options) ||null !==$handler) { | ||
| thrownew \LogicException('Cannot change options or handler of an active session'); |
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.
Do we really to throw here ? maybe a test should be added to test this behaviour ?
| -php:7.1 | ||
| env:deps=high | ||
| -php:7.1 | ||
| -php:7.2 |
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.
maybe we could add without deps=low too
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.
nope: the matrix is optimized to test the maximum of cases with the minimum number of jobs
sroze commentedOct 12, 2017
@nicolas-grekas as discussed, kept here only the process test change and travis configuration and moved the native session storage PR to#24531 to target 2.7. |
sroze commentedOct 12, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
And rebased. Tests are logically still failing because of the HttpFoundation bug fixed in#24531. |
sroze commentedNov 6, 2017
Rebased and now 💚. /cc@xabbuh |
nicolas-grekas commentedNov 6, 2017
Thank you@sroze. |
This PR was squashed before being merged into the 4.0-dev branch (closes#24516).Discussion----------[Process] Fix broken tests for PHP 7.2| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | no| Fixed tickets |#24524,#24515| License | MIT| Doc PR | øFollowing#24515, trying to fix Process tests with PHP 7.2Commits-------b410a36 [Process] Fix broken tests for PHP 7.2
Uh oh!
There was an error while loading.Please reload this page.
Following#24515, trying to fix Process tests with PHP 7.2