Skip to content

Loading…

DDC-384: Add support for Entity namespacing globally #4689

Closed
doctrinebot opened this Issue · 21 comments

1 participant

@doctrinebot

Jira issue originally created by user @guilhermeblanco:

Recently Jonathan committed a patch to support Entity aliasing.
Patch at first glance looks good, but it has some issues (it double processes the loadedMetadata).

I have fixed this issue by including a new level of configuration. Instead of create a 1 alias => 1 entity hashmap, I added support for namespacing.
The idea is similar to AnnotationReader support recently introduced.

To take advantage of it, simply include the $ormConfig->addEntityNamespace('CMS', 'Doctrine\Tests\ORM\Models');
Then, just do: "SELECT u FROM CMS:CmsUser u"

This is also supported in $em->getReference(), ->getRepository(), etc.

Patch is attached including updated unit test plus another test for getRepository().

@doctrinebot

Comment created by romanb:

My initial comments:

  • T_COLON is not needed, its not a token and never used. ** The lexer pattern should remain simple: '[a-z_][a-z0-9_:\]**'
  • The changes to ClassMetadataFactory#getMetadataFor are not good because they bypass the cache, invoking a method and functions. This is the most used method of all times. 1 more method call here means hundreds or even thousands more method calls in practice, 1 more function invocation means hundreds or even thousands more function calls in practice. strrpos is not needed also because a colon ":" should always separate a namespace and there should always only be 1 colon, if any.

== UPDATE ==

I just realized that the old alias map implementation completely bypassed the cache (ouch!). You would never get a hit on $this->_loadedMetadata with an alias because the metadata is only stored for the fully qualified class name. As a result the metadata is loaded again and again and again on each invocation, whether its from the metadata cache or from the sources, both is awful.

Summary:

1) The old/current alias map implementation is badly broken.
2) The new implementation is too slow and I dont see an elegant way to do it properly.

When reconsidering that we're talking of a (imho minor) convenience feature here, unless someone has a good implementation for getMetadataFor that is both fast and correct, I dont see a way to support that feature, neither the old one nor the new one.

@doctrinebot

Comment created by romanb:

The new implementation could use double-check approach on the cache after the full class name has been invoked, similar to what I have shown in a comment on DDC-379. Only people who use aliasing would suffer an extra perf. hit from extracting the namespace and double-checking the cache.

Again, any code that resolves namespace aliases must be inside the outermost if ( ! isset($this->_loadedMetadata[$className])) block, not outside because that impacts every single call, even for people who dont use aliasing.

@doctrinebot

Comment created by romanb:

Here is my suggestion for the getMetadataFor implementation:

    public function getMetadataFor($className)
    {
        if ( ! isset($this->_loadedMetadata[$className])) {
            if (strpos($className, ':') !== false) {
                list($namespaceAlias, $simpleClassName) = explode(':', $className);
                $className = $this->_em->getConfiguration()->getEntityNamespace($namespaceAlias) . '\\' . $simpleClassName;
                // Check cache again after resolving the full class name
                if (isset($this->_loadedMetadata[$className])) {
                    return $this->_loadedMetadata[$className];
                }
            }

            $cacheKey = "$className\$CLASSMETADATA";
            if ($this->_cacheDriver) {
                if (($cached = $this->_cacheDriver->fetch($cacheKey)) !== false) {
                    $this->_loadedMetadata[$className] = $cached;
                } else {
                    foreach ($this->_loadMetadata($className) as $loadedClassName) {
                        $this->*cacheDriver->save($cacheKey, $this->*loadedMetadata[$className], null);
                    }
                }
            } else {
                $this->_loadMetadata($className);
            }
        }

        return $this->_loadedMetadata[$className];
    }

Note that even this implementation means when you use aliasing you get extra strpos/explode/method calls on the configuration on every invocation but at least you have the choice. People who dont use aliasing are only punished with a single strpos() on a cache-miss.

Thats the only feasible implementation I can see currently.

@doctrinebot

Comment created by @guilhermeblanco:

T_COLON:
Sorry, forgot to remove that.

LEXER::
NO. It must be changed. It's a lexical error to have an identifier named: "Foo:" or even "MyProject\" and it's currently supported as a valid one.
It's a bug that is not directly related to this enhancement, but it's still a bug that must be fixed.

We validate this AbstractSchemaName as wrong only in semantical checks, when actually trying to load the class using $exists = class_exists($schemaName, true);
That's wrong and must be fixed.

