[DoctrineBridge] Extend type guessing on enum fields #46676

Sajito · 2022-06-15T07:00:25Z

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	-
License	MIT
Doc PR	-

Doctrine supports enumType on array values, while the current implementation always assumes the value to be an enum object, if enumType is set.

If $typeOfField is json_array the collectionKeyType will still be null, while collectionValueType will be filled. That's because we have no way to determine from doctrine metadata, if the key is string or int. Yet I don't know if that's a valid thing to do, otherwise we should return null in that case.

carsonbot · 2022-06-15T07:00:29Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 6.2 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

carsonbot · 2022-06-15T07:00:32Z

It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you.

Cheers!

Carsonbot

dunglas · 2022-06-15T22:03:09Z

I would indeed return null instead of an incomplete type.

Doctrine supports enumType on array values. In those cases the guessed type should be of type array with collection information.

Sajito · 2022-06-16T08:36:33Z

Returning null now instead of a type without collectionKeyType.

norkunas · 2022-06-16T09:20:30Z

I also use just json field to declare array of enums, could we support that?

Sajito · 2022-06-16T09:31:00Z

Actually that's the reason for this PR. There is no way the DoctrineExtractor can guess the correct type from doctrine json field metadata. But that's not a problem, since getPhpType does not handle json fields, therefore will already return null for those cases. So another extractor will handle those cases.

nicolas-grekas · 2022-06-19T11:40:48Z

Thank you @Sajito.

…no Chianese) This PR was merged into the 4.4 branch. Discussion ---------- [DoctrineBridge] Extend type guessing on enum fields | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Doctrine supports enumType on array values, while the current implementation always assumes the value to be an enum object, if enumType is set. If $typeOfField is `json_array` the collectionKeyType will still be null, while collectionValueType will be filled. That's because we have no way to determine from doctrine metadata, if the key is string or int. Yet I don't know if that's a valid thing to do, otherwise we should return null in that case. Commits ------- 79239fe CS fix a9b0f43 [DoctrineBridge] Extend type guessing on enum fields

nicolas-grekas · 2022-06-19T11:59:12Z

src/Symfony/Bridge/Doctrine/PropertyInfo/DoctrineExtractor.php

-                            return [new Type(Type::BUILTIN_TYPE_ARRAY, $nullable, null, true, new Type(Type::BUILTIN_TYPE_INT), new Type(Type::BUILTIN_TYPE_STRING))];
+                            return [new Type(Type::BUILTIN_TYPE_ARRAY, $nullable, null, true, new Type(Type::BUILTIN_TYPE_INT), $enumType ?? new Type(Type::BUILTIN_TYPE_STRING))];
+                    }
+                case Type::BUILTIN_TYPE_INT:


I may have merged to fast here.
@Sajito is the missing break before this line on purpose? Can you please send a PR that adds either the "break" or the "no break" comment, with a test case ideally?

I have sent a new PR here: #46713.
I didn't know a "no break" comment would be required here, as I took these lines as a reference:

symfony/src/Symfony/Bridge/Doctrine/PropertyInfo/DoctrineExtractor.php

Lines 169 to 175 in 7fc7acb

