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.

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

[Serializer] Remove BC layer #50736

Merged
merged 1 commit into from
Jun 30, 2023
Merged

[Serializer] Remove BC layer #50736

merged 1 commit into from
Jun 30, 2023

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Jun 21, 2023

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

@lyrixx lyrixx requested a review from dunglas as a code owner June 21, 2023 20:13
@carsonbot carsonbot added this to the 7.0 milestone Jun 21, 2023
wouterj
wouterj requested changes Jun 21, 2023
View reviewed changes
Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

Blocking as the necessary CHANGELOG and UPGRADE guide changes are missing ;)

@lyrixx
Copy link
Member Author

lyrixx commented Jun 21, 2023

Blocking as the necessary CHANGELOG and UPGRADE guide changes are missing ;)

Yeah 👍🏼 But I don't remember what are the new guidelines for 7.0. Is there a summary of what I must do for each files?

@tucksaun
Copy link
Contributor

It was on my todo list for the summer, thank you for taking care of it 😌

@GromNaN
Copy link
Member

GromNaN commented Jun 23, 2023

Is there a summary of what I must do for each files?

UPGRADE-7.0.md needs to be updated on the branch 7.0 for each deprecated feature that is removed. As you tried before.

@nicolas-grekas
Copy link
Member

See fabbot also (except the false-positive on the switch)

@lyrixx lyrixx mentioned this pull request Jun 27, 2023
Closed
nicolas-grekas
nicolas-grekas reviewed Jun 27, 2023
View reviewed changes
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.

Please add lines to the changelog of the component, same to the root UPGRADE-7.0.md file, and possibly ake the title of this PR more useful for a changelog

@lyrixx lyrixx force-pushed the serializer-7 branch 5 times, most recently from 525597e to bec40df Compare June 28, 2023 21:48
@lyrixx lyrixx requested a review from yceruto as a code owner June 28, 2023 21:48
@lyrixx
Copy link
Member Author

lyrixx commented Jun 28, 2023

I don't know how to fix theses tests in https://github.com/symfony/symfony/actions/runs/5406243227/jobs/9822895029?pr=50736#step:8:9587

1) Symfony\Component\Messenger\Tests\Stamp\ErrorDetailsStampTest::testDeserialization
Error: Interface "Symfony\Component\Serializer\Normalizer\ContextAwareNormalizerInterface" not found

/home/runner/work/symfony/symfony/src/Symfony/Component/Messenger/Transport/Serialization/Normalizer/FlattenExceptionNormalizer.php:25
/home/runner/work/symfony/symfony/src/Symfony/Component/Messenger/vendor/symfony/error-handler/DebugClassLoader.php:296
/home/runner/work/symfony/symfony/src/Symfony/Component/Messenger/Tests/Stamp/ErrorDetailsStampTest.php:62

@nicolas-grekas
Copy link
Member

fix theses tests

Can we stop using ContextAwareNormalizerInterface in Messenger on branch 6.4? If not, we'll have to declare on branch 6.4 that Messenger 6.4 doesn't work with serializer ^7

@lyrixx
Copy link
Member Author

lyrixx commented Jun 29, 2023 •
edited

I checked, and We don't allow 7 in lower branches
https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/ErrorHandler/composer.json#L25

Can we stop using ContextAwareNormalizerInterface in Messenger on branch 6.4?

I can check

@derrabus
Copy link
Member

Can we stop using ContextAwareNormalizerInterface in Messenger on branch 6.4?

We probably can by dropping support for Serializer 5 which would be acceptable imho.

@lyrixx lyrixx mentioned this pull request Jun 29, 2023
Merged
@lyrixx
Copy link
Member Author

lyrixx commented Jun 29, 2023

Can we stop using ContextAwareNormalizerInterface in Messenger on branch 6.4

Done ➡ #50824

nicolas-grekas added a commit that referenced this pull request Jun 30, 2023
@nicolas-grekas
minor #50824 [ErrorHandler] Do not use ContextAwareNormalizerInterfac…
660b35a 
…e anymore (lyrixx)

This PR was merged into the 6.4 branch.

Discussion
----------

[ErrorHandler] Do not use ContextAwareNormalizerInterface anymore

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

It's a deprecated feature + #50736

Commits
-------

133171b [ErrorHandler] Do not use ContextAwareNormalizerInterface anymore
nicolas-grekas
nicolas-grekas reviewed Jun 30, 2023
View reviewed changes
UPGRADE-7.0.md Outdated Show resolved Hide resolved
nicolas-grekas
nicolas-grekas reviewed Jun 30, 2023
View reviewed changes
UPGRADE-7.0.md Outdated Show resolved Hide resolved
@nicolas-grekas
Copy link
Member

Thank you @lyrixx.

@nicolas-grekas nicolas-grekas merged commit 0132503 into symfony:7.0 Jun 30, 2023
2 of 9 checks passed
@nicolas-grekas nicolas-grekas mentioned this pull request Jun 30, 2023
Closed
@lyrixx lyrixx deleted the serializer-7 branch June 30, 2023 10:21
@fabpot fabpot mentioned this pull request Oct 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wouterj wouterj wouterj requested changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

@yceruto yceruto Awaiting requested review from yceruto

Assignees
No one assigned
Labels
Serializer Status: Reviewed
Projects
None yet
Milestone
7.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@lyrixx @tucksaun @GromNaN @nicolas-grekas @derrabus @wouterj @carsonbot