Introduce PSR-6 for metadata caching

[alcaeus]

Closes #8650.

This bumps the doctrine/cache dependency to the just-released 1.11 version that includes the PSR-6 adapters needed for compatibility code. The dependency to doctrine/persistence does not to be bumped; instead we check if doctrine/persistence supports PSR-6 caching. I can update this if requested.

[greg0ire]

The dependency to doctrine/persistence does not to be bumped; instead we check if doctrine/persistence supports PSR-6 caching. I can update this if requested.

Bumping to 2.2.0 does not look like a big deal dependency-wise (the only added dependency is psr/cache). Let's release 2.2.0 and bump the dependency?

[alcaeus]

I wanted to release after 2.9 and the bundle releases to avoid unnecessary deprecation warnings users can't avoid

[alcaeus]

@greg0ire any guidance on those failures? I don't see how changing the metadata cache could cause these failures 🤔

[greg0ire]

Another issue I do not reproduce locally 😩 … caching issues are the worst!

[greg0ire]

Let's check if it is transient, but there are 2 more recent PRs that did not have this failure, so this time I think it really does come from the code in the PR.

[greg0ire]

I don't understand, but I see that class metadata is involved at some point:

orm/lib/Doctrine/ORM/UnitOfWork.php

Lines 2181 to 2199 in 9a48450

	/**
	* Executes a refresh operation on an entity.
	*
	* @param object $entity The entity to refresh.
	* @psalm-param array<string, object> $visited The already visited entities during cascades.
	*
	* @throws ORMInvalidArgumentException If the entity is not MANAGED.
	*/
	private function doRefresh(object $entity, array &$visited): void
	{
	$oid = spl_object_hash($entity);
	if (isset($visited[$oid])) {
	return; // Prevent infinite recursion
	}
	$visited[$oid] = $entity; // mark visited
	$class = $this->em->getClassMetadata(get_class($entity));

[greg0ire]

Ah I think it's starting to make sense:

orm/lib/Doctrine/ORM/UnitOfWork.php

Lines 3153 to 3158 in 9a48450

	if ($this->hasCache && $class->cache !== null) {
	$persister = $this->em->getConfiguration()
	->getSecondLevelCacheConfiguration()
	->getCacheFactory()
	->buildCachedEntityPersister($this->em, $persister, $class);
	}

I can reproduce the issue locally with ENABLE_SECOND_LEVEL_CACHE=1 vendor/bin/phpunit tests/Doctrine/Tests/ORM/Functional/IdentityMapTest.php, and you probably can too 🙂

[alcaeus]

Thanks for investigating. I'll debug and see what breaks this. Thanks!

[Nyholm]

Thank you.

I reviewed the implementation. Im happy with it. I did not dig into the tests.

[alcaeus]

@greg0ire you're right, I can reproduce the failure locally, but I can also reliably reproduce this in 2.9.x. This looks very much unrelated to this patch.

[greg0ire]

Indeed, me too!

UPDATE: I don't reproduce it when running the same phpunit command as in the CI though: ENABLE_SECOND_LEVEL_CACHE=1 vendor/bin/phpunit -c ci/github/phpunit/sqlite.xml --exclude-group performance,non-cacheable,locking_functional --coverage-clover=coverage-cache.xml

Notice the --exclude-group non-cacheable part, and how a lot of tests fail when dropping that group. Those were introduced along with the second level cache in #808

[greg0ire]

Screenshot of the relevant replies since they are buried deep inside the 200 or so collapsed comments ☝️

Both links are 404s of course:
https://github.com/FabioBatSilva/doctrine2/blob/slc/docs/en/reference/second-level-cache.rst#composite-primary-key
https://github.com/FabioBatSilva/doctrine2/blob/slc/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC117Test.php#L479

But I think the gist of it is: if a test is about counting things, then we can put it in that group?

Note that when running ENABLE_SECOND_LEVEL_CACHE=1 vendor/bin/phpunit --exclude-group performance,locking_functional,non-cacheable, tests pass but the test that fails with the code in this PR still gets executed. It also is executed and passes if using the same dependencies as with your branch.

So some other test before that one makes it somehow pass on 2.9, and no longer does on your branch.

[greg0ire]

I bisected it down to tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php. Removing that test from the suite makes the test fail, so it's the only one that does what's necessary.

Reducing the test to just the following is enough to make IdentityMapTest::testCollectionValuedAssociationIdentityMapBehaviorWithRefresh pass on 2.9.x:

<?php

declare(strict_types=1);

namespace Doctrine\Tests\ORM\Functional;

use Doctrine\ORM\Mapping\ClassMetadataInfo;
use Doctrine\Tests\Models\CMS\CmsArticle;
use Doctrine\Tests\Models\CMS\CmsGroup;
use Doctrine\Tests\Models\CMS\CmsPhonenumber;
use Doctrine\Tests\Models\CMS\CmsUser;
use Doctrine\Tests\Models\DDC2504\DDC2504ChildClass;
use Doctrine\Tests\Models\DDC2504\DDC2504OtherClass;
use Doctrine\Tests\OrmFunctionalTestCase;

use function array_shift;
use function assert;
use function count;

/**
 * Description of ExtraLazyCollectionTest
 */
class ExtraLazyCollectionTest extends OrmFunctionalTestCase
{
    protected function setUp(): void
    {
        parent::setUp();

        $class = $this->_em->getClassMetadata(CmsUser::class);

        // drop that line and IdentityMapTest::testCollectionValuedAssociationIdentityMapBehaviorWithRefresh fails
        unset($class->associationMappings['phonenumbers']['cache']); 

    }

    public function testContainsKeyNonExistentIndexByManyToMany(): void
    {
    }
}

Both tests deal with phonenumbers, and the line that makes the subsequent test pass has to do with metadata cache, so it's starting to make some sense.

[greg0ire]

Blocked by #8659 (and a merge up)

[alcaeus]

@greg0ire @beberlei codecov failures are due to not testing with a development version of doctrine/persistence. That can only be released after an ORM release containing this patch to avoid unnecessary deprecation warnings.

[greg0ire]

That can only be released after an ORM release containing this patch to avoid unnecessary deprecation warnings.

Or maybe revert those, add them back later?

[greg0ire]

alcaeus requested a review from greg0ire 20 minutes ago

Let's talk about the aforementioned issue first? I think the deprecations introduced in doctrine/persistence#144 should be reverted if it is what we are talking about, until all calling projects have adapted. Then, in a subsequent PR, we can reintroduce them, and we might want to use doctrine/deprecations this time, might we not?

[alcaeus]

Summary of discussion with @greg0ire:

• Deprecation will be done in doctrine/persistence 2.2. We're waiting on an unrelated bug fix, so the release is a few days out. Despite that, the deprecation is not in the hot path, so there's no requirement to release ORM before persistence
• The bundle will be released once its work has been completed. This ensures that the bundle itself configures a PSR-6 cache if the methods are available

With that in mind, I think this PR is ready for review.

[greg0ire]

Please summarize the deprecations in UPGRADE.md

[beberlei]

I think i can do the deprecation summarization as part of the general 2.9 work, as this is not done for a few deprecations yet.

[greg0ire]

In that case, let's go!

[greg0ire]

Thanks @alcaeus !