switch ($typeOfField) {

case self::$useDeprecatedConstants ? DBALType::DATE : Types::DATE_MUTABLE:

case self::$useDeprecatedConstants ? DBALType::DATETIME : Types::DATETIME_MUTABLE:

case self::$useDeprecatedConstants ? DBALType::DATETIMETZ : Types::DATETIMETZ_MUTABLE:

case 'vardatetime':

case self::$useDeprecatedConstants ? DBALType::TIME : Types::TIME_MUTABLE:

return [new Type(Type::BUILTIN_TYPE_OBJECT, $nullable, 'DateTime')];

Hope it's fine with the new PR.

This PR was merged into the 4.4 branch. Discussion ---------- [DoctrineBridge] Add missing break ~~This PR only adds the "no break" comment as requested on my last PR:~~ This PR only adds the missing break as requested on my last PR: #46676 (comment) I hope it's fine to not include the describing table. ~~The missing break is on purpose, so here is no real code change.~~ The missing break should not have caused any side effects. I have not changed any tests here, because both cases are already tested by these two lines: https://github.com/symfony/symfony/blob/7fc7acb1e4448762f438af03cadb2ce54e3918e0/src/Symfony/Bridge/Doctrine/Tests/PropertyInfo/DoctrineExtractorTest.php#L186-L187 Commits ------- 445b78a [DoctrineBridge] Add missing break

nicolas-grekas · 2022-06-20T14:11:27Z

It looks like I failed at merging this PR into branch 5.4. There, the added test fails. Could you please have a look @Sajito?

Here is the failure on branch 5.4:

1) Symfony\Bridge\Doctrine\Tests\PropertyInfo\DoctrineExtractorTest::testExtractEnum
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
         'collectionKeyType' => Array (...)
         'collectionValueType' => Array (
             0 => Symfony\Component\PropertyInfo\Type Object (
-                'builtinType' => 'object'
+                'builtinType' => 'string'
                 'nullable' => false
-                'class' => 'Symfony\Bridge\Doctrine\Tests...numInt'
+                'class' => null
                 'collection' => false
                 'collectionKeyType' => Array ()
                 'collectionValueType' => Array ()

~/Code/symfony/src/Symfony/Bridge/Doctrine/Tests/PropertyInfo/DoctrineExtractorTest.php:142

Sajito · 2022-06-20T18:40:33Z

I created another pull request to fix the issue on the 5.4 branch.

… Chianese) This PR was merged into the 5.4 branch. Discussion ---------- [DoctrineBridge] Fix value type for simple_array | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | Fix for wrong collection value type on `simple_array` after merging the type guessing changes (#46676) into 5.4 branch. Commits ------- 6375990 [DoctrineBridge] Fix value type for simple_array

Sajito requested a review from dunglas as a code owner June 15, 2022 07:00

carsonbot added Status: Needs Review Bug DoctrineBridge labels Jun 15, 2022

carsonbot added this to the 4.4 milestone Jun 15, 2022

[DoctrineBridge] Extend type guessing on enum fields

a9b0f43

Doctrine supports enumType on array values. In those cases the guessed type should be of type array with collection information.

nicolas-grekas approved these changes Jun 19, 2022

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jun 19, 2022

nicolas-grekas merged commit a9b0f43 into symfony:4.4 Jun 19, 2022

nicolas-grekas reviewed Jun 19, 2022

View reviewed changes

Sajito mentioned this pull request Jun 19, 2022

[DoctrineBridge] Add missing break #46713

Merged

Sajito mentioned this pull request Jun 20, 2022

[DoctrineBridge] Fix value type for simple_array #46727

Merged

xabbuh mentioned this pull request Jun 21, 2022

[DoctrineBridge] fix merge #46729

Closed

This was referenced Jun 26, 2022

Release v4.4.43 #46780

Merged

Release v5.4.10 #46781

Merged

Release v6.0.10 #46782

Merged

Release v6.1.2 #46783

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DoctrineBridge] Extend type guessing on enum fields #46676

[DoctrineBridge] Extend type guessing on enum fields #46676

Sajito commented Jun 15, 2022

carsonbot commented Jun 15, 2022

carsonbot commented Jun 15, 2022

dunglas commented Jun 15, 2022

Sajito commented Jun 16, 2022

norkunas commented Jun 16, 2022

Sajito commented Jun 16, 2022

nicolas-grekas commented Jun 19, 2022

nicolas-grekas Jun 19, 2022

Sajito Jun 19, 2022

nicolas-grekas commented Jun 20, 2022 •

edited

Loading

Sajito commented Jun 20, 2022

	switch ($typeOfField) {
	case self::$useDeprecatedConstants ? DBALType::DATE : Types::DATE_MUTABLE:
	case self::$useDeprecatedConstants ? DBALType::DATETIME : Types::DATETIME_MUTABLE:
	case self::$useDeprecatedConstants ? DBALType::DATETIMETZ : Types::DATETIMETZ_MUTABLE:
	case 'vardatetime':
	case self::$useDeprecatedConstants ? DBALType::TIME : Types::TIME_MUTABLE:
	return [new Type(Type::BUILTIN_TYPE_OBJECT, $nullable, 'DateTime')];

[DoctrineBridge] Extend type guessing on enum fields #46676

[DoctrineBridge] Extend type guessing on enum fields #46676

Conversation

Sajito commented Jun 15, 2022

carsonbot commented Jun 15, 2022

carsonbot commented Jun 15, 2022

dunglas commented Jun 15, 2022

Sajito commented Jun 16, 2022

norkunas commented Jun 16, 2022

Sajito commented Jun 16, 2022

nicolas-grekas commented Jun 19, 2022

nicolas-grekas Jun 19, 2022

Choose a reason for hiding this comment

Sajito Jun 19, 2022

Choose a reason for hiding this comment

nicolas-grekas commented Jun 20, 2022 • edited Loading

Sajito commented Jun 20, 2022

nicolas-grekas commented Jun 20, 2022 •

edited

Loading