Closed driehle closed 1 year ago
@fezfez @snapshotpl @Ocramius
I'd be happy if you could provide some feedback to this proposed PR! :-)
@Ocramius
[...] but this feels wrong for a minor release: a new major is more appropriate, given that this can drag in many unexpected BC breaks, which seems to be highlighted by many test changes
I can understand that this feels a bit wrong, but there are no signature changes related to DoctrineModule. The change is the major upgrade from doctrine/cache 1 to 2. If we'd go for a major release for each of a dependency's new major, we'd be releaseing new majors each week which - from my perspective - wouldn't be helpful either.
Since simply sticking with "doctrine/cache": "^1.0"
won't cause any change for the user, I'd still like to release this as a minor. This will allow people to choose when they are willing to switch to laminas-cache. When we release the next major, i.e. DoctrineModule 6, people will be forced to move to laminas-cache, because doctrine/cache will then be fully removed.
there are no signature changes related to DoctrineModule.
The breakages are mostly in the types of services produced: such is the nature of integration work when building integration/glue packages :-\
If we'd go for a major release for each of a dependency's new major, we'd be releaseing new majors each week which - from my perspective - wouldn't be helpful either.
Inherited BC breaks are still BC breaks.
I don't have a full overview of breakages here, but what I want to avoid here is that a composer update
with a minor upgrade of only this package in composer.json
leads to the consumer receiving a completely incompatible cache instance:
^1
support, it is even feasible to drop ^1
support in a subsequent minor release tooThe breakages are mostly in the types of services produced: such is the nature of integration work when building integration/glue packages :-\
Yes, but there are two cases to be considered here:
Case 1: The user has doctrine/cache ^1 required. If that's the case, then there are no changes in the services produced, hence, no breaking changes.
Case 2: The user does not have doctrine/cache required at all (which IMHO is their fault). Then, this change will upgrade from doctrine/cache ^1 to ^2. This bears two risks:
Doctrine\Common\Cache\Cache
. That promise won't be broken, but instead of Doctrine\Common\Cache\ArrayCache
it will return an DoctrineModule\Cache\LaminasStorageCache
(which wraps Laminas\Cache\Storage\Adapter\Memory
). This may result in breaks if the user for some reason explicitly checks for instanceof ArrayCache::class
. doctrine.cache.apc
, doctrine.cache.memcache
(note: memcache, not memcacheD!), doctrine.cache.predis
, doctrine.cache.windata
, doctrine.cache.xcache
and doctrine.cache.zenddata
aren't available anymore.
apc
has been replaced by apcu
sind PHP 7.0memcache
has been replaced by memcached
sind PHP 7.0wincache
is not available for PHP 7xcache
is not available for PHP 7To me, these risks seem to be considerably low, given that they can be mitigated by simply added doctrine/cache: ^1.0
to the project's composer.json. The caches that are not available anymore with doctrine/cache ^2 cannot be used with recent PHP versions anyways, except for Predis. We can add a wrapper/decorator for Predis, to circumvent that.
if the returned types are still compatible after removal of
^1
support, it is even feasible to drop^1
support in a subsequent minor release too
Yes, that'd be the ultimate goal. :-)
I agree with @Ocramius I think it will break a lot of applications (including mine π΅βπ«) and it should be reserved for a new major release
@fezfez
I agree with Ocramius I think it will break a lot of applications (including mine π΅βπ«) and it should be reserved for a new major release
Do you see a break with doctrine/cache ^1.0, or do you not want to add that constraint to your application? π€
As @driehle explains in the PR description, as long as a user defines doctrine/cache ^1.0
, there is no break at all.
I suggest adding a Warning on the readme so, if a user does not define it and finds a break, the solution can be easily found.
Additionally, doctrine/cache ^2.0
could be added as a suggestion on the composer.json file.
Since this PR needs further discussion, I propose to release a 5.3.0
version with PHP 8.2 Support and later decide if this PR should be released as 5.4.0
or 6.0
.
What do you think @driehle @Ocramius @fezfez?
BTW, I don't have anything against this patch: just saying that it carries substantial risk baggage, and in the interest of downstream consumers, it would be best to mark it as new major, to prevent confusion and make the risk visible to downstream.
Can then bump again afterwards π
Perfect.
Then, @driehle, could you please release 5.3.0 with PHP 8.2 Support?
Let me know if there is something else I can do from my side.
5.3.0 is now released, still I think this PR should go on a minor. Targeting at 5.4.0. If people really experience breaks with this change, I'd be happy to hear about them in more detail.
@driehle your call on that: besides the risk of BC, this patch is OK :)
Hello, everyone! I'm trying to get this project compatible with annotations v2 (see #801) and I've run into a problem with caching. The Doctrine\Common\Annotations\CachedReader
is removed in v2, replaced by a PsrCachedReader
. The PsrCachedReader
expects an instance of Psr\Cache\CacheItemPoolInterface
, but DoctrineModule\Cache\LaminasStorageCache
does not implement that interface (and indeed, cannot easily implement it, due to a conflicting definition of the save
method). I'm wondering if this is a problem worth trying to sort out as part of the work here, so we can get ahead of further compatibility problems down the road.
Any thoughts? And I apologize if I've gotten any details wrong -- I'm a newbie to Doctrine, but looking to migrate from Laminas\Db to Doctrine in an application I work on, and I've fallen down this update-related rabbit hole. :-)
@demiankatz Using the PSR standard is a step forward for this module. May I strongly recommend you don't use annotations at all, ever, and instead use XML metadata? That may sound daunting but if you'll investigate https://skipper18.com you'll find a tool that can convert from annotations to ERD to XML.
@TomHAnderson, thanks for the advice; I will look into the XML metadata option! But I'm not trying to update annotations here because I'm determined to use them, but simply because I'm trying to resolve a dependency conflict between DoctrineModule and php-cs-fixer that's preventing my composer install
from working. :-)
This module uses phpcs by squizlabs. It is odd your php-cs-fixer is conflicting though. Not to discount @fabpot work but a switch may be in order?
@TomHAnderson, my project uses both php-cs-fixer and phpcs; I find the tools to be complementary. In any case, php-cs-fixer has a dependency on doctrine/annotations for one of its advanced features, and it raised that to require version 2, which is incompatible with the requirements of this module, which thus creates a dependency headache for any project that tries to use both things. :-)
@demiankatz Thanks for the explanation. That clears it up for me. I see two paths for you:
Thanks, @TomHAnderson. As a stopgap measure, I'm just deleting the php-cs-fixer dependency from the branch of my repository that incorporates Doctrine... and I can add it back in when we get everything fixed. I think the most important point is that Doctrine is moving forward in various ways and we need to make sure this module can catch up with it.
I already have a PR in progress to update the annotations dependency (#801), but I believe it is contingent on getting this PR sorted out, and also adding PSR support. So that's the knot we need to untangle at this point! I'm not entirely sure where this PR stands, and I'm also not sure how much work it would take to add PSR cache support -- e.g. whether that's an additional step that should be done here, or a subsequent PR after this one is merged. As a total newcomer to this project (and with limited time, since I already have full-time responsibilities to other projects), I'm probably not the best person to figure it all out, but I'm certainly happy to contribute as much as time will permit. :-)
The library
doctrine/cache
is deprecated and no longer actively developed, see #786 and doctrine/cache. Hence, we should replace the caches provided by this module with implementation oflaminas/laminas-cache
, see docs.This PR enables the installation of
doctrine/cache: ^2.0
, which is just a set of interfaces and no actual implementations anymore. If users havedoctrine/cache: ^1.0
installed, there will be no changes in behaviour. Hence, I do not consider this PR as a BC break!If users opt it to install
doctrine/cache: ^2.0
, the following changes will occur on the caches provided:doctrine.cache.apc
Doctrine\Common\Cache\ApcCache
doctrine.cache.apcu
Doctrine\Common\Cache\ApcuCache
Laminas\Cache\Storage\Adapter\Apcu
(wrapped)doctrine.cache.array
Doctrine\Common\Cache\ArrayCache
Laminas\Cache\Storage\Adapter\Memory
(wrapped)doctrine.cache.filesystem
Doctrine\Common\Cache\FilesystemCache
Laminas\Cache\Storage\Adapter\Filesystem
(wrapped)doctrine.cache.memcache
Doctrine\Common\Cache\MemcacheCache
doctrine.cache.memcached
Doctrine\Common\Cache\MemcachedCache
Laminas\Cache\Storage\Adapter\Memcached
(wrapped)doctrine.cache.predis
Doctrine\Common\Cache\PredisCache
doctrine.cache.redis
Doctrine\Common\Cache\RedisCache
Laminas\Cache\Storage\Adapter\Redis
(wrapped)doctrine.cache.wincache
Doctrine\Common\Cache\WinCacheCache
doctrine.cache.xcache
Doctrine\Common\Cache\XcacheCache
doctrine.cache.zenddata
Doctrine\Common\Cache\ZendDataCache
Caches not supported by laminas-cache have been dropped without replacement. Caches, where a Laminas implementation is available, are instantiated and wrapped in an instance of
DoctrineModule\Cache\LaminasStorageCache
. This effectively decorates the laminas cache instances as doctrine cache and ensures backward compatibility.Users who want to work with the laminas instances directly, i.e. not the wrapped object, can access these objects as follows:
doctrinemodule.cache.apcu
doctrinemodule.cache.array
doctrinemodule.cache.filesystem
doctrinemodule.cache.memcached
doctrinemodule.cache.redis
In the release notes of 5.3.0 we will have to state that users who want to continue using
doctrine/cache
must ensure they havedoctrine/cache: ^1.0
in their composer.json. This should, however, be best practice anyways, if they are using it in their application. Again, this is another argument why I do not consider this PR as a BC break.Moreover, the documentation needs to be updated. I will provide this as a separate PR, once this one is merged.