Upgrade to doctrine/dbal:4 #1872

johanib · 2025-10-07T10:30:09Z

Needed for SF 6.4

MKodde

Nice work! We had a small discussion IRL where I questioned the current solution:

Could you make a more 'generic' solution for the object and array types that you now added. This would need to include passing the type of the array items that are stored for additional type checking. And maybe passing a max length as some types have different requirements for that..
Keep the current setup but add some additional (unit or integration) tests. I've found a couple of unit tests for two of the existing types

Question: are the types used in the existing functional tests? Or are they bypassed by the custom mock solution?

MKodde · 2025-11-03T09:57:26Z

src/OpenConext/EngineBlock/Metadata/Entity/ServiceProvider.php

     * @var string[]
     */
-    #[ORM\Column(name: 'allowed_idp_entity_ids', type: \Doctrine\DBAL\Types\Types::ARRAY, length: 6777215)]
+    #[ORM\Column(name: 'allowed_idp_entity_ids', type: 'json', length: 16777215, nullable: true)]


Was the (significant) length increase intentional?

I also see that the field became nullable, but that is now explicitly set for all of the updated columns. I'm assuming that was previously the default setting which now needs an explicit setting.

@johanib Note to self: Do we really want this? Better to avoid a migration and make specific types for the fields that use array, or copy the Array type into EB.

Error: Use SimpleArray, json is fundamentally different and requires a migration.

@MKodde I checked the length with: #1861 (comment)

Its currently mediumtext on prod, so it should be 16777215. I htink 6777215 was a typo.

johanib · 2026-02-09T15:06:30Z

src/OpenConext/EngineBlock/Metadata/Entity/AbstractRole.php

     * @var string[]
     */
-    #[ORM\Column(name: 'name_id_formats', type: \Doctrine\DBAL\Types\Types::ARRAY, length: 65535)]
+    #[ORM\Column(name: 'name_id_formats', type: 'json', length: 65535)]


Error: Use SimpleArray, json is fundamentally different and requires a migration.

johanib · 2026-02-09T15:08:25Z

src/OpenConext/EngineBlock/Metadata/Entity/ServiceProvider.php

     * @var string[]
     */
-    #[ORM\Column(name: 'allowed_idp_entity_ids', type: \Doctrine\DBAL\Types\Types::ARRAY, length: 6777215)]
+    #[ORM\Column(name: 'allowed_idp_entity_ids', type: 'json', length: 16777215, nullable: true)]


Error: Use SimpleArray, json is fundamentally different and requires a migration.

johanib · 2026-02-10T10:57:09Z

config/packages/doctrine.yaml

+            engineblock_requested_attribute_array: OpenConext\EngineBlockBundle\Doctrine\Type\RequestedAttributeArrayType
+            engineblock_shib_md_scope_array: OpenConext\EngineBlockBundle\Doctrine\Type\ShibMdScopeArrayType
+            engineblock_attribute_release_policy: OpenConext\EngineBlockBundle\Doctrine\Type\AttributeReleasePolicyType
+            engineblock_simple_array: OpenConext\EngineBlockBundle\Doctrine\Type\SimpleArrayType


Does doctrine not have a native simple array type??

https://www.doctrine-project.org/projects/doctrine-dbal/en/4.4/reference/types.html#simple-array

Is this what you are looking for?

Prior to this change, the DoctrineConnectionCheck from the monitor bundle was used instead of the check from EB. This became apparent as the doctrine/dbal broke the DoctrineConnectionHealthCheck from the Monitor bundle. See #1902

kayjoosten · 2026-02-11T10:15:10Z

config/packages/doctrine.yaml

+            engineblock_requested_attribute_array: OpenConext\EngineBlockBundle\Doctrine\Type\RequestedAttributeArrayType
+            engineblock_shib_md_scope_array: OpenConext\EngineBlockBundle\Doctrine\Type\ShibMdScopeArrayType
+            engineblock_attribute_release_policy: OpenConext\EngineBlockBundle\Doctrine\Type\AttributeReleasePolicyType
+            engineblock_simple_array: OpenConext\EngineBlockBundle\Doctrine\Type\SimpleArrayType


https://www.doctrine-project.org/projects/doctrine-dbal/en/4.4/reference/types.html#simple-array

Is this what you are looking for?

kayjoosten · 2026-02-11T10:20:32Z

