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

Support Doctrine Cache 2 #175

Merged
merged 1 commit into from
Apr 30, 2021
Merged

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Apr 29, 2021

This library is already compatible with Doctrine Cache 2 because it only depends on the interfaces. For tests, I've added Symfony Cache because Doctrine Cache 2 does not provide any implementations.

@derrabus derrabus requested a review from alcaeus April 29, 2021 18:35
@greg0ire
Copy link
Member

🤔 any reason to target 1.4.x ? It's no longer maintained… the lowest supported branch is 2.1.x.

@derrabus derrabus force-pushed the doctrine-cache-2 branch 2 times, most recently from 8021c7c to d12e714 Compare April 29, 2021 18:46
@derrabus derrabus changed the base branch from 1.4.x to 1.3.x April 29, 2021 18:46
@derrabus
Copy link
Member Author

derrabus commented Apr 29, 2021

Persistence 1 is still downloaded a lot: https://packagist.org/packages/doctrine/persistence/stats

And since this is merely unblocking a dependency that is already compatible, I thought targeting v1 might be a good idea. But I just realized that 1.4 has never seen a release, so 1.3 would be the proper target.

But if v1 is totally closed for changes, I can target 2.1 instead.

@greg0ire
Copy link
Member

Please do, people using v1 are apparently not worried about staying up-to-date enough to care about upgrading to cache v2 IMO. Also, as you can see, the pipeline is very outdated on that branch (and broken).

@derrabus derrabus changed the base branch from 1.3.x to 2.1.x April 29, 2021 18:56
@greg0ire greg0ire added this to the 2.1.1 milestone Apr 29, 2021
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Ok for me to go in a patch release since the only non-test change is a dependency being relaxed.

$cache->save(ChildEntity::class . '$CLASSMETADATA', $metadata);

$this->cmf->setCacheDriver($cache);

self::assertSame($metadata, $this->cmf->getMetadataFor(ChildEntity::class));
self::assertEquals($metadata, $this->cmf->getMetadataFor(ChildEntity::class));
Copy link
Member Author

@derrabus derrabus Apr 29, 2021

Choose a reason for hiding this comment

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

💡 Those tests were actually broken. A cache usually serialized/deserializes the given object, so you cannot expect to receive the very same instance you've inserted.

Copy link
Member

@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'm fine relaxing the requirement in a patch release. Thanks @derrabus!

@alcaeus alcaeus merged commit 385f2e6 into doctrine:2.1.x Apr 30, 2021
@derrabus derrabus deleted the doctrine-cache-2 branch April 30, 2021 06:32
alcaeus pushed a commit that referenced this pull request May 15, 2021
alcaeus added a commit that referenced this pull request May 15, 2021
* Remove use of deprecated TestCase::at() method

For that test it does not add value to check the order of calls.

* Support Doctrine Cache 2 (#175)

* Fix Psalm and PHPStan errors for Doctrine Cache 2 (#176)

* Fix missing use statement

* Fix invalid assertion

Co-authored-by: Fran Moreno <franmomu@gmail.com>
Co-authored-by: Alexander M. Turek <me@derrabus.de>
Co-authored-by: Andreas Braun <git@alcaeus.org>
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