Upgrade to doctrine/dbal:4 by johanib · Pull Request #1872 · OpenConext/OpenConext-engineblock

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.

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

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.

config/packages/doctrine.yaml

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

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

kayjoosten

Test are still failing

johanib · 2026-02-26T10:28:57Z

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?

Yes, this was the correct way to go from the start. Implemented now.
Also added tests for the two 'new' types.
The behat tests fail if I omit the serialize, so yes, they are used in the behat tests.

MKodde · 2026-02-26T11:04:30Z

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)]


Let me start by complimenting you on this change. This makes the PR so much more to the point!

Two things caught my eye:

Is there no need to set the column length anymore? Did we set the default value excessively before?

And, I was also triggered by you setting the nullable value for each field now. I'm OK with that, before we would only set it if the non-default value was required.

Does this change cause 0 schema changes? If it does, you might need to think about a strategy to roll out that change safely. But I guess there are none judging the code.

As discussed.
Do not pass default values. So I removed the nullable: false.
This makes it in-line with the removed default lengths.
It does not result in any db schema changes, but will dig into that to see why the changed nullable: does not result in changes.

I will revert the nullable: false changes.
The reason is that we use inheritance: IdentityProvider and ServiceProvider both extend the AbstractRole. So doctrine enforces all fields in the children als nullable: false. Which makes sense.

So adding nullable: false is plain misleading.

johanib · 2026-02-26T11:27:17Z

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

     * @var IndexedService[]
     */
-    #[ORM\Column(name: 'assertion_consumer_services', type: \Doctrine\DBAL\Types\Types::ARRAY, length: 65535)]
+    #[ORM\Column(name: 'assertion_consumer_services', type: SerializedArrayType::NAME, nullable: true)]


@kayjoosten raised the question: Why is are these fields made nullable: true.

A: To mirror the database, where they are currently nullable.

Weirdly, changing nullable to true / false does not result in schema changes. I will look into this to make sure.

Doctrine forces these fields to nullable, as this entity inherits from the AbstractRole and shares the same table with IdentityProvider.

I will remove them and add a comment to the AbstractRole.

johanib · 2026-02-26T13:54:49Z

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: SerializedObjectType::NAME, length: 65535, nullable: true)]


Another detail: Currently, on prod, this is a TEXT. In order to keep it TEXT, pass the correct length. Otherwise, it will be mapped to a LONGTEXT.

See: https://github.com/doctrine/dbal/blob/476f7f0fa6ea4aa5364926db7fabdf6049075722/src/Platforms/AbstractMySQLPlatform.php#L159

- The old array / object serialization types from doctrine have been moved / integrated into our codebase for backward compatability. - Fix typo in allowed_idp_entity_ids. The db is a medium text, which is 16777215 bytes. This change aligns the column with the mapping. - Make the nullable: false explicit to avoid future confusion. Confusion could stem from the fact that the fields are nullable in code, but not nullable in the database. This is because a serialized `null` results in something like `s:` in the database.

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

config/packages/doctrine.yaml Outdated Show resolved Hide resolved

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

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

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

kayjoosten reviewed Feb 11, 2026

View reviewed changes

kayjoosten approved these changes Feb 13, 2026

View reviewed changes

kayjoosten requested changes Feb 13, 2026

View reviewed changes

github-project-automation bot moved this from In Progress to Backlog in PHP development Feb 13, 2026

johanib moved this from Backlog to In Progress in PHP development Feb 25, 2026

johanib force-pushed the upgrade-84 branch from 1d36306 to 45bd19f Compare February 25, 2026 14:19

johanib force-pushed the feature/update_dbal_4_1799_3 branch from 6f86ba5 to 074c48f Compare February 25, 2026 14:20

kayjoosten approved these changes Feb 26, 2026

View reviewed changes

johanib force-pushed the feature/update_dbal_4_1799_3 branch 4 times, most recently from b019847 to 29b6168 Compare February 26, 2026 10:22

johanib requested a review from MKodde February 26, 2026 10:30

kayjoosten approved these changes Feb 26, 2026

View reviewed changes

MKodde reviewed Feb 26, 2026

View reviewed changes

johanib commented Feb 26, 2026

View reviewed changes

johanib force-pushed the feature/update_dbal_4_1799_3 branch 3 times, most recently from 17fbd2f to dbadae3 Compare February 26, 2026 13:51

johanib commented Feb 26, 2026

View reviewed changes

johanib force-pushed the feature/update_dbal_4_1799_3 branch from dbadae3 to b83392f Compare February 26, 2026 14:23

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kayjoosten left a comment

Choose a reason for hiding this comment

Uh oh!

johanib commented Feb 26, 2026

Uh oh!

MKodde Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johanib Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johanib Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

MKodde Feb 26, 2026 •

edited

Loading

johanib Feb 26, 2026 •

edited

Loading

johanib Feb 26, 2026 •

edited

Loading