src/OpenConext/EngineBlock/Metadata/Entity/AbstractRole.php

     * @deprecated Will be removed in favour of using the Mdui value object, use the getter for this field instead
     */
-    #[ORM\Column(name: 'logo', type: \Doctrine\DBAL\Types\Types::OBJECT)]
+    #[ORM\Column(name: 'logo', type: 'engineblock_logo', nullable: true)]


Maybe consider using a enum for the custom type. This will make it more save to use them without making mistakes. What do you think?

kayjoosten · 2026-02-11T10:21:28Z

src/OpenConext/EngineBlock/Metadata/Entity/AbstractRole.php

     * @var Organization
     */
-    #[ORM\Column(name: 'organization_nl_name', type: \Doctrine\DBAL\Types\Types::OBJECT, nullable: true, length: 65535)]
+    #[ORM\Column(name: 'organization_nl_name', type: 'engineblock_organization', nullable: true)]


As these are custom types for this project specific do we need the engineblock prefix?

kayjoosten · 2026-02-11T10:22:58Z

src/OpenConext/EngineBlock/Metadata/Entity/AbstractRole.php

     * @var string[]
     */
-    #[ORM\Column(name: 'name_id_formats', type: \Doctrine\DBAL\Types\Types::ARRAY, length: 65535)]
+    #[ORM\Column(name: 'name_id_formats', type: 'engineblock_simple_array', length: 65535)]


Looks like the simple array from doctrine should a good case for this one

kayjoosten · 2026-02-11T10:51:35Z

src/OpenConext/EngineBlockBundle/Doctrine/Type/LogoType.php

+    }
+
+    /**
+     * @param Logo|null $value


I see that you have added a param for the value here but thats not consistent with the other custom types. Is this on purpose?

johanib added this to PHP development Oct 7, 2025

github-project-automation bot moved this to New in PHP development Oct 7, 2025

johanib linked an issue Oct 7, 2025 that may be closed by this pull request

Upgrade to symfony 6.4 #1845

Open

johanib added the tech-debt label Oct 7, 2025

johanib moved this from New to In Progress in PHP development Oct 7, 2025

johanib mentioned this pull request Oct 7, 2025

Symfony/php upgrade voor Engineblock #1799

Closed

5 tasks

johanib changed the base branch from upgrade to upgrade-84 October 8, 2025 07:33

johanib marked this pull request as ready for review October 8, 2025 08:27

johanib requested a review from MKodde November 3, 2025 08:42

MKodde reviewed Nov 3, 2025

View reviewed changes

johanib force-pushed the upgrade-84 branch from dd8f74d to 5002bb7 Compare November 5, 2025 07:58

baszoetekouw assigned johanib Feb 2, 2026

johanib force-pushed the upgrade-84 branch from d76210f to 0e28ad8 Compare February 9, 2026 14:03

johanib force-pushed the feature/update_dbal_4_1799_3 branch 2 times, most recently from 805c922 to 271256a Compare February 9, 2026 15:10

johanib commented Feb 9, 2026

View reviewed changes

johanib force-pushed the feature/update_dbal_4_1799_3 branch from 271256a to cb43e68 Compare February 10, 2026 08:57

johanib force-pushed the upgrade-84 branch from 0e28ad8 to e51cbe4 Compare February 10, 2026 10:07

johanib force-pushed the feature/update_dbal_4_1799_3 branch from cb43e68 to fdc995d Compare February 10, 2026 10:10

johanib commented Feb 10, 2026

View reviewed changes

johanib force-pushed the upgrade-84 branch from e51cbe4 to 1d36306 Compare February 10, 2026 13:04

Upgrade to doctrine/dbal:^4

84b50bb

johanib force-pushed the feature/update_dbal_4_1799_3 branch from fdc995d to 84b50bb Compare February 10, 2026 13:04

Fix connection check

aa748af

Prior to this change, the DoctrineConnectionCheck from the monitor bundle was used instead of the check from EB. This became apparent as the doctrine/dbal broke the DoctrineConnectionHealthCheck from the Monitor bundle. See #1902

johanib requested a review from kayjoosten February 10, 2026 13:18

kayjoosten reviewed Feb 11, 2026

View reviewed changes

Upgrade to doctrine/dbal:4 #1872

Are you sure you want to change the base?

Upgrade to doctrine/dbal:4 #1872

Uh oh!

Conversation

johanib commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MKodde 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

johanib commented Oct 7, 2025 •

edited

Loading