Continue merge up

[greg0ire]

I want to check if the imerge steps also pass the build when running on Github with more jobs.

[greg0ire]

In #10832, this interface was introduced by the Proxy interface in the same namespace. Now, that interface no longer exists (since #10551). I'm not sure how to resolve that conflict.

[greg0ire]

I think the solution is to have the proxy classes also implement this interface. That's what I did in my second commit anyway.

[greg0ire]

@nicolas-grekas @derrabus can you please look into this one? 🙏

[nicolas-grekas]

Sure, ASAP!

[greg0ire]

In the first failing test, it looks like Doctrine interacts with the DBAL at some point, performing an SQL query, when it really should not: the test does not create an entity in the database, it merely creates entities in the cache, and I think what's expected is that the entity is fetched from the cache. I think we expect the closure inside ProxyFactory::createInitializer to exit after checking that the proxy is initialized (it does not):

orm/lib/Doctrine/ORM/Proxy/ProxyFactory.php

Lines 194 to 196 in 62ed63b

	if ($proxy->__isInitialized()) {
	return;
	}

The test line that causes the initialization on 3.0.x is this one:

orm/tests/Doctrine/Tests/ORM/Cache/DefaultEntityHydratorTest.php

Line 61 in 62ed63b

$entity = $this->structure->loadCacheEntry($metadata, $key, $entry, $proxy);

It calls that one:

orm/lib/Doctrine/ORM/Cache/DefaultEntityHydrator.php

Line 187 in 62ed63b

$result = $this->uow->createEntity($entry->class, $data, $hints);

This is caused by a change in a UnitOfWork code block from

from

            if ($entity instanceof Proxy && ! $entity->__isInitialized()) {
                $entity->__setInitialized(true);
            } else {
                if (
                    ! isset($hints[Query::HINT_REFRESH])
                    || (isset($hints[Query::HINT_REFRESH_ENTITY]) && $hints[Query::HINT_REFRESH_ENTITY] !== $entity)
                ) {
                    return $entity;
                }
            }

to

            if ($this->isUninitializedObject($entity)) {
                $entity->__setInitialized(true);
            } else {
                if (
                    ! isset($hints[Query::HINT_REFRESH])
                    || (isset($hints[Query::HINT_REFRESH_ENTITY]) && $hints[Query::HINT_REFRESH_ENTITY] !== $entity)
                ) {
                    return $entity;
                }
            }

On 3.0.x, the if branch is used, while on imerge/continue-merge-up, the else branch is used.

[greg0ire]

Unsurprisingly, the issue is caused by my unresolved comment: the proxy does not implement InternalProxy, which causes isUninitializedObject to return false.

[greg0ire]

@derrabus @nicolas-grekas I pushed a commit with a very ugly fix, please have a look at it and let me know whether you think I should introduce an interface (InternalCommonProxy ?) that would extend InternalProxy and the Proxy from doctrine/common.

[stof]

@greg0ire shouldn't proxy from Common not be used anymore in the 3.0.x branch ?

[greg0ire]

@stof I think so, but I try to keep merges up diffs as small as possible. The cleanup should IMO happen in a separate PR.

[stof]

@greg0ire the conflict comes from the fact that the old proxy logic is already removed from the 3.0.x (based on the version of the codebase before the refactoring you are trying to merge up). So you cannot delay the cleanup here. You need to solve the conflicts between the past cleanup and the refactoring.

[greg0ire]

the conflict comes from the fact that the old proxy logic is already removed from the 3.0.x

@stof really? I don't think I truly understand what you mean by "old proxy logic" because the proxy logic relying on doctrine/common seems very much still in use on 3.0.x:

orm/.github/workflows/continuous-integration.yml

Lines 42 to 43 in 9a1085d

	proxy:
	- "common"

orm/tests/Doctrine/Tests/TestUtil.php

Lines 94 to 112 in 9a1085d

	public static function configureProxies(Configuration $configuration): void
	{
	$configuration->setProxyDir(__DIR__ . '/Proxies');
	$configuration->setProxyNamespace('Doctrine\Tests\Proxies');
	$proxyImplementation = getenv('ORM_PROXY_IMPLEMENTATION')
	?: (trait_exists(LazyGhostTrait::class) ? 'lazy-ghost' : 'common');
	switch ($proxyImplementation) {
	case 'lazy-ghost':
	$configuration->setLazyGhostObjectEnabled(true);
	return;
	case 'common':
	$configuration->setLazyGhostObjectEnabled(false);
	return;
	}

orm/composer.json

Line 27 in 9a1085d

"doctrine/common": "^3.3",

Can you please clarify what you are referring to?

[greg0ire]

let me know whether you think I should introduce an interface (InternalCommonProxy ?) that would extend InternalProxy and the Proxy from doctrine/common.

As pointed out by @stof , this is all going to disappear soon, so let's not bother with a short-lived interface. This addresses my only concern about this, so let's mark this as ready for review.

Regarding the docs job, it should get fixed in a subsequent merge up.

[greg0ire]

@derrabus @SenseException can I get an approval on this? I'd like to continue and possibly finish the merge up before the hackathon.

[derrabus]

By all means, please continue.