[DoctrineBridge] Ulid and Uuid as Doctrine Types #37678

gennadigennadigennadi · 2020-07-27T12:31:48Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	#36102
License	MIT

This Types Implementations are basically copies from https://github.com/ramsey/uuid-doctrine, with minor tweaks to use Symfonys Uid classes.

I am not done yet. I'm going to implement some UnitTests and I also do need to add the Changelog Entries.

And I do ask my self if the Types should be registered automatically (with the recipe?), cause it would collide with Ramseys uuids implementation.

stof · 2020-07-27T13:22:22Z

Rather than adding a required dependency on symfony/uid in symfony/doctrine-bridge (which would then bring this package to everyone using the Doctrine Bridge for another reason), I think we should create a dedicated bridge package between doctrine/dbal and symfony/uid (and then, it becomes possible to have a smart default registering the type based on the fact that the package is installed, as there would be no reason to install that bridge without configuring it).

My own opinion (note that this has not been discussed yet with the other members of the core team to take a decision) is that we should not use symfony/doctrine-bridge to host more integrations. Making a bridge between all Symfony components and all Doctrine packages at once makes it harder to handle dependencies and makes it impossible to have smart defaults.

nicolas-grekas · 2020-07-29T14:29:18Z

Thanks for the PR. I didn't think about @stof's proposal but that shouldn't prevent moving forward on the rest. Can you please remove the declare statements (we don't use strict mode) and apply the fabbot patch?

nicolas-grekas

Some random thought for the discussion:

don't miss adding a changelog entry
what about adding implementations of AbstractIdGenerator?
do we want storage-optimized types? ie UuidAsBinaryType, UlidAsBinaryType and UlidAsUuidType?

src/Symfony/Bridge/Doctrine/Types/UlidType.php

nicolas-grekas · 2020-07-29T15:26:27Z

src/Symfony/Bridge/Doctrine/composer.json

@@ -22,6 +22,7 @@
        "symfony/polyfill-ctype": "~1.8",
        "symfony/polyfill-mbstring": "~1.0",
        "symfony/polyfill-php80": "^1.15",
+        "symfony/uid": "^5.1",


this should be added to require-dev
the constructors of the above classes could then do a runtime check + throw if the component is missing (see similar LogicException in the codebase when a dep is missing)

The Base Type class has a private construct https://github.com/doctrine/dbal/blob/e7c4149a13f1267380a8061452816a1a33909dc1/lib/Doctrine/DBAL/Types/Type.php#L134, therefor I can not do the runtime check you are suggesting.

I'm going to implement IdGenerators tomorrow.

well, if we go for my proposal of a separate package, the issue of the optional dependency is solved, btw.

nicolas-grekas · 2020-08-01T17:58:41Z

src/Symfony/Bridge/Doctrine/Id/UuidGenerator.php

+use Doctrine\ORM\Id\AbstractIdGenerator;
+use Symfony\Component\Uid\UuidV4;
+
+final class UuidGenerator extends AbstractIdGenerator


shouldn't we have UuidV4Generator but also UuidV1Generator and UuidV6Generator? (no V3 nor V5)

nicolas-grekas

The Base Type class has a private construct

OK, then we can add the check just before the class declaration

src/Symfony/Bridge/Doctrine/CHANGELOG.md

nicolas-grekas · 2020-08-01T18:00:23Z

src/Symfony/Bridge/Doctrine/CHANGELOG.md

+5.2.0
+-----
+
+ * added support for symfony/uid as types


I would suggest mentioning what this PR provides by giving the name of the new classes

nicolas-grekas · 2020-08-01T18:02:56Z

src/Symfony/Bridge/Doctrine/Types/UuidBinaryType.php

+    /**
+     * @throws ConversionException
+     */
+    public function convertToDatabaseValue($value, AbstractPlatform $platform): ?string


doesn't this deserve an AbstractBinaryUidType?

nicolas-grekas · 2020-08-01T18:09:51Z

I think we should create a dedicated bridge package between doctrine/dbal and symfony/uid (and then, it becomes possible to have a smart default registering the type based on the fact that the package is installed, as there would be no reason to install that bridge without configuring it).

Why can't we have a smart default without a dedicated bridge? Can't DoctrineBundle register the types when detecting that Uid is installed?

stof · 2020-08-03T12:15:47Z

hmm, for this particular case, we could indeed define a smart default based on 2 packages. But this does not remove the more complex management of dependencies (as we have to manage them through conflict rules rather than requirements).
And I think registering custom types adds some code running during the booting phase and/or the first instantiation of the connection service. So registering unused types might no be a good idea. Just having the uid component installed does not mean you want to use the new DBAL types for them. If they were in a dedicated package, that would be a much stronger signal (why installing a package providing the uid-based DBAL types if you don't want to register them ?)

ogizanagi · 2020-08-13T08:41:05Z

This does not solve any of @stof 's concerns, but we already have components with their own bridges to Doctrine shipped within it.
But if we mean to ask the user to require another dep for such a simple need, I'd rather go with @stof 's suggestion which indeed conveys more than the doctrine bridge itself.

One solution for now may be to ship this bridge into the component itself and split it later into a dedicated package if we want it to happen, as we did for Messenger transports?

gennadigennadigennadi · 2020-08-15T17:29:26Z

Todo:

Register Types if Uid Component is installed
Update Changelog

gennadigennadigennadi · 2020-08-16T17:53:14Z

I have added a CompilerPass to register the Types if no uuid or ulid is already registered. An alternative is to register them directly inside of the DoctrineExtension .

fabpot · 2020-08-22T06:42:52Z

@gennadigennadigennadi Can you finish the items on your todolist and move the PR to a non-draft status?

fabpot

@stof Can you review this one?

nicolas-grekas

Did you think about the DoctrineBundle's side?
What about opening a PR on it to integrate this work?

src/Symfony/Bridge/Doctrine/DependencyInjection/CompilerPass/RegisterUidTypePass.php

src/Symfony/Bridge/Doctrine/Types/AbstractBinaryUidType.php

src/Symfony/Bridge/Doctrine/Types/AbstractUidType.php

nicolas-grekas

Sorry for the double review, here is one more note.
Could you please add some tests maybe also?

src/Symfony/Bridge/Doctrine/Types/AbstractBinaryUidType.php

gennadigennadigennadi · 2020-09-02T18:20:53Z

Did you think about the DoctrineBundle's side?
What about opening a PR on it to integrate this work?

That's exactly what I was about to do as soon this PR gets accepted, but I can open a PR sooner if you like.

fabpot · 2020-09-03T06:30:27Z

src/Symfony/Bridge/Doctrine/composer.json

@@ -64,6 +65,7 @@
    },
    "suggest": {
        "symfony/form": "",
+        "symfony/uid": "",


This can be removed

nicolas-grekas · 2020-09-03T19:53:22Z

Thank you @gennadigennadigennadi.

maxhelias · 2020-10-05T16:03:17Z

src/Symfony/Bridge/Doctrine/Id/UlidGenerator.php

+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bridge\Doctrine\Types;


The namespace does not match the folder path. BTW, why not call it "Generator"?

can you send a PR please to fix the namespace?
and change the namespace to Uid meanwhile

why not call it "Generator"?

not sure what you mean here sorry...

why not call it "Generator"?

got it!
yes, let's name it Generator
I'm looking forward to your PR :)
If you can add tests, that'd be awesome.

IdGenerator actually.

Sure, it was just not very clear to have the namespace "Id" but "Uid" looks better to me

The namespace does not match the folder path. BTW, why not call it "Generator"?

Good catch.
I just haven't spent enough time with implementing those Generators.

Thx.

@javiereguiluz

This PR was merged into the 5.2-dev branch. Discussion ---------- [DoctrineBridge] fix and replace namespace to Uid | Q | A | ------------- | --- | Branch? | master  | Bug fix? | yes | New feature? | no  | Deprecations? | no  | Tickets | #37678 (comment)  | License | MIT | Doc PR | ...  This post should also be corrected: https://symfony.com/blog/new-in-symfony-5-2-doctrine-types-for-uuid-and-ulid cc @javiereguiluz Commits ------- 28d1169 [DoctrineBridge] fix and replace namespace to Uid

carsonbot added Status: Needs Review Feature labels Jul 27, 2020

nicolas-grekas added this to the next milestone Jul 29, 2020

gennadigennadigennadi force-pushed the feature/add-uid-as-doctrine-types branch from d3af2ba to d0b62b4 Compare July 29, 2020 15:06

nicolas-grekas reviewed Jul 29, 2020

View reviewed changes

gennadigennadigennadi force-pushed the feature/add-uid-as-doctrine-types branch 2 times, most recently from a77cbed to 0e9424c Compare July 29, 2020 16:49

nicolas-grekas reviewed Aug 1, 2020

View reviewed changes

nicolas-grekas changed the title ~~Add: Ulid and Uuid as Doctrine Types~~ [DoctrineBridge] Ulid and Uuid as Doctrine Types Aug 1, 2020

nicolas-grekas reviewed Aug 1, 2020

View reviewed changes

gennadigennadigennadi force-pushed the feature/add-uid-as-doctrine-types branch from 77d8adf to ffc49ae Compare August 16, 2020 17:45

gennadigennadigennadi marked this pull request as ready for review August 23, 2020 15:42

fabpot approved these changes Aug 23, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Aug 23, 2020

nicolas-grekas reviewed Aug 26, 2020

View reviewed changes

src/Symfony/Bridge/Doctrine/DependencyInjection/CompilerPass/RegisterUidTypePass.php Show resolved Hide resolved

nicolas-grekas reviewed Aug 26, 2020

View reviewed changes

src/Symfony/Bridge/Doctrine/Types/AbstractBinaryUidType.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Aug 26, 2020

View reviewed changes

src/Symfony/Bridge/Doctrine/Types/AbstractUidType.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Aug 26, 2020

View reviewed changes

src/Symfony/Bridge/Doctrine/Types/AbstractBinaryUidType.php Outdated Show resolved Hide resolved

gennadigennadigennadi force-pushed the feature/add-uid-as-doctrine-types branch from 457f451 to 95c9daf Compare September 2, 2020 19:26

gennadigennadigennadi requested a review from nicolas-grekas September 2, 2020 20:27

carsonbot added Status: Needs Review and removed Status: Reviewed labels Sep 2, 2020

fabpot approved these changes Sep 3, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Sep 3, 2020

nicolas-grekas force-pushed the feature/add-uid-as-doctrine-types branch from c3e5f6f to ca46e63 Compare September 3, 2020 19:41

[DoctrineBridge] Ulid and Uuid as Doctrine Types

f44fa34

nicolas-grekas force-pushed the feature/add-uid-as-doctrine-types branch from ca46e63 to f44fa34 Compare September 3, 2020 19:52

nicolas-grekas approved these changes Sep 3, 2020

View reviewed changes

nicolas-grekas merged commit 174d91c into symfony:master Sep 3, 2020

This was referenced Sep 3, 2020

[Uid] integration with the other components #36102

Closed

[DoctrineBridge] Ulid and Uuid as Doctrine Types symfony/symfony-docs#14167

Closed

gennadigennadigennadi mentioned this pull request Sep 4, 2020

Add symfony uuid to a list of custom types as default uuid/ulid type doctrine/DoctrineBundle#1207

Merged

nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020

fabpot mentioned this pull request Oct 5, 2020

Release v5.2.0-BETA1 #38417

Merged

maxhelias reviewed Oct 5, 2020

View reviewed changes

maxhelias mentioned this pull request Oct 5, 2020

[DoctrineBridge] fix and replace namespace to Uid #38419

Merged

secit-pl mentioned this pull request Oct 21, 2020

Doctrine UUID/ULID types not recognized #38658

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DoctrineBridge] Ulid and Uuid as Doctrine Types #37678

