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

[FrameworkBundle] Fix MicroKernelTrait for php 8 #36943

Merged
merged 1 commit into from
May 24, 2020

Conversation

derrabus
Copy link
Member

Q A
Branch? 5.1
Bug fix? yes
New feature? no
Deprecations? no
Tickets #36872
License MIT
Doc PR N/A

This PR fixes several php 8 related issues with MicroKernelTrait.

  • Anonymous microkernel classes were not handled properly. I've added a test case to cover this scenario and fixed the issues.
  • As part of the upgrade path, the trait logic parsed TypeErrors raised by php. That code broke because the wording of those errors has changed. I've replaced that logic with a hopefully less brittle reflection-based approach.
  • In order to fix compatibility issues, @nicolas-grekas has already commented out the two abstract methods of the trait. If someone forgets to implement them, they would've ran into rather cryptic errors. I've added tests for these scenarios as well and introduced user-friendly exceptions.

I've noticed that implementing an old-style configureContainer() does not raise any deprecation. Is that on purpose? I really think that we should deprecate the old way, so we can re-add the abstract methods again in Symfony 6.

@derrabus derrabus changed the title [HttpFoundation] Fix MicroKernelTrait for php 8 [FrameworkBundle] Fix MicroKernelTrait for php 8 May 24, 2020
@nicolas-grekas nicolas-grekas added this to the 5.1 milestone May 24, 2020
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Nice improvements thanks, just some nitpicking.

I've noticed that implementing an old-style configureContainer() does not raise any deprecation. Is that on purpose? I really think that we should deprecate the old way, so we can re-add the abstract methods again in Symfony 6.

Yes, that's on purpose: my opinion on this is that the is a nice extension point to easily allow extensibility of the DSL. We're fine is ppl want to use any variant of it (by taking inspiration of the one we added in 5.1).

The previous DSL is still fine, it's not deprecated and shouldn't be to me.

@derrabus
Copy link
Member Author

Ladies and gentlemen, attached to this PR, you can witness the first green php 8 build of the 5.1 branch. 🎉

@nicolas-grekas
Copy link
Member

Thank you @derrabus.

@nicolas-grekas nicolas-grekas merged commit 3693875 into symfony:5.1 May 24, 2020
@nicolas-grekas
Copy link
Member

And now master \o/ Thanks for your hard work @derrabus!!!

@derrabus derrabus deleted the bugfix/microkernel-php8 branch May 25, 2020 05:50
@fabpot fabpot mentioned this pull request May 26, 2020
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.

None yet

5 participants