Tame doctrine migrations / determine which we want to apply. by johanib · Pull Request #1917 · OpenConext/OpenConext-engineblock

johanib · 2026-02-26T14:51:51Z

Prior to this change, doctrine wants to change the longtext types of consent_settings and idp_discoveries to json. This has some implications, as at a db level, as it requires a migration. This migration is not strictly needed.

Ensure no database migrations are applied by configuring doctrine to use the old legacy mappings, until we have reason to upgrade in a coordinated operation.

Even with this change, doctrine wants to remove the comments. But we can safely apply this in the next release, without causing any big migrations.

After this change, doctrine wants to create a migration for this.

--- <unnamed>
+++ <unnamed>
@@ -10,37 +10,37 @@
   `display_name_nl` varchar(255) NOT NULL,
   `display_name_en` varchar(255) NOT NULL,
   `display_name_pt` varchar(255) NOT NULL,
-  `logo` longtext NOT NULL COMMENT '(DC2Type:object)',
-  `organization_nl_name` text DEFAULT NULL COMMENT '(DC2Type:object)',
-  `organization_en_name` text DEFAULT NULL COMMENT '(DC2Type:object)',
-  `organization_pt_name` text DEFAULT NULL COMMENT '(DC2Type:object)',
+  `logo` longtext NOT NULL,
+  `organization_nl_name` text DEFAULT NULL,
+  `organization_en_name` text DEFAULT NULL,
+  `organization_pt_name` text DEFAULT NULL,
   `keywords_nl` varchar(255) NOT NULL,
   `keywords_en` varchar(255) NOT NULL,
   `keywords_pt` varchar(255) NOT NULL,
-  `certificates` text NOT NULL COMMENT '(DC2Type:array)',
+  `certificates` text NOT NULL,
   `workflow_state` varchar(255) NOT NULL,
-  `contact_persons` text NOT NULL COMMENT '(DC2Type:array)',
+  `contact_persons` text NOT NULL,
   `name_id_format` varchar(255) DEFAULT NULL,
-  `name_id_formats` text NOT NULL COMMENT '(DC2Type:array)',
-  `single_logout_service` text DEFAULT NULL COMMENT '(DC2Type:object)',
+  `name_id_formats` text NOT NULL,
+  `single_logout_service` text DEFAULT NULL,
   `requests_must_be_signed` tinyint(1) NOT NULL,
   `manipulation` text NOT NULL,
   `type` varchar(255) NOT NULL,
-  `attribute_release_policy` text DEFAULT NULL COMMENT '(DC2Type:array)',
-  `assertion_consumer_services` text DEFAULT NULL COMMENT '(DC2Type:array)',
-  `allowed_idp_entity_ids` mediumtext DEFAULT NULL COMMENT '(DC2Type:array)',
+  `attribute_release_policy` text DEFAULT NULL,
+  `assertion_consumer_services` text DEFAULT NULL,
+  `allowed_idp_entity_ids` mediumtext DEFAULT NULL,
   `allow_all` tinyint(1) DEFAULT NULL,
-  `requested_attributes` text DEFAULT NULL COMMENT '(DC2Type:array)',
+  `requested_attributes` text DEFAULT NULL,
   `enabled_in_wayf` tinyint(1) DEFAULT NULL,
-  `single_sign_on_services` text DEFAULT NULL COMMENT '(DC2Type:array)',
-  `shib_md_scopes` text DEFAULT NULL COMMENT '(DC2Type:array)',
+  `single_sign_on_services` text DEFAULT NULL,
+  `shib_md_scopes` text DEFAULT NULL,
   `support_url_en` varchar(255) DEFAULT NULL,
   `support_url_pt` varchar(255) DEFAULT NULL,
   `support_url_nl` varchar(255) DEFAULT NULL,
-  `consent_settings` longtext DEFAULT NULL COMMENT '(DC2Type:json)',
-  `coins` longtext NOT NULL COMMENT '(DC2Type:engineblock_metadata_coins)',
-  `mdui` longtext NOT NULL COMMENT '(DC2Type:engineblock_metadata_mdui)',
-  `idp_discoveries` longtext DEFAULT NULL COMMENT '(DC2Type:json)',
+  `consent_settings` longtext DEFAULT NULL,
+  `coins` longtext NOT NULL,
+  `mdui` longtext NOT NULL,
+  `idp_discoveries` longtext DEFAULT NULL,
   PRIMARY KEY (`id`),
   UNIQUE KEY `idx_sso_provider_roles_entity_id_type` (`type`,`entity_id`),
   KEY `idx_sso_provider_roles_type` (`type`),

