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

Allow installing doctrine/cache 2.0 #8672

Merged
merged 11 commits into from
May 14, 2021
Merged

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented May 5, 2021

This PR is a draft for changes that would have to be made in ORM to allow installing doctrine/cache 2.0. The initial commit updating composer.json would have to be reworked - the @dev constraints are necessary until we tag releases allowing doctrine/cache 2.0 in those packages.

The changes mostly affect the test, but as is, this PR breaks applications that are relying on the setup tool. Without cache implementations, we won't be able to restore functionality, so we would either have to block doctrine/cache 2.0 until this has changed, or ship a different cache implementation which isn't really what we want.

I'm not sure what the best course of action is here. The Setup tool isn't necessary to run the ORM, so I'm a bit bummed that it's the only thing blocking us from allowing doctrine/cache 2.0. On the other hand, even if we were to simply not set a cache if doctrine/cache 2.0 is installed, this could cause severe issues for people relying on caches to be set automatically. @beberlei would you have any guidance on this specifically?

@alcaeus alcaeus requested review from beberlei and greg0ire May 5, 2021 07:51
@alcaeus alcaeus self-assigned this May 5, 2021
@alcaeus alcaeus force-pushed the allow-cache-2.0 branch 2 times, most recently from 790bf24 to 439d752 Compare May 13, 2021 08:51
@alcaeus
Copy link
Member Author

alcaeus commented May 13, 2021

After discussion with @beberlei, this PR now contains a minor BC break in the setup tool when doctrine/cache 2.0 is installed. In those cases, the setup tool will need to be provided with a cache or have the symfony/cache package installed. Other PSR-6 libraries can be supported as well; people who have an interest in having their cache of choice supported should create a PR adding it.

I've also improved the BC layer for getMetadataCacheImpl to return a wrapped PSR-6 cache when one was configured. If a doctrine/cache was configured, the method always returns that cache instead.

@alcaeus alcaeus marked this pull request as ready for review May 13, 2021 09:36
Copy link
Member

@beberlei beberlei left a comment

Choose a reason for hiding this comment

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

Looks good, just one question about composer.json changes, I assume they need to be reverted to loose the @dev?

composer.json Outdated
@@ -19,17 +19,17 @@
"php": "^7.1|^8.0",
"ext-pdo": "*",
"composer/package-versions-deprecated": "^1.8",
"doctrine/annotations": "^1.12",
"doctrine/cache": "^1.11.0",
"doctrine/annotations": "^1.12@dev",
Copy link
Member

Choose a reason for hiding this comment

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

These need to be reverted to their stable counter parts, I assume they were only present for testing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. If everything else looks good, I'd remove the dev portion to ensure I didn't break anything.

@alcaeus alcaeus force-pushed the allow-cache-2.0 branch from ad2932c to 590551d Compare May 13, 2021 18:17
@alcaeus alcaeus requested a review from beberlei May 13, 2021 18:17
@alcaeus
Copy link
Member Author

alcaeus commented May 13, 2021

Change applied, this now depends on doctrine/annotations 1.13 (which was just released) for the PsrCachedReader.

@alcaeus alcaeus added this to the 2.9.0 milestone May 14, 2021
@alcaeus alcaeus merged commit d52dab5 into doctrine:2.9.x May 14, 2021
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.

3 participants