Support setting PSR-6 caches with ORM 2.9+ #1332
Conversation
| $metadataCacheSetter = 'setMetadataCacheImpl'; | ||
|
|
||
| $metadataCacheType = $entityManager['metadata_cache']['type'] ?? 'pool'; | ||
| if (method_exists(OrmConfiguration::class, 'setMetadataCache') && $metadataCacheType !== 'service') { |
There was a problem hiding this comment.
@nicolas-grekas I believe this check is kosher. While users could theoretically change the class being used, ORM requires the configuration class to be of the instance we're checking here. Please let me know if this is not safe to do this way. Thanks!
alcaeus
force-pushed
the
support-doctrine-cache-2
branch
from
b94488f to
3d804f3
Compare
| $metadataCacheSetter = 'setMetadataCacheImpl'; | ||
|
|
||
| $metadataCacheType = $entityManager['metadata_cache']['type'] ?? 'pool'; | ||
| if (method_exists(OrmConfiguration::class, 'setMetadataCache') && $metadataCacheType !== 'service') { |
There was a problem hiding this comment.
@alcaeus we might need to add a need configuration setting to support telling DoctrineBundle that the service is a PSR-6 cache pool rather than a doctrine/cache implementation
There was a problem hiding this comment.
I was about to add this, then found out that #1260 deprecated setting a metadata cache entirely. @ostrolucky @dmaicher @ossinkine can you elaborate on this? From what I can tell, the PhpArrayCache adapter wraps the cache that was configured initially, which relies on setting a cache with the now deprecated key. I'm not sure I understand the whole idea here, so some clarification before adding new functionality would be nice.
There was a problem hiding this comment.
@alcaeus so from what I can tell the config for metadata_cache_driver was deprecated as it does not change anything for no-debug cases when the PhpArrayCache is used.
In theory there cannot be any cache misses as the cache is properly warmed up fully for all metadata with a Symfony CacheWarmer. So the decorated adapter would never be used/accessed. This means even an in-memory cache (ArrayCache/Adpater) will do fine as a fallback.
See #1196 (comment) and the comment afterwards.
On my apps I removed the metadata_cache_driver config for prod completely and I don't have any issues. Things are actually a tiny bit faster now with the PhpArrayCache.
There was a problem hiding this comment.
Sounds good. PSR-6 caches injected via service caches are now detected automatically and handled accordingly. The PhpArrayCache is used as it previously was. Deprecating the key sounds fine to me.
|
@ostrolucky @dmaicher this is ready for another review. |
|
@ostrolucky can to elaborate what you mean with "BC layer is on too much places" to make the feedback actionable? The BC layer is where it needs to be, there's only so much we can do to cover this. |
|
I know, that's why I proposed to bump requirements so we don't need such exhaustive bc layer. Layer would be much smaller if we drop support for those old versions. |
|
doctrine/cache can't be constrained to 2.0 only, as that makes no sense (why forbid someone from using an implementation when the API is compatible) and would most likely prevent everyone from installing this. As for bumping ORM dependency, this would delay this release since I can't tell when that will be released. This also includes figuring out what to do with doctrine/orm#8672, as ORM 2.9 in its current form is not compatible with doctrine/cache 2.0 at all (since it needs a cache implementation for the setup tool). However, other Doctrine libraries will be released soon, so it's in our interest to not needlessly block other releases. In this case, the compatibility layer is also quite common in Symfony to allow for packages to be updated more independently of each other. |
|
Ok well we don't actually need to bump doctrine/cache, since 1.11 is compatible with the changes, only ORM. As for ORM 2.9 release, doctrine-bundle 2.3 branch is compatible with it already so for doctrine-bundle 2.4, people can update ORM to 2.9 first and afterwards doctrine-bundle to 2.4, that's still within symfony goals to not having to update multiple packages at once. You are right such change might delay release of doctrine-bundle 2.4 as we would have to wait for ORM 2.9 to be released first. But we are in control of that, aren't we? I think @beberlei will manage the next release and I think he is interested to ship it out of the door soon. WDYT @beberlei? |
There was a problem hiding this comment.
Went through the diff to see what we could save if we dropped ORM < 2.9. While it doesn't really reduce the amount of logic in the extension itself, the fact that the tests are no longer conditional makes this change worth it IMO, regardless of the release timeline for ORM 2.9.
alcaeus
force-pushed
the
support-doctrine-cache-2
branch
from
b2b1455 to
da98fbc
Compare
alcaeus
force-pushed
the
support-doctrine-cache-2
branch
from
da98fbc to
7922ef0
Compare
alcaeus
force-pushed
the
support-doctrine-cache-2
branch
from
92b7f1d to
88a474e
Compare
|
@ostrolucky dropped ORM < 2.8, please let me know what else we might need. |
This adds support for doctrine/cache 2.0 as well as the new
setMetadataCachemethod being introduced to ORM in doctrine/orm#8651. This removes the need to wrap the configured cache pool in a doctrine/cache wrapper and injects this pool directly. If a service is configured as metadata cache, or ORM < 2.9 is installed, this will continue to use the then deprecatedsetMetadataCacheImplto inject the cache. It will however wrap the cache pool in the compatibility layer provided by doctrine/cache to remove the need for Symfony to maintain their own adapter for doctrine/cache (see symfony/symfony#40908).