Skip to content

[Cache] Deprecate DoctrineProvider #40908

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
Jul 4, 2021
Merged

[Cache] Deprecate DoctrineProvider #40908

merged 1 commit into from
Jul 4, 2021

Conversation

derrabus
Copy link
Member

Q A
Branch? 5.4
Bug fix? no
New feature? no
Deprecations? yes
Tickets N/A
License MIT
Doc PR TODO

The DoctrineProvider class has been reimplemented in the doctrine/cache package, so we don't have to maintain it anymore.

@derrabus derrabus requested a review from nicolas-grekas April 22, 2021 10:14
@derrabus derrabus added this to the 5.x milestone Apr 22, 2021
@derrabus
Copy link
Member Author

cc @alcaeus

@derrabus derrabus force-pushed the improvement/deprecate-doctrine-cache-provider branch 3 times, most recently from 8b209b9 to b912136 Compare April 22, 2021 10:58
alcaeus
alcaeus approved these changes Apr 22, 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.

LGTM, but we can allow cache 2.0, which will be released next week.

src/Symfony/Bundle/FrameworkBundle/composer.json Outdated
@@ -34,7 +34,7 @@
},
"require-dev": {
"doctrine/annotations": "^1.10.4",
"doctrine/cache": "~1.0",
"doctrine/cache": "^1.11",
Copy link
Contributor

Choose a reason for hiding this comment

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

This can also allow 2.0 as long as you're not using an actual implementation:

Suggested change
"doctrine/cache": "^1.11",
"doctrine/cache": "^1.11|^2.0",

src/Symfony/Component/Validator/composer.json Outdated
@@ -41,10 +41,11 @@
"symfony/property-info": "^5.3",
"symfony/translation": "^4.4|^5.0",
"doctrine/annotations": "^1.10.4",
"doctrine/cache": "~1.0",
"doctrine/cache": "^1.11",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above with 2.0

Copy link
Member Author

Choose a reason for hiding this comment

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

This needs more changes in the Validator component. Under certain circumstances, we still use an array cache from Doctrine. I'll do this in a follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, thank you!

@derrabus derrabus force-pushed the improvement/deprecate-doctrine-cache-provider branch from b912136 to c4c35d0 Compare April 22, 2021 12:46
@derrabus derrabus force-pushed the improvement/deprecate-doctrine-cache-provider branch from c4c35d0 to 5fb1867 Compare April 22, 2021 12:51
nicolas-grekas
nicolas-grekas approved these changes Apr 22, 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.

I'm also 👍 to add ^2.0, either in this PR or in a follow up one.

Nyholm
Nyholm reviewed Apr 22, 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.

Would it be simpler if we also updated doctrine/annotations at the same time?

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AnnotationsCacheWarmer.php Outdated
@@ -59,7 +59,7 @@ protected function doWarmUp(string $cacheDir, ArrayAdapter $arrayAdapter)
// doctrine/annotations:1.13 and above
$reader = new PsrCachedReader($this->annotationReader, $arrayAdapter, $this->debug);
} else {
$reader = new CachedReader($this->annotationReader, new DoctrineProvider($arrayAdapter), $this->debug);
$reader = new CachedReader($this->annotationReader, DoctrineProvider::wrap($arrayAdapter), $this->debug);
Copy link
Member

Choose a reason for hiding this comment

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

This line can be removed, right? It is dead code if we also update to doctrine/annotations:1.13.

Copy link
Member Author

Choose a reason for hiding this comment

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

As soon as we have a stable release of doctrine/annotations 1.13, yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

doctrine/annotations 1.13 is not released yet and is not sure to be released before the next 5.x release.

Copy link
Member

Choose a reason for hiding this comment

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

oops sorry. I read the wrong version on packagist. I though it was released already.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries. I also misread and it looks like this is scheduled for 5.4 anyways, so we can definitely make that change.

@alcaeus alcaeus mentioned this pull request Apr 23, 2021
Merged
@derrabus derrabus mentioned this pull request Apr 30, 2021
Merged
@derrabus derrabus force-pushed the improvement/deprecate-doctrine-cache-provider branch from 5fb1867 to 262ac66 Compare May 14, 2021 22:41
@derrabus
Copy link
Member Author

I'll update the PR as soon as #41230 is merged and has bubbled up to 5.x.

@derrabus derrabus force-pushed the improvement/deprecate-doctrine-cache-provider branch from 262ac66 to 0d0b932 Compare May 17, 2021 20:17
@fabpot fabpot force-pushed the improvement/deprecate-doctrine-cache-provider branch from 0d0b932 to 16fccfa Compare July 4, 2021 12:17
@fabpot
Copy link
Member

fabpot commented Jul 4, 2021

Thank you @derrabus.

@fabpot fabpot mentioned this pull request Jul 4, 2021
Closed
@fabpot fabpot merged commit 5010ebd into symfony:5.4 Jul 4, 2021
@derrabus derrabus mentioned this pull request Jul 4, 2021
Merged
chalasr added a commit that referenced this pull request Jul 4, 2021
@chalasr
feature #41976 [Cache] Remove DoctrineProvider (derrabus)
e42b5dc 
This PR was merged into the 6.0 branch.

Discussion
----------

[Cache] Remove DoctrineProvider

| Q             | A
| ------------- | ---
| Branch?       | 6.0
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | #40908
| License       | MIT
| Doc PR        | N/A

Commits
-------

b71101f [Cache] Remove DoctrineProvider
@derrabus derrabus deleted the improvement/deprecate-doctrine-cache-provider branch July 26, 2021 18:40
This was referenced Nov 5, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alcaeus alcaeus alcaeus approved these changes

@Nyholm Nyholm Nyholm left review comments

@fabpot fabpot fabpot approved these changes

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

@maxhelias maxhelias maxhelias approved these changes

Assignees
No one assigned
Labels
Cache Deprecation Status: Reviewed
Projects
None yet
Milestone
5.4
Development

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