8000 Fix error with unset properties on ThrowMatcher by jmleroux · Pull Request #1401 · phpspec/phpspec · GitHub
[go: up one dir, main page]
Skip to content

Fix error with unset properties on ThrowMatcher #1401

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 3 commits into from
Mar 7, 2022
Merged

Fix error with unset properties on ThrowMatcher #1401

merged 3 commits into from
Mar 7, 2022

Conversation

jmleroux
Copy link
Contributor
@jmleroux jmleroux commented Nov 22, 2021 •
edited
Loading

The ThrowMatcher could crash when class properties are unset. Alas, the isInitialized method only exists since PHP 7.4 and the fix will not work in PHP 7.3.

This behavior was detected after this Symfony PR: symfony/symfony#44037 related to the use of dynamic properties.

@jmleroux jmleroux mentioned this pull request Nov 22, 2021
Merged
@ciaranmcnulty
Copy link
Member

Is there a test that could accompany this?

@jmleroux
Copy link
Contributor Author
jmleroux commented Dec 1, 2021 •
edited
Loading

Is there a test that could accompany this?

👍

I'll try to add one, I was waiting for the CI activation.

@jmleroux
Copy link
Contributor Author
jmleroux commented Dec 1, 2021

Would you accept the addition of a Dockerfile and/or docker-compose file in this PR?

It could facilitate the tests on different PHP versions for example

@ciaranmcnulty
Copy link
Member

I'd rather keep a dockerfile out of it; we manage ok on Github Actions testing different versions and don't want things in two places

@jmleroux
Copy link
Contributor Author
jmleroux commented Dec 3, 2021

FYI, I cannot yet reproduce the case with a simple test when I still can reproduce it on Akeneo.
Work in progress.

@jmleroux jmleroux marked this pull request as ready for review January 7, 2022 00:33
@jmleroux
Copy link
Contributor Author
jmleroux commented Jan 7, 2022 •
edited
Loading

I added a Behat scenario to test the modification.
For a reason I still not identify, the previous ThrowMatcher will still pass the test in PHP 7.4, but will fail on PHP 8.0. PHP 8.0 may be more strict with uninitialized reflection values.
That's why it took me so much time to finalize this PR because I was trying to validate my test with PHP 7.4

@jmleroux jmleroux mentioned this pull request Jan 10, 2022
Closed
@jmleroux
Copy link
Contributor Author
jmleroux commented Jan 19, 2022 •
edited
Loading

Hello,
Is there a problem with my PR?
I'd like to see it running on the real CI and I am waiting for it because i'm stuck with my specs on one private project after migrating to PHP 8.0 😉

@ 8000 shouze
Copy link
shouze commented Jan 19, 2022

Hello,
Is there a problem with my PR?
I'd like to see it running on the real CI and waiting for it because i'm stuck with my specs on one private project after migrating to PHP 8.0 😉

I would like too ❤️

@jmleroux
Copy link
Contributor Author

I would like too

Hello @shouze ,
Did you have the opportunity to validate it on your side?

@jmleroux
Copy link
Contributor Author

Hello guys,
Is there a problem with this PR or is this inactivity normal?

@ciaranmcnulty
Copy link
Member

@jmleroux no problem with the PR, looks good thank you

@ciaranmcnulty ciaranmcnulty merged commit e070f01 into phpspec:main Mar 7, 2022
@ciaranmcnulty
Copy link
Member

Weirdly this is failing on main for the older PHP versions... taking a look

@jmleroux
Copy link
Contributor Author
jmleroux commented Mar 9, 2022

Weird, I think I tested it. I even added a dummy PR to test without the fix: #1409
Do not hesitate to ping me if I can help.

Note: I don't have several PHP version locally and didn't have access to Travis for this PR, so I tested with several docker files.

@ciaranmcnulty
Copy link
Member

yes it's very odd; I can't reproduce locally but haven't had time to debug properly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Non 4894 e yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jmleroux @ciaranmcnulty @shouze
0