Before this change, Doctrine wanted to migrate to this:

--- <unnamed>
+++ <unnamed>
@@ -10,37 +10,37 @@
   `display_name_nl` varchar(255) NOT NULL,
   `display_name_en` varchar(255) NOT NULL,
   `display_name_pt` varchar(255) NOT NULL,
-  `logo` longtext NOT NULL COMMENT '(DC2Type:object)',
-  `organization_nl_name` text DEFAULT NULL COMMENT '(DC2Type:object)',
-  `organization_en_name` text DEFAULT NULL COMMENT '(DC2Type:object)',
-  `organization_pt_name` text DEFAULT NULL COMMENT '(DC2Type:object)',
+  `logo` longtext NOT NULL,
+  `organization_nl_name` text DEFAULT NULL,
+  `organization_en_name` text DEFAULT NULL,
+  `organization_pt_name` text DEFAULT NULL,
   `keywords_nl` varchar(255) NOT NULL,
   `keywords_en` varchar(255) NOT NULL,
   `keywords_pt` varchar(255) NOT NULL,
-  `certificates` text NOT NULL COMMENT '(DC2Type:array)',
+  `certificates` text NOT NULL,
   `workflow_state` varchar(255) NOT NULL,
-  `contact_persons` text NOT NULL COMMENT '(DC2Type:array)',
+  `contact_persons` text NOT NULL,
   `name_id_format` varchar(255) DEFAULT NULL,
-  `name_id_formats` text NOT NULL COMMENT '(DC2Type:array)',
-  `single_logout_service` text DEFAULT NULL COMMENT '(DC2Type:object)',
+  `name_id_formats` text NOT NULL,
+  `single_logout_service` text DEFAULT NULL,
   `requests_must_be_signed` tinyint(1) NOT NULL,
   `manipulation` text NOT NULL,
   `type` varchar(255) NOT NULL,
-  `attribute_release_policy` text DEFAULT NULL COMMENT '(DC2Type:array)',
-  `assertion_consumer_services` text DEFAULT NULL COMMENT '(DC2Type:array)',
-  `allowed_idp_entity_ids` mediumtext DEFAULT NULL COMMENT '(DC2Type:array)',
+  `attribute_release_policy` text DEFAULT NULL,
+  `assertion_consumer_services` text DEFAULT NULL,
+  `allowed_idp_entity_ids` mediumtext DEFAULT NULL,
   `allow_all` tinyint(1) DEFAULT NULL,
-  `requested_attributes` text DEFAULT NULL COMMENT '(DC2Type:array)',
+  `requested_attributes` text DEFAULT NULL,
   `enabled_in_wayf` tinyint(1) DEFAULT NULL,
-  `single_sign_on_services` text DEFAULT NULL COMMENT '(DC2Type:array)',
-  `shib_md_scopes` text DEFAULT NULL COMMENT '(DC2Type:array)',
+  `single_sign_on_services` text DEFAULT NULL,
+  `shib_md_scopes` text DEFAULT NULL,
   `support_url_en` varchar(255) DEFAULT NULL,
   `support_url_pt` varchar(255) DEFAULT NULL,
   `support_url_nl` varchar(255) DEFAULT NULL,
-  `consent_settings` longtext DEFAULT NULL COMMENT '(DC2Type:json)',
-  `coins` longtext NOT NULL COMMENT '(DC2Type:engineblock_metadata_coins)',
-  `mdui` longtext NOT NULL COMMENT '(DC2Type:engineblock_metadata_mdui)',
-  `idp_discoveries` longtext DEFAULT NULL COMMENT '(DC2Type:json)',
+  `consent_settings` longtext CHARACTER SET utf8mb4 COLLATE utf8mb4_bin DEFAULT NULL CHECK (json_valid(`consent_settings`)),
+  `coins` longtext CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL CHECK (json_valid(`coins`)),
+  `mdui` longtext CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL CHECK (json_valid(`mdui`)),
+  `idp_discoveries` longtext CHARACTER SET utf8mb4 COLLATE utf8mb4_bin DEFAULT NULL CHECK (json_valid(`idp_discoveries`)),
   PRIMARY KEY (`id`),
   UNIQUE KEY `idx_sso_provider_roles_entity_id_type` (`type`,`entity_id`),
   KEY `idx_sso_provider_roles_type` (`type`),

Prior to this change, doctrine wants to change the longtext types of consent_settings and idp_discoveries to `json`. This has some implications, as at a db level, as it requires a migration. This migration is not strictly needed. Ensure no database migrations are applied by configuring doctrine to use the old legacy mappings, until we have reason to upgrade in a coordinated operation. Even with this change, doctrine wants to remove the comments. But we can safely apply this in the next release, without causing any big migrations.

MKodde · 2026-03-05T09:55:07Z

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

        // We want a `TEXT` field declaration in our column, the `LONGTEXT` default causes issues when running the
        // DBMS in strict mode.
-        $column['length'] = 65535;
-        return $platform->getJsonTypeDeclarationSQL($column);
+        $column['length'] = null; // Ensure a longtext, this is currently on prod.
+        return $platform->getClobTypeDeclarationSQL($column);


The comment on l33..34 contradict the comment on l35. No doubt the new comment is correct and reflects the real world situation. Maybe remove the old comment to prevent questions in the future?

Also this makes me wonder. Was the longtext issue we ran into before fixed in the DBMS? Or..

MKodde · 2026-03-05T10:05:11Z

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

+
+    public function getSQLDeclaration(array $column, AbstractPlatform $platform): string
+    {
+        return $platform->getClobTypeDeclarationSQL([]);


Below, you explicitly pass length = null to ensure no data changes occur. Maybe for consistency sake do this here to? I know the default length is null if you do not pass it.

Also here you do not pass the $column configuration to the getClobTypeDeclarationSQL method. Which is intentional and I get it. It is different from the other custom type implementations. Maybe give that a quick extra thought before merging this. 🤸‍♂️

MKodde · 2026-03-05T10:05:52Z

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

+ *
+ * Use this type instead of Doctrine\DBAL\Types\Types::JSON for columns that should remain as longtext.
+ */
+class LegacyJsonType extends Type


The other types got some nice'n shiny tests. Why not generate some for this custom type?

MKodde · 2026-03-05T10:07:25Z

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

+ * This type stores JSON data in a longtext column, deliberately avoiding migration to a native MySQL JSON column type.
+ * The native JSON column type (declared via getJsonTypeDeclarationSQL) was introduced when upgrading to doctrine/dbal:^4,
+ * but migrating existing longtext columns to JSON columns is not desirable at this time.


Perhaps you could advertise that the column definitions that can be passed in the ORM column definition attribute are NOT passed along to the ORM and that only the default column definition is used for this type?

johanib mentioned this pull request Feb 26, 2026

Upgrade to doctrine/dbal:4 #1872

Merged

johanib added this to PHP development Feb 26, 2026

github-project-automation bot moved this to New in PHP development Feb 26, 2026

johanib moved this from New to Backlog in PHP development Feb 26, 2026

baszoetekouw moved this from Backlog to In Progress in PHP development Mar 2, 2026

johanib force-pushed the feature/update_dbal_4_1799_3 branch from b83392f to 156245f Compare March 3, 2026 08:48

Base automatically changed from feature/update_dbal_4_1799_3 to upgrade-84 March 3, 2026 12:29

johanib force-pushed the feature/ensure_no_schema_changes branch from e9849bf to 3eebb07 Compare March 5, 2026 09:46

johanib marked this pull request as ready for review March 5, 2026 09:48

johanib requested a review from MKodde March 5, 2026 09:48

MKodde requested changes Mar 5, 2026

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tame doctrine migrations / determine which we want to apply. #1917

Tame doctrine migrations / determine which we want to apply. #1917
johanib wants to merge 1 commit intoupgrade-84from
feature/ensure_no_schema_changes

johanib commented Feb 26, 2026

Uh oh!

MKodde Mar 5, 2026

Uh oh!

MKodde Mar 5, 2026

Uh oh!

MKodde Mar 5, 2026

Uh oh!

MKodde Mar 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

johanib commented Feb 26, 2026

Uh oh!

MKodde Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

MKodde Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

MKodde Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

MKodde Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants