Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

sroze
Copy link
Contributor

@sroze sroze commented Oct 11, 2017

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.2

@sroze
Copy link
Contributor Author

sroze commented Oct 11, 2017

Status: needs work
Reason: tests are not actually running and it's only running PHP 7.2 for now

.travis.yml Outdated
env: deps=high
- php: 7.1
env: deps=low
- php: 7.2
Copy link
Member

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=low

@sroze sroze force-pushed the process-and-php72 branch 3 times, most recently from 5be2f4c to 1677777 Compare October 11, 2017 13:12
@sroze sroze changed the title PHP 7.2 Process tests Process & HttpFoundation tests for PHP 7.2 Oct 11, 2017
@stof
Copy link
Member

stof commented Oct 11, 2017

you should avoid it being marked as an intermediary version though. You don't want it to be skipped.

@sroze
Copy link
Contributor Author

sroze commented Oct 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
Copy link
Contributor Author

sroze commented Oct 12, 2017

Status: Needs Review


if (\PHP_SESSION_ACTIVE === session_status()) {
if (!empty($options) || null !== $handler) {
throw new \LogicException('Cannot change options or handler of an active session');
Copy link
Contributor

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 ?


matrix:
include:
- php: 7.1.3
- php: 7.1
env: deps=high
- php: 7.1
- php: 7.2
Copy link
Contributor

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

Copy link
Member

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 sroze changed the title Process & HttpFoundation tests for PHP 7.2 [Process] Fix broken tests for PHP 7.2 Oct 12, 2017
@sroze
Copy link
Contributor Author

sroze commented Oct 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 sroze force-pushed the process-and-php72 branch from 04164e2 to f764735 Compare October 12, 2017 11:02
@sroze
Copy link
Contributor Author

sroze commented Oct 12, 2017

And rebased. Tests are logically still failing because of the HttpFoundation bug fixed in #24531.

@sroze sroze force-pushed the process-and-php72 branch from f764735 to dd39cb5 Compare November 6, 2017 08:16
@sroze
Copy link
Contributor Author

sroze commented Nov 6, 2017

Rebased and now 💚.

/cc @xabbuh

@nicolas-grekas
Copy link
Member

Thank you @sroze.

nicolas-grekas added a commit that referenced this pull request Nov 6, 2017
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.2

Commits
-------

b410a36 [Process] Fix broken tests for PHP 7.2
@sroze sroze deleted the process-and-php72 branch November 6, 2017 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants