Upgrade doctrine/dbal to 4 - #632
Open
kayjoosten wants to merge 4 commits into
Open
Conversation
If applied, this commit will make the ApiBundle Doctrine Type classes compile and run correctly against doctrine/dbal 4.3, unblocking the dbal4 upgrade that was rebased onto this branch. Why is this change needed? Prior to this change, the dbal4 upgrade rebase compiled but crashed at runtime. dbal4 tightened several Type contracts: convertToPHPValue/ convertToDatabaseValue now require typed mixed $value parameters, IntegerType and BooleanType lock their convertToPHPValue return types to ?int/?bool (breaking any subclass that converts to a value object), and ConversionException::conversionFailed() was removed outright. doctrine/dbal 4.4 also dropped LegacySchemaManagerFactory, which the installed doctrine/doctrine-bundle 2.19 still references, so the container failed to build. How does it address the issue? This change types every untyped $value parameter across the Type classes as mixed, switches the six option types that convert database integers/booleans into value objects (NumberOfTokensPerIdentityType, SsoRegistrationBypassOptionType, SsoOn2faOptionType, SelfVetOptionType, SelfAssertedTokensOptionType, VerifyEmailOptionType) from extending IntegerType/BooleanType to extending Type directly with an explicit getBindingType(), and replaces the removed ConversionException::conversionFailed() calls with ValueNotConvertible::new(). It also pins doctrine/dbal to ^4.3,<4.4 to stay on the last release still compatible with doctrine-bundle 2.19, matching the dbal version already validated by CI upstream.
If applied, this commit will make the doctrine/dbal 4 upgrade depend on a repository we control instead of a personal fork. Why is this change needed? Prior to this change, broadway/event-store-dbal was sourced from a contributor's personal GitHub fork (dev-feature/dbal4 branch). That branch carries the dbal4 compatibility fix upstream hasn't merged yet (broadway/event-store-dbal#75), but pinning a dependency to a personal account is fragile: the branch or account could disappear or change without notice. How does it address the issue? This change points the "repositories" entry and the broadway/event-store-dbal requirement at https://github.com/SURFnet/broadway-event-store-dbal, a fork under our own org carrying the same feature/dbal4 branch and commit. composer.lock is regenerated to match; the resolved commit is unchanged.
If applied, this commit will make an invalid stored value for number-of-tokens-per-identity fail loudly instead of silently becoming zero, and cleans up a stray indentation mismatch. Why is this change needed? Prior to this change, NumberOfTokensPerIdentityType::convertToPHPValue defaulted non-scalar database values to 0 instead of raising a conversion error, unlike every sibling Type class (and DateTimeType), which throw ValueNotConvertible on an unexpected shape. That silently masks a genuine data problem. Separately, composer.json's new "repositories" block used 2-space indentation while the rest of the file uses 4-space. How does it address the issue? This change makes convertToPHPValue throw ValueNotConvertible for non-scalar values before attempting the cast, matching the error handling used elsewhere in this file and across the other Type classes. It also re-indents the "repositories" block to match the rest of composer.json.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mixed $valueparams, switching option types that convert to value objects offIntegerType/BooleanTypeontoTypedirectly, and replacing the removedConversionException::conversionFailed()withValueNotConvertible::new().doctrine/dbalto^4.3,<4.4(doctrine-bundle 2.19 still referencesLegacySchemaManagerFactory, removed in dbal 4.4).broadway/event-store-dbalfrom https://github.com/SURFnet/broadway-event-store-dbal (feature/dbal4 branch) instead of a personal fork, carrying the fix from Upgradedoctrine/dbal3 > 4 broadway/event-store-dbal#75 until it's merged upstream.Supersedes #556.
Test plan
php vendor/bin/phpstan analyse --memory-limit=-1 --no-ansi -c ./ci/qa/phpstan.neon./ci/qa/phpcsphp vendor/bin/phpunit --configuration=ci/qa/phpunit.xml(1130 tests)php vendor/bin/phpunit --configuration=ci/qa/phpunit.xml --testsuite=database(8 tests)composer check-ci(full CI script: phplint, phpmd, phpcs, phpstan, license-headers, phpunit) — exit 0