[DoctrineBridge] Ulid and Uuid as Doctrine Types #37678

gennadigennadigennadi commented Jul 27, 2020

stof commented Jul 27, 2020

nicolas-grekas commented Jul 29, 2020

nicolas-grekas left a comment

nicolas-grekas Jul 29, 2020

gennadigennadigennadi Jul 29, 2020

stof Jul 29, 2020

nicolas-grekas Aug 1, 2020

nicolas-grekas left a comment

nicolas-grekas Aug 1, 2020

nicolas-grekas Aug 1, 2020

nicolas-grekas commented Aug 1, 2020

stof commented Aug 3, 2020

ogizanagi commented Aug 13, 2020

gennadigennadigennadi commented Aug 15, 2020 •

edited

Loading

gennadigennadigennadi commented Aug 16, 2020

fabpot commented Aug 22, 2020

fabpot left a comment

nicolas-grekas left a comment

nicolas-grekas left a comment

gennadigennadigennadi commented Sep 2, 2020

fabpot Sep 3, 2020

nicolas-grekas commented Sep 3, 2020

maxhelias Oct 5, 2020

nicolas-grekas Oct 5, 2020

nicolas-grekas Oct 5, 2020

nicolas-grekas Oct 5, 2020

maxhelias Oct 5, 2020

gennadigennadigennadi Oct 5, 2020

[DoctrineBridge] Ulid and Uuid as Doctrine Types #37678

[DoctrineBridge] Ulid and Uuid as Doctrine Types #37678

Conversation

gennadigennadigennadi commented Jul 27, 2020

stof commented Jul 27, 2020

nicolas-grekas commented Jul 29, 2020

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Aug 1, 2020

stof commented Aug 3, 2020

ogizanagi commented Aug 13, 2020

gennadigennadigennadi commented Aug 15, 2020 • edited Loading

gennadigennadigennadi commented Aug 16, 2020

fabpot commented Aug 22, 2020

fabpot left a comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

gennadigennadigennadi commented Sep 2, 2020

Choose a reason for hiding this comment

nicolas-grekas commented Sep 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gennadigennadigennadi commented Aug 15, 2020 •

edited

Loading