Use typed properties for default metadata #2411

franmomu · 2022-02-20T00:20:30Z

Q	A
Type	improvement
BC Break	no
Fixed issues

Summary

~~I'll retarget to 2.4.x once is created.~~

This is kind of based on doctrine/orm#8439 and doctrine/orm#8589.

The idea is that if you use typed properties, it gets the type from that properties. For array, I've chosen hash, I thought it was more right.

IonBazan · 2022-02-20T06:58:56Z

Do you think we could also add support for References and embedded documents there?

docs/en/reference/basic-mapping.rst

franmomu · 2022-02-20T19:17:57Z

Do you think we could also add support for References and embedded documents there?

Right now it sets targetDocument for EmbedOne and ReferenceOne if that's what you mean.

IonBazan

Nice, I think I missed that during review 👍🏻 This looks great and good to go from my side 🚀

I'm just wondering if we need to add the changelog entry, as it could potentially introduce a BC break for people who relied on non-strict PHP type casting:

#[Field] // Will be stored in DB as `string` but casted back to `int`
private int $myProp;

which doesn't seem like a big deal though.

malarzm

Awesome idea and job porting the feature from ORM! Two additional ideas:

Do you know why ORM is not inferring nullable typed properties? I think we should do that, seems pretty straightforward, but I wonder if there are any dragons down the road.
In the same vein as EmbedOne we could autocomplete collectionClass attribute for EmbedMany/ReferenceMany. I can take care of it in a follow-up PR if you'd like me to

malarzm · 2022-02-21T19:36:51Z

docs/en/reference/basic-mapping.rst

+- ``bool``: ``bool``
+- ``float``: ``float``
+- ``int``: ``int``
+- ``string`` or any other type: ``string``


Suggested change

- ``string`` or any other type: ``string``

- ``string``: ``string``

I'd remove the "or any other type" part as this paragraph is before we teach user what types we have. Now I wrote that I think the section should be below listing all available mapping types :)

malarzm · 2022-02-21T19:38:37Z

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php

+    {
+        $type = $this->reflClass->getProperty($mapping['fieldName'])->getType();
+
+        if ($type instanceof ReflectionNamedType && ! isset($mapping['type'])) {


It'd be better to have an early return instead of a nested switch.

malarzm · 2022-02-21T19:40:57Z

tests/Doctrine/ODM/MongoDB/Tests/Mapping/ClassMetadataTest.php

+
+        // float
+        $cm->mapField(['fieldName' => 'float']);
+        self::assertEquals(Type::FLOAT, $cm->getTypeOfField('float'));


int seems to be missing compared to the list of what is autocompleted :)

franmomu · 2022-02-22T07:41:36Z

Awesome idea and job porting the feature from ORM! Two additional ideas:

Do you know why ORM is not inferring nullable typed properties? I think we should do that, seems pretty straightforward, but I wonder if there are any dragons down the road.

In the same vein as EmbedOne we could autocomplete collectionClass attribute for EmbedMany/ReferenceMany. I can take care of it in a follow-up PR if you'd like me to

I'll take a look at those maybe later today, with the nullable type I remember there were some BC breaks as doctrine/orm#8723, but I haven't checked the reason.

franmomu · 2022-02-22T07:42:48Z

I'm just wondering if we need to add the changelog entry, as it could potentially introduce a BC break for people who relied on non-strict PHP type casting:
#[Field] // Will be stored in DB as `string` but casted back to `int`
private int $myProp;
which doesn't seem like a big deal though.

Interesting! I'll add a comment for that case

franmomu · 2022-02-22T18:31:09Z

About the nullable type property see doctrine/orm#8723 (comment)

I'm not sure if that would introduce some unwanted behaviour 🤔

tests/Doctrine/ODM/MongoDB/Tests/Mapping/AbstractMappingDriverTest.php

IonBazan · 2022-02-23T02:13:58Z

Regarding nullable properties, I think we should leave it in current state. The behaviour in ODM is slightly different - nullable parameter decides whether to store the fields that were set to null and I would expect it to require explicit nullable: true declaration to make it store the field when using typed properties.

malarzm

As for nullable types I'd suggest to leave this PR as is and merge it once checks are passing. Then create a follow-up PR that will emit a deprecation once it detect typed nullable property that is not on par with mapping with intention to throw an exception in 3.0.

malarzm · 2022-03-06T17:37:35Z

UPGRADE-2.4.md

+private int $myProp;
+```
+
+This property will be stored in DB as `string` but casted back to `int`.


Suggested change

This property will be stored in DB as `string` but casted back to `int`.

This property will be stored in DB as `string` but casted back to `int`. Please note that at this

time, due to backward compatibility reasons, nullable type does not imply `nullable` mapping.

malarzm · 2022-03-06T17:39:29Z

docs/en/reference/basic-mapping.rst

+- ``float``: ``float``
+- ``int``: ``int``
+- ``string``: ``string``
+


I'd add similar note about nullable as in upgrade docs as the behaviour is not fully intuitional

malarzm · 2022-03-08T08:08:45Z

tests/Documents74/UserTyped.php

+    #[ODM\ReferenceMany]
+    public CustomCollection $referenceMany;
+
+    public static function loadMetadata(ClassMetadata $metadata): void


Just noticed: what's this?

🤔 It was copied from the ORM thinking about us having support for Static PHP Mapping, but we are not, so I'll remove it, thanks!

malarzm

Are you fixing the PHPStan failure here or in a follow-up PR as it seems unrelated?

franmomu · 2022-03-08T08:11:37Z

Are you fixing the PHPStan failure here or in a follow-up PR as it seems unrelated?

I was creating #2417 👍 to target 2.3.x

malarzm · 2022-03-08T16:14:41Z

Thanks @franmomu 🚀

malarzm · 2022-03-22T07:52:41Z

About the deprecation thing: https://doctrine.slack.com/archives/CAAE6T66B/p1647934766611479?thread_ts=1647885533.164609&cid=CAAE6T66B

@IonBazan some may prefer to keep these null values in DB, but some could decide to skip them and this is controlled by nullable: true/false accordingly. The name is a bit unfortunate for someone who comes from ORM background because it actually means “store null values” but that’s another story. I chose not to store my nulls in the DB on purpose.

@malarzm Actually I never used ODM in a way that I have a null in the object, not-nullable mapping, and these two factors combined end with field not being written or $unset. Frankly I thought that'd throw an exception somewhere in the reflection land

franmomu added the Feature label Feb 20, 2022

franmomu changed the base branch from 2.3.x to 2.4.x February 20, 2022 06:32

franmomu marked this pull request as ready for review February 20, 2022 06:32

franmomu added this to the 2.4.0 milestone Feb 20, 2022

IonBazan reviewed Feb 20, 2022

View reviewed changes

docs/en/reference/basic-mapping.rst Outdated Show resolved Hide resolved

IonBazan mentioned this pull request Feb 20, 2022

add BackedEnum support #2412

Closed

franmomu force-pushed the type_properties branch 5 times, most recently from 5cb2d81 to cc02d38 Compare February 20, 2022 19:11

IonBazan approved these changes Feb 21, 2022

View reviewed changes

malarzm reviewed Feb 21, 2022

View reviewed changes

franmomu force-pushed the type_properties branch from cc02d38 to 5d29ffd Compare February 22, 2022 18:22

franmomu force-pushed the type_properties branch 2 times, most recently from 8afcb4d to 90600fb Compare February 22, 2022 18:38

IonBazan reviewed Feb 23, 2022

View reviewed changes

tests/Doctrine/ODM/MongoDB/Tests/Mapping/AbstractMappingDriverTest.php Show resolved Hide resolved

franmomu force-pushed the type_properties branch 2 times, most recently from 493e765 to c04baac Compare March 5, 2022 06:45

malarzm approved these changes Mar 6, 2022

View reviewed changes

franmomu force-pushed the type_properties branch from c04baac to 7276a92 Compare March 8, 2022 07:30

malarzm reviewed Mar 8, 2022

View reviewed changes

malarzm approved these changes Mar 8, 2022

View reviewed changes

franmomu force-pushed the type_properties branch 2 times, most recently from 23ee469 to bfa8161 Compare March 8, 2022 08:27

Use typed properties for default metadata

ee0f876

franmomu force-pushed the type_properties branch from bfa8161 to ee0f876 Compare March 8, 2022 13:13

malarzm merged commit e3da298 into doctrine:2.4.x Mar 8, 2022

franmomu deleted the type_properties branch March 8, 2022 16:32

This was referenced May 4, 2022

Remove targetDocument autocompletion #2442

Merged

Allow using enums directly in queries #2447

Merged

malarzm mentioned this pull request Mar 29, 2023

Too few arguments error thrown for Doctrine\ODM\MongoDB\PersistentCollection - Constructor #2505

Closed

malarzm mentioned this pull request Jun 30, 2024

Detect field type with property type #2661

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use typed properties for default metadata #2411

Use typed properties for default metadata #2411

franmomu commented Feb 20, 2022 •

edited

Loading

IonBazan commented Feb 20, 2022

franmomu commented Feb 20, 2022

IonBazan left a comment

malarzm left a comment

malarzm Feb 21, 2022

malarzm Feb 21, 2022

malarzm Feb 21, 2022

franmomu commented Feb 22, 2022

franmomu commented Feb 22, 2022

franmomu commented Feb 22, 2022

IonBazan commented Feb 23, 2022 •

edited

Loading

malarzm left a comment

malarzm Mar 6, 2022 •

edited

Loading

malarzm Mar 6, 2022

malarzm Mar 8, 2022

franmomu Mar 8, 2022

malarzm left a comment

franmomu commented Mar 8, 2022

malarzm commented Mar 8, 2022

malarzm commented Mar 22, 2022

	- ``string`` or any other type: ``string``
	- ``string``: ``string``

	This property will be stored in DB as `string` but casted back to `int`.
	This property will be stored in DB as `string` but casted back to `int`. Please note that at this
	time, due to backward compatibility reasons, nullable type does not imply `nullable` mapping.

Use typed properties for default metadata #2411

Use typed properties for default metadata #2411

Conversation

franmomu commented Feb 20, 2022 • edited Loading

Summary

IonBazan commented Feb 20, 2022

franmomu commented Feb 20, 2022

IonBazan left a comment

Choose a reason for hiding this comment

malarzm left a comment

Choose a reason for hiding this comment

malarzm Feb 21, 2022

Choose a reason for hiding this comment

malarzm Feb 21, 2022

Choose a reason for hiding this comment

malarzm Feb 21, 2022

Choose a reason for hiding this comment

franmomu commented Feb 22, 2022

franmomu commented Feb 22, 2022

franmomu commented Feb 22, 2022

IonBazan commented Feb 23, 2022 • edited Loading

malarzm left a comment

Choose a reason for hiding this comment

malarzm Mar 6, 2022 • edited Loading

Choose a reason for hiding this comment

malarzm Mar 6, 2022

Choose a reason for hiding this comment

malarzm Mar 8, 2022

Choose a reason for hiding this comment

franmomu Mar 8, 2022

Choose a reason for hiding this comment

malarzm left a comment

Choose a reason for hiding this comment

franmomu commented Mar 8, 2022

malarzm commented Mar 8, 2022

malarzm commented Mar 22, 2022

franmomu commented Feb 20, 2022 •

edited

Loading

IonBazan commented Feb 23, 2022 •

edited

Loading

malarzm Mar 6, 2022 •

edited

Loading