Removing the metadata instantiation from the EM by mikeSimonson · Pull Request #7143 · doctrine/orm

mikeSimonson · 2018-03-20T13:07:33Z

The metadata instantiation is kept in the create method for BC for now.
A new static method is added to be able to pass the metadata to the EM.

TODOs

Document BC Breakages
Add lazy collection implementation (populated as-we-go)
Ensure type aliases are correctly cached (create key copies in the metadata)
Ensure the \\ ltrim() is gone

The metadata instantiation is kept in the create method for BC for now. A new static method is added to be able to pass the metadata to the EM.

mikeSimonson · 2018-03-20T13:09:30Z

@Ocramius Is this what you meant when we discussed at phpbenelux ?
Is the next step refactoring all the call to create to the new createWithMetadata function ?

Ocramius · 2018-03-20T13:09:32Z

@mikeSimonson the idea was to make the metadata a ClassMetadata[] lazy collection (injectable) rather than a ClassMetadataFactory - is this a middle-ground before we go there?

mikeSimonson · 2018-03-20T13:10:52Z

I just wanted to make sure that I was going in the right direction.
I should pull lazymap as a dep then I suppose ?

Ocramius · 2018-03-20T13:11:56Z

@mikeSimonson I'd suggest making it an eager map for now, and later we fix it with LazyMap

mikeSimonson · 2018-03-23T15:31:19Z

Does the new commit goes in the right direction ?

Can I
get rid of the cache configuration in the EM constructor ?
change the behavior of getAllMetadata for simplicity ?
remove the initialize thingy in the ClassMetadataFactory ?

How do I proceed to remove the entity manager from the OnClassMetadataNotFoundEventArgs in the ClassMetadataFactory and LoadClassMetadataEventArgs ?

Any other remarks / comment / direction ?
@Ocramius

Ocramius · 2018-03-23T18:13:39Z

lib/Doctrine/ORM/EntityManager.php

+    /**
+     * @return MetadataCollection
+     */
+    public function getMetadatas(): MetadataCollection


Would avoid exposing it here at all.

Ocramius · 2018-03-23T18:14:24Z

lib/Doctrine/ORM/EntityManager.php

+     * @return MetadataCollection
     */
-    public function getConnection()
+    public function getMetadatas(): MetadataCollection


Avoid exposing this

Ocramius · 2018-03-23T18:14:47Z

