doctrine/archived-orm
1
0
Fork 0
mirror of https://github.com/doctrine/orm.git synced 2026-04-24 06:58:19 +02:00
Code Issues 5.3k Packages Projects Releases 100 Wiki Activity

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

New Issue
Closed
opened 2026-01-22 12:39:36 +01:00 by admin · 21 comments
admin commented 2026-01-22 12:39:36 +01:00
Owner
Copy Link
Copy Source

Originally created by @doctrinebot on GitHub (Feb 28, 2010).

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().

Originally created by @doctrinebot on GitHub (Feb 28, 2010). 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().
admin added the Improvement label 2026-01-22 12:39:36 +01:00
admin closed this issue 2026-01-22 12:39:36 +01:00
admin commented 2026-01-22 12:39:38 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 1, 2010):

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 commented on GitHub (Mar 1, 2010): 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.
admin commented 2026-01-22 12:39:38 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 1, 2010):

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 commented on GitHub (Mar 1, 2010): 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](http://www.doctrine-project.org/jira/browse/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.
admin commented 2026-01-22 12:39:39 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 1, 2010):

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 commented on GitHub (Mar 1, 2010): 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.
admin commented 2026-01-22 12:39:40 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 1, 2010):

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 commented on GitHub (Mar 1, 2010): 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?
admin commented 2026-01-22 12:39:40 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 1, 2010):

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 commented on GitHub (Mar 1, 2010): 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.
admin commented 2026-01-22 12:39:42 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 1, 2010):

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 commented on GitHub (Mar 1, 2010): 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.
admin commented 2026-01-22 12:39:43 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 2, 2010):

Comment created by @guilhermeblanco:

New patch, supporting alias and more performant

@doctrinebot commented on GitHub (Mar 2, 2010): Comment created by @guilhermeblanco: New patch, supporting alias and more performant
admin commented 2026-01-22 12:39:44 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 2, 2010):

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 commented on GitHub (Mar 2, 2010): 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?
admin commented 2026-01-22 12:39:45 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 2, 2010):

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 commented on GitHub (Mar 2, 2010): 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 <ins> explode instead of strrpos </ins> 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.
admin commented 2026-01-22 12:39:46 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 2, 2010):

Comment created by @guilhermeblanco:

In r7300 this enhancement was committed.

@doctrinebot commented on GitHub (Mar 2, 2010): Comment created by @guilhermeblanco: In r7300 this enhancement was committed.
admin commented 2026-01-22 12:39:47 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 3, 2010):

Comment created by romanb:

Perfect, looks good. Nice work!

@doctrinebot commented on GitHub (Mar 3, 2010): Comment created by romanb: Perfect, looks good. Nice work!
admin commented 2026-01-22 12:39:48 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 3, 2010):

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 commented on GitHub (Mar 3, 2010): 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"
admin commented 2026-01-22 12:39:48 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 3, 2010):

Comment created by @guilhermeblanco:

Joshua found an issue in implementation, reopening it for inspection

@doctrinebot commented on GitHub (Mar 3, 2010): Comment created by @guilhermeblanco: Joshua found an issue in implementation, reopening it for inspection
admin commented 2026-01-22 12:39:50 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 3, 2010):

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 commented on GitHub (Mar 3, 2010): 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,
admin commented 2026-01-22 12:39:50 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 3, 2010):

Comment created by @guilhermeblanco:

Increased priority... this should be fixed ASAP.

@doctrinebot commented on GitHub (Mar 3, 2010): Comment created by @guilhermeblanco: Increased priority... this should be fixed ASAP.
admin commented 2026-01-22 12:39:50 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 3, 2010):

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 commented on GitHub (Mar 3, 2010): 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.
admin commented 2026-01-22 12:39:51 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 4, 2010):

Comment created by romanb:

This should be fixed now. Can you confirm?

@doctrinebot commented on GitHub (Mar 4, 2010): Comment created by romanb: This should be fixed now. Can you confirm?
admin commented 2026-01-22 12:39:52 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 4, 2010):

Comment created by jnye:

Yes, it's working again. Thanks.

@doctrinebot commented on GitHub (Mar 4, 2010): Comment created by jnye: Yes, it's working again. Thanks.
admin commented 2026-01-22 12:39:52 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 4, 2010):

Comment created by romanb:

Seems to be ok now.

@doctrinebot commented on GitHub (Mar 4, 2010): Comment created by romanb: Seems to be ok now.
admin commented 2026-01-22 12:39:52 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Mar 4, 2010):

Issue was closed with resolution "Fixed"

@doctrinebot commented on GitHub (Mar 4, 2010): Issue was closed with resolution "Fixed"
admin commented 2026-01-22 12:39:54 +01:00
Author
Owner
Copy Link
Copy Source

@doctrinebot commented on GitHub (Dec 13, 2015):

Imported 1 attachments from Jira into https://gist.github.com/2cb4ffaa21c9862eb0cd

@doctrinebot commented on GitHub (Dec 13, 2015): Imported 1 attachments from Jira into https://gist.github.com/2cb4ffaa21c9862eb0cd - [10408_patch-entity-namespace.diff](https://gist.github.com/2cb4ffaa21c9862eb0cd#file-10408_patch-entity-namespace-diff)
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
BC Break
Bug
Can't Fix
Deprecation
Documentation
DQL
Duplicate
Failing Test
Hacktoberfest
Improvement
Improvement
Incomplete
Invalid
Missing Tests
New Feature
PR Created
pull-request
Mirrored from GitHub Pull Request
 Question
Regression
Status: Waiting feedback
Waiting feedback
WIP
Won't Fix
No Label Improvement
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
admin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: doctrine/archived-orm#477
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?