-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Re-enable Symfony test on PHP 8 #5620
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
Conversation
Fails with:
|
@kocsismate What do you think about always displaying the class name, even if the expected type is not a class? I think this kind of error would be easier to debug if it said |
@nikic Yeah, it's a good idea for sure. I'll implement this in the coming days. |
Thanks for updating the azure config to run community jobs.
I don't have this failure locally, did anything related to return types change recently? We're using a stringable object message. The reason is that computing this specific error message takes a lot of resources, while we might just skip the error entirely in most situations. It's been working well since always. |
@nicolas-grekas This is likely a debug-build only error. Most likely Exception::getMessage() now specifies a string return type, while maybe it shouldn't. |
@nikic I've recently added that specific return type there. When should it return an object? EDIT: I see it now. |
Yes, looks like this is the case: php-src/Zend/zend_exceptions.stub.php Line 37 in ce668c0
I'm wondering, what is the better behavior here: Return the Stringable object from |
this option would break our use case... (I'm fact-checking this claim, give me a few minutes) |
Hmm actually I'm not sure it would break our use case. For reference, here is the logic in Symfony: |
@kocsismate Generally, all the types on exception methods that return a protected property aren't right. We either need to make the necessary conversions in the getter, throw an exception, or drop the type. |
Now we get a use-after-free :( |
The script now runs each component independently. The use-after-free is in the DI component. It might be related to the very change about Most other failures are spammy The failures in the |
@nicolas-grekas The The issue in case of should rather be
|
It's definitely related, but more in the sense that the change exposed an existing bug. I can reproduce locally with an asan build, but have some trouble debugging this, because gdb itself crashes :(
This is most likely due to a different ICU version rather than something in the intl extension itself (I hope), so for now we can ignore it. For our purposes the main important part is that there are no crashes / assertion failures / fatal errors. |
The exception use-after-free should be fixed with 55dd394. From a local run I'm still getting a
though. |
I've submitted #5636 to make the private
@nicolas-grekas Would you mind changing the FatalError code to use [] instead of null as the default value of trace? |
…icolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [ErrorHandler] fix setting $trace to null in FatalError | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Spotted by @nikic in php/php-src#5620 (comment) Commits ------- aa50c92 [ErrorHandler] fix setting $trace to null in FatalError
Following symfony/symfony#36872, Symfony is now green on PHP 8.