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 #1375

Merged
merged 4 commits into from
Oct 6, 2021
Merged

Conversation

alcaeus
Copy link
Contributor

@alcaeus alcaeus commented May 15, 2021

Q A
Branch? 2.0
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets Fixes #1368
License MIT
Doc PR

This PR adds a new PsrCacheResolver class and deprecates the existing CacheResolver. With some changes to the tests, we are able to limit our exposure to doctrine/cache, which is being retired. Allowing installing doctrine/cache 2.0 signals that while the library makes use of the cache interfaces, it does not have any direct dependency on logic from doctrine/cache. At the same deprecating the existing resolver indicates to users that they should no longer rely on doctrine/cache for their needs.

@lsmith77
Copy link
Contributor

thank you .. if you rebase, PHP CS fixer should work fine again.

not sure about the failing test https://github.com/liip/LiipImagineBundle/pull/1375/checks?check_run_id=2645211384#step:10:32

@alcaeus alcaeus force-pushed the allow-doctrine-cache-2 branch from 13c99d2 to af9a806 Compare May 31, 2021 12:15
@alcaeus
Copy link
Contributor Author

alcaeus commented May 31, 2021

I have lots of failures when running the PngQuantPostProcessorTest locally in 2.x, so I can't really tell whether it's related. Looking at the test, it does not seem to involve any of the cache logic I modified (unless I missed something).

@lsmith77
Copy link
Contributor

@alcaeus ok .. I triggered the github actions.

@lsmith77
Copy link
Contributor

lsmith77 commented Jun 1, 2021

looks like a tiny CS issue and something with PHP 8.0 is still left

@alcaeus
Copy link
Contributor Author

alcaeus commented Jun 1, 2021

I fixed phpcs, but the PHP 8 issue is unrelated and likely due to incompatible phpunit versions being used across builds.

@jkabat
Copy link
Contributor

jkabat commented Jun 23, 2021

Friendly bump. Can it be merged?

@lsmith77
Copy link
Contributor

@dbu / @Tobion

Copy link
Member

@dbu dbu 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 the merge request!

sorry for the late reply. just looked at this now and i have some minor questions.

composer.json Outdated
@@ -19,6 +19,8 @@
},
"require": {
"php": "^7.1|^8.0",
"doctrine/cache": "^1.11|^2.0",
"psr/cache": "^1.0|^2.0|^3.0",
Copy link
Member

Choose a reason for hiding this comment

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

is caching no longer optional?

@alcaeus
Copy link
Contributor Author

alcaeus commented Oct 5, 2021

I honestly don't remember why they were moved. I won't have time to wrap up this pull request anytime soon, so I'd like to hand it off for finishing touches.

@mbabker
Copy link
Contributor

mbabker commented Oct 5, 2021

@alcaeus You should be able to apply my suggestions then this PR should be good to go.

@dbu
Copy link
Member

dbu commented Oct 6, 2021

thanks for the work alcaeus and mbabker. i will apply those changes and rebase on 2.x to see if all pans out.

@dbu dbu merged commit 6d93c7c into liip:2.x Oct 6, 2021
@alcaeus alcaeus deleted the allow-doctrine-cache-2 branch October 6, 2021 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CacheResolver relies on to-be-retired doctrine/cache
5 participants