STRRPOS:
I don't have the direct intention, but the purpose of strrpos is to actually support subnamespacing of entities. In theory, this would be supported:

$ormConfig->addEntityNamespace('FMM', 'FMM\Entity')
                      ->addEntityNamespace('FMM:Wiki', 'FMM\Wiki\Entity');

$q = $em->createQuery('SELECT a FROM FMM:Wiki:Article a');

Of course by adding the alias, we support subnamespacing WITHIN the same namespace by doing:

$q = $em->createQuery('SELECT e FROM FMM:SomeModule\Entity e');

Finally, I agree that I wasn't happy with the bypass of cache hit and surely I can add the second lookup in cache hash map.
But also, I think we could take advatange of ClassMetadataFactory::setMetadataFor($className, $class) and consider to add an alias there too, preventing even the substr. Since class instances are always passed by reference, this would have a very low resources usage increase, but almost none performant impact. What do you think?

@doctrinebot

Comment created by romanb:

LEXER:
-Well, the purpose of our lexer is not to be ultimately strict. Even with your change it sill allows ":Foo" and other weird edge-cases. Once you start wanting to make everything super-strict you're on a road with no end. Keep the lexer patterns as simple as possible. False positives are not really problematic, only false negatives are.-
OK.

STRRPOS:
I think subnamespacing unnecessarily overcomplicates things. Lets stay simple.

Re: Storing alias + full class name in _loadedMetadata.
I thought about that but it can not be implemented easily because *loadMetadata populates *loadedMetadata directly and must do so in order to work properly. Thus I discarded the idea, it would spread the knowledge of aliasing out across larger parts of the code, we dont want that.

I still think the implementation I proposed above is the best.

@doctrinebot

Comment created by @guilhermeblanco:

LEXER: Fine... will commit the patch when we agree on final things. If we didn't reach an optimal implementation, patch is not 100% correct.

STRRPOS: I'm fine with that. That would make code much more simple.

HASHMAP OF ALIAS:
I can be implemented and my patch is already done. Will upload it as soon as I get my implementation updated (due to previous subject).
I found another issue inside the getMetadataFor that I like to talk to you, so once possible... let's chat.

@doctrinebot

Comment created by @guilhermeblanco:

New patch, supporting alias and more performant

@doctrinebot

Comment created by @jwage:

Roman, what is your opinion on this? Are you ok with Guilhermes patch now or would you still like to go with your original proposed patch?

@doctrinebot

Comment created by romanb:

The new patch looks smart but I would still like to see _resolveClassName inlined and simplified according to my earlier proposal in a comment (strpos explode instead of strrpos substr + substr).

Btw, in the lexer pattern I think you can leave off the {1}, it makes no difference, the set [a-z0-9_] already means 1 char of this set afaik.

Apart from these things the patch looks good now.

@doctrinebot

Comment created by @guilhermeblanco:

In r7300 this enhancement was committed.

@doctrinebot

Comment created by romanb:

Perfect, looks good. Nice work!

@doctrinebot

Comment created by jnye:

I think this broke proxy objects.
"Parse error: syntax error, unexpected ':', expecting '{' in /home/josh/src/superman/app/models/proxies/app:UserProxy.php on line 6"

@doctrinebot

Comment created by @guilhermeblanco:

Joshua found an issue in implementation, reopening it for inspection

@doctrinebot

Comment created by @guilhermeblanco:

@Joshua, could you please provide more information about what were you trying to do when you got this issue, etc?

Depending how complex the issue is, it may be required for us to revert my changeset and try a different approach.

Cheers,

@doctrinebot

Comment created by @guilhermeblanco:

Increased priority... this should be fixed ASAP.

@doctrinebot

Comment created by jnye:

I was using the new namespace syntax while trying to get a reference. I'm not sure if support for this was meant or not.

$config->setAutoGenerateProxyClasses(true); // Enable proxy class generation.
$config->addEntityNamespace('app', 'app\models'); // Setup namespace on the EntityManager configuration.
$em = EntityManager::create($conn, $config);
$user = $em->getReference('app:User', 3610); // Get reference. Triggers generation of invalid PHP proxy class.

@doctrinebot

Comment created by romanb:

This should be fixed now. Can you confirm?

@doctrinebot

Comment created by jnye:

Yes, it's working again. Thanks.

@doctrinebot

Comment created by romanb:

Seems to be ok now.

@doctrinebot

Issue was closed with resolution "Fixed"

@doctrinebot doctrinebot added this to the 2.0-BETA1 milestone
@doctrinebot doctrinebot closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.