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

Fix Metadata Caching when it changes in EventListeners #21

Conversation

andrey-bondar-dron
Copy link

@andrey-bondar-dron andrey-bondar-dron commented Oct 20, 2018

This PR fixed 2 bugs:

  1. Using https://www.doctrine-project.org/projects/doctrine-orm/en/2.6/cookbook/resolve-target-entity-listener.html with metadata cache is not working, because ResolveTargetEntityListener calls setMetadataFor without any effect on generated cache.
  2. Fallback metadata is not caching now.

I hope, it will be merged to version 1.0.2 and released ASAP.

@Majkl578 Majkl578 requested a review from alcaeus November 9, 2018 09:55
Copy link
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

Looking good, just some minor details

@lcobucci
Copy link
Member

Can you also rebase your branch to organise your commits, please?

alcaeus
alcaeus previously approved these changes Nov 11, 2018
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.

Apart from the issues highlighted by @lcobucci, LGTM! Thanks @andrey-bondar-dron!

@Majkl578 Majkl578 added this to the 1.1.0 milestone Nov 11, 2018
@Majkl578 Majkl578 added the Bug Something isn't working label Nov 11, 2018
@Majkl578 Majkl578 force-pushed the fix-metadata-caching branch from 93248ee to 6f785d5 Compare November 14, 2018 14:36
@Majkl578 Majkl578 requested a review from lcobucci November 14, 2018 14:39
@andrey-bondar-dron
Copy link
Author

Hi! Guys, someone has already fixed all required changes, while I was absent. Thanks. :)

Copy link
Contributor

@Majkl578 Majkl578 left a comment

Choose a reason for hiding this comment

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

The change and refactoring looks good overall, but there is one issue that needs to be addressed (see inline comment).

Note that class aliases are to be removed in next major, but now they only cause troubles (like out-of-sync metadata during setMetadata() or in cache).

@@ -167,7 +167,9 @@ public function getMetadataFor($className)

if (isset($this->loadedMetadata[$realClassName])) {
// We do not have the alias name in the map, include it
return $this->loadedMetadata[$className] = $this->loadedMetadata[$realClassName];
$this->setMetadataFor($realClassName, $this->loadedMetadata[$realClassName]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This particular change breaks in-memory caching ($this->loadedMetadata) for aliased classes, therefore breaking hot path at the top of this method for those:

public function testGettingAliasesOnlyFetchesFromCacheOnce() : void
{
    $loadedReflection = new ReflectionProperty(AbstractClassMetadataFactory::class, 'loadedMetadata');
    $loadedReflection->setAccessible(true);

    $this->cmf->getMetadataFor(RootEntity::class);
    self::assertArrayHasKey(RootEntity::class, $loadedReflection->getValue($this->cmf));
    self::assertArrayNotHasKey('Alias:RootEntity', $loadedReflection->getValue($this->cmf));

    $this->cmf->getMetadataFor('Alias:RootEntity');
    self::assertArrayHasKey(RootEntity::class, $loadedReflection->getValue($this->cmf));
    self::assertArrayHasKey('Alias:RootEntity', $loadedReflection->getValue($this->cmf));
}

It's because this condition has changed semantics and no longer does what the comment above says: "We do not have the alias name in the map, include it"

Copy link
Author

@andrey-bondar-dron andrey-bondar-dron Nov 21, 2018

Choose a reason for hiding this comment

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

@Majkl578 When it is needed to change metadata for some entity in EventListeners, we do not know about aliases that were used.
When I call setMetadataFor with real class name, AbstractClassMetadataFactory should replace metadata for all aliases in $loadedMetadata. And vise versa when I call setMetadataFor with alias.
For my opinion, it is normal when we will save only real classes in $loadedMetadata, because it prevent to have many problems with aliases.
On every call getMetadataFor with alias this code will be executed

// Check for namespace alias
        if (strpos($className, ':') !== false) {
            [$namespaceAlias, $simpleClassName] = explode(':', $className, 2);

            $realClassName = $this->getFqcnFromAlias($namespaceAlias, $simpleClassName);
        } else {
            $realClassName = $this->getRealClass($className);
        }

In order to prevent it I can create map for 'alias' => 'realClassName' and replace this code

        // Check for namespace alias
        if (strpos($className, ':') !== false) {
            [$namespaceAlias, $simpleClassName] = explode(':', $className, 2);

            $realClassName = $this->getFqcnFromAlias($namespaceAlias, $simpleClassName);
        } else {
            $realClassName = $this->getRealClass($className);
        }

with something like this

        switch (true) {
            case isset($this->aliasesMap[$className]):
                $realClassName = $this->aliasesMap[$className];
                break;
            case strpos($className, ':') !== false // Check for namespace alias
                [$namespaceAlias, $simpleClassName] = explode(':', $className, 2);
                $realClassName = $this->getFqcnFromAlias($namespaceAlias, $simpleClassName);
                $this->aliasesMap[$className] = $realClassName;
                break;
            default:
                $realClassName = $this->getRealClass($className);
        }

In this case AbstractClassMetadataFactory will not be too coupled with aliases in setMetadataFor/getMetadataFor methods.

Copy link
Author

Choose a reason for hiding this comment

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

So, fixed.
For now in in-memory cache (also in MetadataCache) will be saved single value for all aliases.
Calling setMetadataFor and getMetadataFor with aliases or FQCN will same keys.

@andrey-bondar-dron
Copy link
Author

@Majkl578 @lcobucci @alcaeus @malarzm Hi all! Give please your feedback.

@alcaeus alcaeus self-requested a review December 11, 2018 06:56
@lcobucci lcobucci requested a review from Majkl578 January 2, 2019 09:40
*/
private function getRealClass(string $class) : string
private function getRealClassName($className)
Copy link
Member

Choose a reason for hiding this comment

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

Type hints are here to stay: there's no reason you should assume anything else than a string for $className.

*/
private function getRealClass(string $class) : string
private function getRealClassName($className)
Copy link
Member

Choose a reason for hiding this comment

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

We may also profit from making this method protected: class name resolution for proxy class names is currently more difficult in ODM 2.0 and ORM 3.0 (master) since moving away from the legacy proxies. Overriding this method would at least temporarily solve this problem. Opinions @lcobucci @Majkl578?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants