Skip to content

[FrameworkBundle][Validator] Fix deprecations from Doctrine Annotations+Cache #41230

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
Merged
merged 1 commit into from
May 17, 2021
Merged

[FrameworkBundle][Validator] Fix deprecations from Doctrine Annotations+Cache #41230

merged 1 commit into from
May 17, 2021

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented May 14, 2021 •
edited
Loading

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets N/A
License MIT
Doc PR N/A
  • Doctrine Annotations' CachedReader is deprecated. Let's not use it if we don't have to.
  • Doctrine Cache 2 has been released. Since we're mostly only using the interfaces, we can indicate compatibility.

Paslm is going to complain about missing classes, which is kind-of expected here. 🙂

@derrabus derrabus requested review from dunglas and yceruto as code owners May 14, 2021 20:04
@carsonbot carsonbot added this to the 4.4 milestone May 14, 2021
@carsonbot carsonbot changed the title Fix deprecations from Doctrine Annotations+Cache [FrameworkBundle][Validator] Fix deprecations from Doctrine Annotations+Cache May 14, 2021
@derrabus derrabus force-pushed the bugfix/annotations-cache-deprecations branch from 12721f8 to aae8b2d Compare May 14, 2021 20:07
@derrabus
Copy link
Member Author

cc @alcaeus

@derrabus
Copy link
Member Author

The Travis failure is particularly interesting. In debug mode, PsrCachedReader seems to behave differently than CachedReader does. 👀

@carsonbot

This comment has been minimized.

Sign in to view
alcaeus
alcaeus approved these changes May 15, 2021
View reviewed changes
Copy link
Contributor

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

I really appreciate the fact that you managed to introduce compatibility with a new major release in a patch release with a nice downgrade to an uncached reader if everything else fails. Defensive programming to perfection! 👏

@ostrolucky
Copy link
Contributor

He didn't manage that yet, though. He mentioned he has problems that tests discovered new cache having different behaviour.

@derrabus derrabus force-pushed the bugfix/annotations-cache-deprecations branch from aae8b2d to 2c9dd17 Compare May 16, 2021 10:38
@derrabus derrabus force-pushed the bugfix/annotations-cache-deprecations branch from 2c9dd17 to 3b8dfc7 Compare May 16, 2021 11:41
@alcaeus
Copy link
Contributor

alcaeus commented May 16, 2021

He mentioned he has problems that tests discovered new cache having different behaviour.

I'll take the blame for that, since the difference in behaviour comes from doctrine/annotations providing the wrong functionality. They're supposed to have the same behaviour with different cache libraries.

@derrabus derrabus mentioned this pull request May 16, 2021
Merged
@derrabus
Copy link
Member Author

derrabus commented May 16, 2021 •
edited
Loading

The issue should be has been fixed by doctrine/annotations#412.

@derrabus derrabus force-pushed the bugfix/annotations-cache-deprecations branch 2 times, most recently from 99bc0bd to 1cfd50c Compare May 16, 2021 18:23
nicolas-grekas
nicolas-grekas approved these changes May 16, 2021
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.

(but let's release annotations 1.13.1 before merging?)

@derrabus derrabus force-pushed the bugfix/annotations-cache-deprecations branch from 1cfd50c to 4c12a26 Compare May 16, 2021 18:41
@derrabus derrabus force-pushed the bugfix/annotations-cache-deprecations branch from 4c12a26 to aa4b8bd Compare May 16, 2021 18:47
@derrabus derrabus mentioned this pull request May 16, 2021
Merged
@alcaeus
Copy link
Contributor

alcaeus commented May 16, 2021

doctrine/annotations 1.13.1 is released, so feel free to bump 👍

@derrabus
Copy link
Member Author

(but let's release annotations 1.13.1 before merging?)

1.13.1 has been tagged.

@derrabus derrabus force-pushed the bugfix/annotations-cache-deprecations branch from aa4b8bd to ec51c21 Compare May 16, 2021 21:41
Nyholm
Nyholm approved these changes May 17, 2021
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you. Just a minor question to clarify

@derrabus derrabus merged commit 373528f into symfony:4.4 May 17, 2021
@derrabus derrabus deleted the bugfix/annotations-cache-deprecations branch May 17, 2021 19:36
This was referenced May 19, 2021
Merged
Merged
Merged
@derrabus derrabus mentioned this pull request May 30, 2021
Merged
fabpot added a commit that referenced this pull request May 31, 2021
@fabpot
bug #41452 [FrameworkBundle] Remove redundant cache service (derrabus)
852cc29 
This PR was merged into the 5.3 branch.

Discussion
----------

[FrameworkBundle] Remove redundant cache service

| Q             | A
| ------------- | ---
| Branch?       | 5.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | N/A
| License       | MIT
| Doc PR        | N/A

Follows #40444 and #41230.

`@Nyholm` and I have fixed the same problem on different branches. Merging both efforts, we've created two cache services that are supposed to serve the same purpose. This PR suggests to remove one of them.

Commits
-------

3b197fe [Framework] Remove redundant cache service
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@alcaeus alcaeus alcaeus approved these changes

@stof stof stof approved these changes

@Nyholm Nyholm Nyholm approved these changes

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

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

Assignees
No one assigned
Labels
Bug FrameworkBundle Status: Reviewed Validator
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
7 participants
@derrabus @carsonbot @ostrolucky @alcaeus @nicolas-grekas @stof @Nyholm