Add support for Entity namespacing globally

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.

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.

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.

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?

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.

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.

New patch, supporting alias and more performant

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?

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

, 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.

In r7300 this enhancement was committed.

Perfect, looks good. Nice work!

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"

Joshua found an issue in implementation, reopening it for inspection

@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,

Increased priority... this should be fixed ASAP.

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.

This should be fixed now. Can you confirm?

Yes, it's working again. Thanks.

Seems to be ok now.