Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[FrameworkBundle] Fix MicroKernelTrait for php 8#36943

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@derrabus
Copy link
Member

QA
Branch?5.1
Bug fix?yes
New feature?no
Deprecations?no
Tickets#36872
LicenseMIT
Doc PRN/A

This PR fixes several php 8 related issues withMicroKernelTrait.

  • 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 parsedTypeErrors 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-styleconfigureContainer() 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.

@derrabusderrabusforce-pushed thebugfix/microkernel-php8 branch from47b908f to5e44cb4CompareMay 24, 2020 14:37
@derrabusderrabus changed the title[HttpFoundation] Fix MicroKernelTrait for php 8[FrameworkBundle] Fix MicroKernelTrait for php 8May 24, 2020
@derrabusderrabusforce-pushed thebugfix/microkernel-php8 branch from5e44cb4 to7476a16CompareMay 24, 2020 14:58
@nicolas-grekasnicolas-grekas added this to the5.1 milestoneMay 24, 2020
Copy link
Member

@nicolas-grekasnicolas-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 reacted with thumbs up emoji
@derrabusderrabusforce-pushed thebugfix/microkernel-php8 branch from7476a16 toede0502CompareMay 24, 2020 19:56
@derrabus
Copy link
MemberAuthor

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

nicolas-grekas, tdtm, romainnorberg, mhujer, and javiereguiluz reacted with hooray emoji

@nicolas-grekas
Copy link
Member

Thank you@derrabus.

@nicolas-grekasnicolas-grekas merged commit3693875 intosymfony:5.1May 24, 2020
@nicolas-grekas
Copy link
Member

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

derrabus and javiereguiluz reacted with thumbs up emojiderrabus and xabbuh reacted with heart emoji

@derrabusderrabus deleted the bugfix/microkernel-php8 branchMay 25, 2020 05:50
@fabpotfabpot mentioned this pull requestMay 26, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

+2 more reviewers

@goetasgoetasgoetas left review comments

@dbalabkadbalabkadbalabka left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.1

Development

Successfully merging this pull request may close these issues.

5 participants

@derrabus@nicolas-grekas@goetas@dbalabka@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp