Use typed properties for default metadata for #7939 #8439

Lustmored · 2021-01-26T13:20:19Z

This is simple implementation of idea outlined in #7939. During completion step of a field mapping completion or association mapping completion it looks for typed property and, when existent, uses it to determine default for type, targetEntity and nullable (in fields and joinTable).

I have added simple tests covering all cases I found.

I am not a great fan of encapsulating code in PHP_VERSION_ID >= 70400, but at the time of writing got no better idea.

I couldn't get master branch to work locally and that's the only reason why I target '2.9.x' (Composer has problem resolving doctrine/dbal).

This is my first PR to Doctrine project, so if anything is done wrong please let me know and I will do my best to fix it :)

Lustmored · 2021-01-26T13:39:02Z

I don't know how to proceed with tests under PHP < 7.4 as there are no typed properties and entity class required for new test cases fails there. I probably could move it to other namespace, but that seems like complicating file structure without much benefit. Please help me with any suggestion :)

beberlei

Thank you for your PR, this is a very good first contribution, congrats :-)

I made some comments and have some questions inline.

beberlei · 2021-02-05T22:12:44Z

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

@@ -1414,6 +1420,53 @@ protected function _validateAndCompleteFieldMapping(array &$mapping)
            throw MappingException::missingFieldName($this->name);
        }