lib/Doctrine/ORM/EntityManager.php

    public function getClassMetadata($className) : Mapping\ClassMetadata
    {
-        return $this->metadataFactory->getMetadataFor($className);
+        $className = StaticClassNameConverter::getRealClass($className);


This can likely be dealt with in a MetadataCollection decorator instead

Ocramius · 2018-03-23T18:15:28Z

lib/Doctrine/ORM/EntityManager.php

+        $this->conn            = $conn;
+        $this->config          = $config;
+        $this->eventManager    = $eventManager;
+        $this->metadatas       = $metadataCollection;


metadata is irregular: the plural of metadata is metadata. A better way to name this is probably mappings

Ocramius · 2018-03-23T18:15:53Z

lib/Doctrine/ORM/EntityManager.php

+                try{
+                    $this->metadatas->get(StaticClassNameConverter::getClass($value));
+                    $id[$i] = $this->unitOfWork->getSingleIdentifierValue($value);
+                } catch (\Exception $e) {


Most likely wrong here

Ocramius · 2018-03-23T18:17:08Z

lib/Doctrine/ORM/EntityManager.php

+                try{
+                    $this->metadatas->get(StaticClassNameConverter::getClass($value));
+                    $id[$i] = $this->unitOfWork->getSingleIdentifierValue($value);
+                } catch (\Exception $e) {


Likely wrong here - why did you introduce these try-catches? I know you are still playing around with the patch: just wondering why this got added to make things work

just because for now get throws an Exception if the metadata is not found where it wasn't before.

Ocramius · 2018-03-23T18:17:42Z

lib/Doctrine/ORM/EntityManager.php

-        return new EntityManager($connection, $config, $connection->getEventManager());
+        $metadataFactoryClassName = $config->getClassMetadataFactoryName();
+        /** @var \Doctrine\Common\Persistence\Mapping\ClassMetadataFactory $metadataFactory */
+        $metadataFactory          = new $metadataFactoryClassName($config, $connection, $eventManager);


👍 this is a decent BC-compliant approach for now :)

Ocramius · 2018-03-23T18:19:18Z

lib/Doctrine/ORM/Mapping/MetadataCollection.php

+        return $this->classMetadatas[$name];
+    }
+
+    public static function fromClassMetadatas(array $classMetadatas)


I suggest using (ClassMetadata $firstClass, ClassMetadata ...$otherClasses) here, which prevents instantiation if no metadata is available at all.

Ocramius · 2018-03-23T18:19:28Z

lib/Doctrine/ORM/Mapping/MetadataCollection.php

+
+    public function get($name)
+    {
+        if(!isset($this->classMetadatas[$name])){


Add the class name normalisation here

Ocramius · 2018-03-23T18:20:29Z

lib/Doctrine/ORM/Mapping/MetadataCollection.php

+
+    private function __construct(array $classMetadatas)
+    {
+        $metadataByClassName = array_reduce (


This is simpler with:

array_combine( array_map(function (ClassMetadata $metadata) { return $metadata->getClassName(); }, $metadata), $metadata );

Ocramius · 2018-03-23T18:25:33Z

Does the new commit goes in the right direction ?

👍

Can I get rid of the cache configuration in the EM constructor ?

Most likely. Metadata caching can be moved to a CachedMetadataCollection or such

change the behavior of getAllMetadata for simplicity ?

That is a subtle BC break: please don't change that.

remove the initialize thingy in the ClassMetadataFactory ?

Yes: since metadata now has a lifecycle that is completely separate from the ORM itself, removal is fine here 👍

How do I proceed to remove the entity manager from the OnClassMetadataNotFoundEventArgs in the ClassMetadataFactory and LoadClassMetadataEventArgs ?

I suggest we completely remove these events, as the user can simply decorate the MetadataCollection in order to manipulate metadata information

Any other remarks / comment / direction ?

Provided above :-)

as it's completely separeted from the ORM lifecycle now, at least it's the goal

Making sure that it can't be instantiated with an empty array

Majkl578 · 2018-03-25T14:38:25Z

lib/Doctrine/ORM/Mapping/MetadataCollection.php

+        return $this->metadata[$name];
+    }
+
+    public static function fromClassMetadatas(ClassMetadata $firstClass, ClassMetadata ...$otherClasses)


Why is this a static instead of constructor directly?

Also the signature is ugly, let's just have __construct(ClassMetadata ...$metadata) with assert(count($metadata) !== 0) if it really MUST be non-empty.

Majkl578 · 2018-03-25T14:40:58Z

lib/Doctrine/ORM/EntityManager.php


            default:
-                $class = $this->config->getCustomHydrationMode($hydrationMode);
+                $class = $this->config->getCustomHydrationMode((string) $hydrationMode);


This is most likely wrong, see #7130 (comment). It could be int|string currently.

getCustomHydrationMode expect a string in it's function definition.

Changed this in my PR since it's really int|string now. Something to re-do later on probably.

mikeSimonson · 2018-04-12T21:11:15Z

@Ocramius My main question now is to know if passing the whole mapping collection to the BasicEntityPersister is the way to go ? (commit extract the MetadataFactory from the basic entity persister and the following one)

Ocramius

@Ocramius My main question now is to know if passing the whole mapping collection to the BasicEntityPersister is the way to go ? (commit extract the MetadataFactory from the basic entity persister and the following one)

@mikeSimonson I'd suggest that any component requiring metadata should just be given the collection. The MetadataCollection is now to be considered a "dumb" immutable structure, so it should be fine to pass it around.

Obviously, if only one entity metadata is required in a persister, then it should be passed in explicitly, but I don't know how big that change would be.

It will be easy to extract these scenarios when looking at classes that always get() on the collection with the same parameter.

Ocramius · 2018-04-13T10:55:55Z

lib/Doctrine/ORM/EntityManager.php

+                try{
+                    $this->mappings->get($value);
+                    $id[$i] = $this->unitOfWork->getSingleIdentifierValue($value);
+                } catch (\Exception $e) {


We need to get rid of these later on.

Ocramius · 2018-04-13T10:56:11Z

lib/Doctrine/ORM/EntityManager.php

        foreach ($id as $i => $value) {
-            if (is_object($value) && $this->metadataFactory->hasMetadataFor(StaticClassNameConverter::getClass($value))) {
-                $id[$i] = $this->unitOfWork->getSingleIdentifierValue($value);
+            if (is_object($value)) {


The API for getting metadata by object or string should be separate

Ocramius · 2018-04-13T10:57:20Z

lib/Doctrine/ORM/EntityManager.php

-            if (is_object($value) && $this->metadataFactory->hasMetadataFor(StaticClassNameConverter::getClass($value))) {
-                $scalarId[$i] = $this->unitOfWork->getSingleIdentifierValue($value);
+            if (is_object($value)) {
+                try{


Same here - try-catch block needs to go

Ocramius · 2018-04-13T10:57:41Z

lib/Doctrine/ORM/EntityManager.php

-            if (is_object($value) && $this->metadataFactory->hasMetadataFor(StaticClassNameConverter::getClass($value))) {
-                $id[$i] = $this->unitOfWork->getSingleIdentifierValue($value);
+            if (is_object($value)) {
+                try{


Same: try-catch should be removed overall. We possibly need a better abstraction.

Ocramius · 2018-04-13T11:02:02Z

lib/Doctrine/ORM/Mapping/MetadataCollection.php

+
+    public function get(string $name) : ClassMetadata
+    {
+        $name = StaticClassNameConverter::getRealClass($name);


Consider checking for existence before calling StaticClassNameConverter. Copying a metadata reference into a different key is also a good idea for performance reasons

Ocramius · 2018-04-13T11:02:16Z

lib/Doctrine/ORM/Mapping/MetadataCollection.php

+     */
+    private $metadata;
+
+    private function __construct(array $metadata)


Document this please

Ocramius · 2018-04-13T11:03:51Z

lib/Doctrine/ORM/Mapping/MetadataCollection.php

+        $this->metadata = $metadataByClassName;
+    }
+
+    public function get(string $name) : ClassMetadata


The various APIs getting class metadata by object reference could need a getByObject(object $object)

Ocramius · 2018-04-13T11:06:16Z

tests/Doctrine/Tests/OrmFunctionalTestCase.php


        $config->setMetadataDriverImpl(
            $mappingDriver ?? $config->newDefaultAnnotationDriver([
+                realpath(__DIR__ . '/Models/CMS'),


Wondering why this test changed to load everything - is this because we don't yet have a lazy collection?

Just because I am pocking around a bit everywhere to try and fix everything that I broke.

Ocramius · 2018-04-13T15:33:06Z

@mikeSimonson I've updated the PR description with the TODOs matching the discussion above.

Majkl578 · 2018-05-04T21:52:43Z

@mikeSimonson Do you plan to continue with this?

mikeSimonson · 2018-05-05T12:57:15Z

@Majkl578 yes. I don't have much time right now but I hope that I will have it in a good state for amsterdamphp

derrabus · 2022-05-11T09:28:06Z

I'm closing this PR because it hasn't seen progress for several years. Please open a new one if you want to pick up the work one day.

Removing the metadata instantiation from the EM

89afca3

The metadata instantiation is kept in the create method for BC for now. A new static method is added to be able to pass the metadata to the EM.

mikeSimonson assigned Ocramius Mar 20, 2018

WIP

bc4133e

WIP EM tests pass

1d8b666

Majkl578 added this to the 3.0 milestone Mar 23, 2018

Majkl578 added WIP Improvement BC Break labels Mar 23, 2018

Ocramius suggested changes Mar 23, 2018

View reviewed changes

mikeSimonson added 14 commits March 25, 2018 09:20

Removing those event that won't be necessary anymore

742a389

No need to manage the initalization lifecycle anymore

e1a03df

as it's completely separeted from the ORM lifecycle now, at least it's the goal

adding a property for the mapping driver

6511274

removing subtle BC break

cd87209

Not exposing the metadatas from the entity manager

c5cec3f

Removing the left cache concern from the entity manager

81964d6

Moving the responsability of cleaning the class name to the collection

98e1b82

simplifying the metadata collection

ab610c9

Making sure that it can't be instantiated with an empty array

Correcting the property name for clarity

34b2de0

Adding type hint and return type

92fee1f

Making the Event manager required for now (debug easier)

b60ded3

Moving the responsability of the canonicalizing the class name

746672b

Fixing test WIP

d8ca5cf

The StaticClassNameConverter is already called in the MetadataCollection

f0d6f93

Majkl578 reviewed Mar 25, 2018

View reviewed changes

mikeSimonson added 6 commits April 12, 2018 22:13

fix missed namespace migration

5406415

WIP

344c0ba

simplify the tests

8075fa8

add typing

788bf4c

extract the MetadataFactory from the basic entity persister

a5b1336

Adapt entity manager and unit of work to the basic persister change

77e0021

Ocramius suggested changes Apr 13, 2018

View reviewed changes

Ocramius assigned mikeSimonson and unassigned Ocramius Apr 13, 2018

Base automatically changed from master to old-prototype-3.x February 23, 2021 08:18

derrabus closed this May 11, 2022

derrabus removed this from the 3.0.0 milestone May 11, 2022

Conversation

mikeSimonson commented Mar 20, 2018 • edited by Ocramius Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODOs

Uh oh!

mikeSimonson commented Mar 20, 2018

Uh oh!

Ocramius commented Mar 20, 2018

Uh oh!

mikeSimonson commented Mar 20, 2018

Uh oh!

Ocramius commented Mar 20, 2018

Uh oh!

mikeSimonson commented Mar 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ocramius commented Mar 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeSimonson commented Apr 12, 2018

Uh oh!

Ocramius left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ocramius commented Apr 13, 2018

Uh oh!

Majkl578 commented May 4, 2018

Uh oh!

mikeSimonson commented May 5, 2018

Uh oh!

derrabus commented May 11, 2022

Uh oh!

mikeSimonson commented Mar 20, 2018 •

edited by Ocramius

Loading