Skip to content
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

[Debug][ErrorHandler] Do not use the php80 polyfill #42223

Merged
merged 1 commit into from
Jul 22, 2021

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? 4.4
Bug fix? no
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

@carsonbot carsonbot added this to the 4.4 milestone Jul 21, 2021
@nicolas-grekas nicolas-grekas changed the title [ErrorHandler] Do not use the php80 polyfill in DebugClassLoader [ErrorHandler] Do not use the php80 polyfill Jul 21, 2021
@nicolas-grekas nicolas-grekas changed the title [ErrorHandler] Do not use the php80 polyfill [ErrorHandler][Debug] Do not use the php80 polyfill Jul 21, 2021
@stof
Copy link
Member

stof commented Jul 21, 2021

the usage of get_debug_type in FatalThrowableError also seems to cause issues (see the Goutte issue referencing that ticket)

@derrabus
Copy link
Member

Out of curiosity: What's the problem with using the polyfill here?

@carsonbot carsonbot changed the title [ErrorHandler][Debug] Do not use the php80 polyfill [ErrorHandler] Do not use the php80 polyfill Jul 21, 2021
@stof
Copy link
Member

stof commented Jul 21, 2021

@derrabus polyfills rely on autoloading (for the internal class). So this breaks if the polyfill is called inside the autoloading stack (as autoloading the polyfill internal classes will also try to use them then).
For the error handling part, I suspect it is similar (for cases where an error happens during the error handling or during the autoloading of the polyfill internals)

Copy link
Member

@Tobion Tobion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this remove the polyfill-php80 dependency from composer.json then?

@Nyholm
Copy link
Member

Nyholm commented Jul 22, 2021

I dont see us using any PHP8-only code in the ErrorHandler. I agree with @Tobion

With that change, Im 👍

@nicolas-grekas
Copy link
Member Author

Polyfill now removed (we were using a few calls to get_debug_type(), now removed).

@carsonbot carsonbot changed the title [ErrorHandler] Do not use the php80 polyfill [Debug][ErrorHandler] Do not use the php80 polyfill Jul 22, 2021
@nicolas-grekas nicolas-grekas merged commit 8c84365 into symfony:4.4 Jul 22, 2021
@Tobion
Copy link
Member

Tobion commented Jul 22, 2021

I guess we could revert this in 6.0 as it will require php 8?

@stof
Copy link
Member

stof commented Jul 22, 2021

@Tobion yes (but without re-adding the polyfill package)

@nicolas-grekas nicolas-grekas deleted the eh-nopolyfill branch July 22, 2021 11:09
@nicolas-grekas
Copy link
Member Author

I did revert while merging up.

This was referenced Jul 26, 2021
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.

6 participants