+        if (
+            PHP_VERSION_ID >= 70400
+            && isset($this->reflClass)


Just to understand, do you have a case where $this->reflClass is not set? I assume when DisconnectedMetadataFactory is used while converting an XML/YML mapping or generating entities?

The condition is duplicated below as well, can you factor it out into a private method?

private function isTypedProperty($name) : bool { }

When I remove isset($this->reflClass) there are 56 test errors and 3 failures with Error: Call to a member function hasProperty() on null message. I have no idea if those tests should be fixed or it is expected behavior and don't know Doctrine internals enough yet to determine if it can occur in real usage.

I can work through those tests and enforce setting $this->reflClass if that's desired, but someone more experience would need to take a look for real cases where unset reflClass is desired.

I have also factored condition as requested

It makes sense that it fails for when the DisconnectedMetadataFactory is used in case of EntityGenerator and exporter and such things. Of course this creates an inconsistency where the types default to "string" when reflClass does not exist. But there is no way to avoid that at the moment, so lets keep it this way.

beberlei · 2021-02-05T22:15:56Z

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

+                        case DateTimeImmutable::class:
+                            $mapping['type'] = 'datetime_immutable';
+                            break;
+                        case 'array':


this is ambiguous, could also be simple_array or object type. Not sure what to do, since otherwise it will default to "string" later.

My reasoning was to provide useful defaults instead of solving every possible use case. In this example having "string" as property type would simply fail at runtime as ORM would try to assign string value to array typed property. Therefore I believe useful default is better than nothing.

But the same problem comes with \DateType and many other fields. I believe the best long-term solution would be to create new interface, for example DefaultTypeResolverInterface that could look like this:

interface DefaultTypeResolverInterface { public function determineTypeForProperty(\ReflectionType $type): ?string; }

And provide at least one implementation (maybe working as here) and then allow users to choose implementation based on their config.

This way we could provide useful defaults for everyone and allow users to have simple method of providing own rules (for example I would benefit from overwriting \DateTimeImmutable in my projects).

I don't know how to approach such solution in ORM structure, but would love to do so. If this seems reasonable please just give me some directions and I will follow through.

Yes, lets stay with the hardcoded resolving rules for now and we'll leave the replacable resolving rules for a future PR.

You should document what PHP types get mapped to which Doctrine types in the documentation, see docs/ folder.

OK, I have added documentation where it seemed logical to me

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

Lustmored · 2021-02-06T08:49:33Z

Thanks for the review. I will work on it next week 👍

Lustmored · 2021-02-08T12:17:46Z

I have rebased onto 2.9.x (still some conflicts pop out, don't know why) and handled suggestions 👍

beberlei · 2021-02-08T18:06:45Z

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

+     *
+     * @return void
+     */
+    private function _validateAndCompleteTypedFieldMapping(array &$mapping)


don't pass by reference please but return the array again. This is how other _validate methods in this class do it already, so that stay consistent.

OK, I have changed _validateAndCompleteFieldMapping too while at it - it was still using reference.

beberlei · 2021-02-08T18:07:23Z

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

+     *
+     * @return void
+     */
+    private function _validateAndCompleteTypedAssociationMapping(array &$mapping)


same here, return the array again and don't pass by reference

Lustmored · 2021-02-09T10:55:16Z

I think I addressed all current comments to some degree. I see that checks are somehow stuck and I see no way to "unstuck" them to see if my code passes. Is it possible somehow to run them?

beberlei · 2021-02-09T18:06:23Z

@Lustmored this is because of the conflict in ClassMetadataInfo.php, you need to fix that before the checks run.

beberlei

Just two small changes, then I believe this is ready to go.

beberlei · 2021-02-09T18:07:28Z

docs/en/reference/annotations-reference.rst

@@ -715,6 +717,7 @@ Required attributes:

 -  **targetEntity**: FQCN of the referenced target entity. Can be the
   unqualified class name if both classes are in the same namespace.
+   When typed properties are used it is inherited from PHP type.


I would formulate this a bit differently: You can omit this value if you use a PHP property type instead.

OK, changed

beberlei · 2021-02-09T18:10:59Z

tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php

+
+        // Join table Nullable
+        $cm->mapOneToOne(['fieldName' => 'email', 'joinColumns' => [[]]]);
+        $this->assertFalse($cm->getAssociationMapping('email')['joinColumns'][0]['nullable']);


Missing assertion on target_entity.

Right, missed it. Assertion added

Lustmored · 2021-02-10T08:52:07Z

I think I have complied with all your comments. I have also rebased onto 2.9.x and pushed CS fixed.

Only remaining failing check is CS complaing about method names starting with underscore - I don't have the confidence to change them without asking first.

Lustmored · 2021-02-18T09:42:58Z

I have rebased onto 2.9 and made a simple change to use Types:: constants instead of strings for types. Is there anything else I can do to make this happen? :)

beberlei · 2021-03-01T21:14:22Z

@Lustmored thank you very much!

beberlei · 2021-04-04T09:35:26Z

@Lustmored FYI, This doesn't work anymore with AttributeDriver (does it with Annotations?) Because of the default value of Column::$type = "string". The typed mapping completion only works when $mapping['type'] is not set. I am going to investigate more.

Lustmored · 2021-04-04T10:44:04Z

@beberlei I see the problem and I see how I missed it in tests (I have created only tests for ClassMetadataInfo and not metadata obtained from mapping drivers). I am also looking into it. Probably simplest solution would be to change the rule from isset to ! isset($foo) || $foo === 'string', but that would lead to obvious problem of impossibility to overwrite type inherited from typed property with a simple string type.

beberlei · 2021-04-04T10:56:15Z

@Lustmored i think setting Column::$type to nullable & default null is better

Lustmored · 2021-04-04T10:57:55Z

@beberlei That's exactly what I'm experimenting with right now. Probably will come with a PR fixing the issue and introducing more tests to ensure it works with mapping later today 👍

Lustmored · 2021-04-04T12:30:57Z

@beberlei let's continue in #8589. Hopefully all cases are covered there this time :)

Lustmored force-pushed the 2.9.x branch from ea8af53 to ce31327 Compare January 26, 2021 13:34

Lustmored mentioned this pull request Jan 26, 2021

Typed Properties and default column/association types #7939

Closed

beberlei requested changes Feb 5, 2021

View reviewed changes

beberlei added this to the 2.9.0 milestone Feb 5, 2021

Lustmored requested a review from beberlei February 8, 2021 12:17

beberlei requested changes Feb 8, 2021

View reviewed changes

Lustmored requested a review from beberlei February 9, 2021 10:55

beberlei requested changes Feb 9, 2021

View reviewed changes

Lustmored force-pushed the 2.9.x branch from 53ad1a3 to 12818a4 Compare February 10, 2021 08:36

Lustmored requested a review from beberlei February 10, 2021 08:52

Lustmored added 11 commits February 18, 2021 10:28

Use typed properties for default metadata for doctrine#7939

4c2355c

Coding Standards

a6ce4c0

Remove $name from CmsUserTypes and adapt tests

20ce8d0

Factor out conditions required for typed property

6a307eb

Factor out typed validation and completion methods

729ddfc

Move Typed tests model to separate namespace

4731190

Don't pass by reference, return array

514ae54

Document changes to default mapping for typed properties

8017bf0

Better wording in annotation reference

6303da3

Add missing targetEntity assertion on typed association

91ffc1c

Try to comply with CS

61d81d2

Lustmored force-pushed the 2.9.x branch from be96425 to 05f5cea Compare February 18, 2021 09:28

Lustmored added 2 commits February 18, 2021 10:30

USe constants instead of strings

05f5cea

Use one-line comments for single line content

086601c

beberlei added 3 commits March 1, 2021 21:54

Merge branch '2.9.x' into Lustmored2.9.x

c6dca03

phpcs

cfbbd48

phpcs

7613eab

beberlei approved these changes Mar 1, 2021

View reviewed changes

beberlei merged commit b3ed525 into doctrine:2.9.x Mar 1, 2021

Lustmored deleted the 2.9.x branch March 4, 2021 09:43

jmsche mentioned this pull request May 25, 2021

Upgrade from 2.8.x to 2.9.1: possible BC break for nullable behaviour #8723

Closed

Warxcell mentioned this pull request May 25, 2021

Add automatic type detection for Embedded. #8724

Merged

troytft mentioned this pull request May 27, 2021

Resolve embeded class by property type #8729

Closed

mbabker mentioned this pull request Jun 24, 2021

Restore bc for annotations #8758

Merged

franmomu mentioned this pull request Feb 20, 2022

Use typed properties for default metadata doctrine/mongodb-odm#2411

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use typed properties for default metadata for #7939 #8439

Use typed properties for default metadata for #7939 #8439

Lustmored commented Jan 26, 2021

Lustmored commented Jan 26, 2021

beberlei left a comment

beberlei Feb 5, 2021

beberlei Feb 5, 2021

Lustmored Feb 8, 2021

Lustmored Feb 8, 2021

beberlei Feb 8, 2021

beberlei Feb 5, 2021

Lustmored Feb 8, 2021

beberlei Feb 8, 2021

Lustmored Feb 9, 2021

Lustmored commented Feb 6, 2021

Lustmored commented Feb 8, 2021

beberlei Feb 8, 2021

Lustmored Feb 9, 2021

beberlei Feb 8, 2021

Lustmored Feb 9, 2021

Lustmored commented Feb 9, 2021

beberlei commented Feb 9, 2021

beberlei left a comment

beberlei Feb 9, 2021

Lustmored Feb 10, 2021

beberlei Feb 9, 2021

Lustmored Feb 10, 2021

Lustmored commented Feb 10, 2021

Lustmored commented Feb 18, 2021

beberlei commented Mar 1, 2021

beberlei commented Apr 4, 2021

Lustmored commented Apr 4, 2021

beberlei commented Apr 4, 2021

Lustmored commented Apr 4, 2021

Lustmored commented Apr 4, 2021

Use typed properties for default metadata for #7939 #8439

Use typed properties for default metadata for #7939 #8439

Conversation

Lustmored commented Jan 26, 2021

Lustmored commented Jan 26, 2021

beberlei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lustmored commented Feb 6, 2021

Lustmored commented Feb 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lustmored commented Feb 9, 2021

beberlei commented Feb 9, 2021

beberlei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lustmored commented Feb 10, 2021

Lustmored commented Feb 18, 2021

beberlei commented Mar 1, 2021

beberlei commented Apr 4, 2021

Lustmored commented Apr 4, 2021

beberlei commented Apr 4, 2021

Lustmored commented Apr 4, 2021

Lustmored commented Apr 4, 2021