feat: allow doctrine/dbal v4

kochen commented 7 months ago

Description

Support doctrine/dbal:^4.0

Closes: https://github.com/ramsey/uuid-doctrine/issues/246

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

PR checklist

[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I have read the CONTRIBUTING.md document.
[ ] I have added tests to cover my changes.

kochen commented 6 months ago

@ramsey could you please take a look?

greg0ire commented 5 months ago

@kochen I've created https://github.com/ramsey/uuid-doctrine/pull/253 to show how it could be possible to avoid forcing users to upgrade both packages at once. Feel free to incorporate the changes into your PR.

kochen commented 5 months ago

Thanks @greg0ire - on it...

kochen commented 5 months ago

@greg0ire @ramsey now supporting both dbal v3 (& v2.8) as well as v4!

kochen commented 5 months ago

Hi @greg0ire, I tried reverting each of the changes I made and they all seem to be required for dbal v4.

Do you have one specific suspect you could point out?

tasselchof commented 4 months ago

What need to be done here to get this pull request approved @greg0ire @ramsey?

greg0ire commented 4 months ago

I don't know, I'm not a maintainer of this repository

tasselchof commented 4 months ago

I don't know, I'm not a maintainer of this repository

Oh, I didn't realize you were just helping with this commit. Let's wait for the maintainer.

greg0ire commented 4 months ago

I'm a Doctrine maintainer, and I want DBAL 4 to get more adoption, hence my review :)

tasselchof commented 4 months ago

@greg0ire I think this library will need more work. With this last merge I had a problems, probably because of this: https://github.com/doctrine/dbal/blob/4.0.x/UPGRADE.md#bc-break-changes-to-handling-binary-fields. At this point I am getting decoded string: https://github.com/ramsey/uuid-doctrine/blob/main/src/UuidBinaryOrderedTimeType.php#L72.

I am not sure how this should work with dbal version 4, can you please clarify?

greg0ire commented 4 months ago

Clarify what? I didn't understand your issue, sorry.

tasselchof commented 4 months ago

Clarify what? I didn't understand your issue, sorry.

In latest version of DBAL there is a change in handling of the blob fields from database (in my case it's bytea type from PostgreSQL).

So now when I use types from this package I am getting this (with this patch merged):

Doctrine\ORM\ORMInvalidArgumentException raised in file /home/dev/domains/api-dev/vendor/doctrine/orm/src/ORMInvalidArgumentException.php line 44:
Message: The given entity of type 'Api\User\Entity\User' (Api\User\Entity\User@1415) has no identity/no id values set. It cannot be added to the identity map.

This is because of this code: https://github.com/ramsey/uuid-doctrine/blob/72ac784f1060694f0a412ea9b65a3b30fe79ec26/src/UuidBinaryOrderedTimeType.php#L72-L74

I am getting stream in uuid and it's not string so it's being nulled.

greg0ire commented 4 months ago

I am getting stream in uuid

The upgrade guide you mentioned says: "Binary fields are no longer represented as streams in PHP. They are represented as strings." so now I'm confused as to why you are getting a stream.

tasselchof commented 4 months ago

I am getting stream in uuid

The upgrade guide you mentioned says: "Binary fields are no longer represented as streams in PHP. They are represented as strings." so now I'm confused as to why you are getting a stream.

This is why I asked, package versions are looking good to me:

$ composer show | grep doctrine/*
Composer could not detect the root package () version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
doctrine/annotations                           2.0.1              Docblock Annotations Parser
doctrine/collections                           2.2.1              PHP Doctrine Collections library that adds additional functionality on top of PHP arrays.
doctrine/common                                3.4.4              PHP Doctrine Common project is a library that provides additional functionality that other Doctr...
doctrine/data-fixtures                         1.7.0              Data Fixtures for all Doctrine Object Managers
doctrine/dbal                                  4.0.1              Powerful PHP database abstraction layer (DBAL) with many features for database schema introspect...
doctrine/deprecations                          1.1.3              A small layer on top of trigger_error(E_USER_DEPRECATED) or PSR-3 logging with options to disabl...
doctrine/event-manager                         2.0.0              The Doctrine Event Manager is a simple PHP event system that was built to be used with the vario...
doctrine/inflector                             2.0.10             PHP Doctrine Inflector is a small library that can perform string manipulations with regard to u...
doctrine/instantiator                          2.0.0              A small, lightweight utility to instantiate objects in PHP without invoking their constructors
doctrine/lexer                                 3.0.1              PHP Doctrine Lexer parser library that can be used in Top-Down, Recursive Descent Parsers.
doctrine/migrations                            3.7.4              PHP Doctrine Migrations project offer additional functionality on top of the database abstractio...
doctrine/orm                                   3.1.2              Object-Relational-Mapper for PHP
doctrine/persistence                           3.3.2              The Doctrine Persistence project is a set of shared interfaces and functionality that the differ...
dotkernel/dot-data-fixtures                    1.1.3              Provides a CLI interface for listing & executing doctrine data fixtures
dotkernel/dot-doctrine-metadata                3.2.2              DotKernel wrapper that sits on top of mezzio-hal package and resolves the doctrine proxy issues.
ramsey/uuid-doctrine                           dev-main e91146a   Use ramsey/uuid as a Doctrine field type.
roave/psr-container-doctrine                   5.1.0              Doctrine Factories for PSR-11 Containers

convertToPhpValue here https://github.com/doctrine/orm/blob/f79d166a4e844beb9389f23bdb44abdbf58cec38/src/Internal/Hydration/AbstractHydrator.php#L320 called with stream as value, not string as per upgrade guide.

For this uuid package I just added:

$r = stream_get_contents($value);

Which is returning me string b6606896-3a24-11ee-96a0-db08812a60d3. This is also not good by the way, because later this library is attempting to decode string and exception here: https://github.com/kochen/uuid-doctrine/blob/b149bea434024b03f3e76681b27891d9a03a492d/src/UuidBinaryOrderedTimeType.php#L80-L86

Does it makes sense to create an issue in doctrine/orm? @greg0ire maybe you have an idea what might be the reason?

greg0ire commented 4 months ago

I don't know, I still don't understand.

convertToPhpValue here doctrine/orm@f79d166/src/Internal/Hydration/AbstractHydrator.php#L320 called with stream as value, not string as per upgrade guide.

For instance here, you sentence is not syntactically valid, you're not quoting what part of the upgrade guide is relevant to that line, or why this is important, or even why you know that method is called with a stream.

For this uuid package I just added:

Great, but where did you add this? I have no idea…

tasselchof commented 4 months ago

@greg0ire I am sorry for that - seems like I am just sharing my thoughts that looks random to you.

Let me start from the beginning:

I've merged this pull request to be able to use doctrine/dbal v4, I have an entity where I have this annotation for an id property:

#[ORM\Id]
#[ORM\Column(name: 'uuid', type: 'uuid_binary_ordered_time', unique: true)]
#[ORM\GeneratedValue(strategy: 'CUSTOM')]
#[ORM\CustomIdGenerator(class: \Ramsey\Uuid\Doctrine\UuidOrderedTimeGenerator::class)]
protected ?UuidInterface $uuid;

And I started to get an exception which I've mentioned before:

Doctrine\ORM\ORMInvalidArgumentException raised in file /home/dev/domains/api-dev/vendor/doctrine/orm/src/ORMInvalidArgumentException.php line 44:
Message: The given entity of type 'Api\User\Entity\User' (Api\User\Entity\User@1415) has no identity/no id values set. It cannot be added to the identity map.

So in the process of figuring out what is happened I've got into this inconsistency. An upgrade guide is mentioning that "Binary fields are no longer represented as streams in PHP. They are represented as strings.". Yet I am getting resource (stream) in the type method that is responsible for converting database value to php value (convertToPhpValue).

I've checked packages versions, I am using the correct ones:

$ composer show | grep doctrine/*
Composer could not detect the root package () version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
doctrine/annotations                           2.0.1              Docblock Annotations Parser
doctrine/collections                           2.2.1              PHP Doctrine Collections library that adds additional functionality on top of PHP arrays.
doctrine/common                                3.4.4              PHP Doctrine Common project is a library that provides additional functionality that other Doctr...
doctrine/data-fixtures                         1.7.0              Data Fixtures for all Doctrine Object Managers
doctrine/dbal                                  4.0.1              Powerful PHP database abstraction layer (DBAL) with many features for database schema introspect...
doctrine/deprecations                          1.1.3              A small layer on top of trigger_error(E_USER_DEPRECATED) or PSR-3 logging with options to disabl...
doctrine/event-manager                         2.0.0              The Doctrine Event Manager is a simple PHP event system that was built to be used with the vario...
doctrine/inflector                             2.0.10             PHP Doctrine Inflector is a small library that can perform string manipulations with regard to u...
doctrine/instantiator                          2.0.0              A small, lightweight utility to instantiate objects in PHP without invoking their constructors
doctrine/lexer                                 3.0.1              PHP Doctrine Lexer parser library that can be used in Top-Down, Recursive Descent Parsers.
doctrine/migrations                            3.7.4              PHP Doctrine Migrations project offer additional functionality on top of the database abstractio...
doctrine/orm                                   3.1.2              Object-Relational-Mapper for PHP
doctrine/persistence                           3.3.2              The Doctrine Persistence project is a set of shared interfaces and functionality that the differ...
dotkernel/dot-data-fixtures                    1.1.3              Provides a CLI interface for listing & executing doctrine data fixtures
dotkernel/dot-doctrine-metadata                3.2.2              DotKernel wrapper that sits on top of mezzio-hal package and resolves the doctrine proxy issues.
ramsey/uuid-doctrine                           dev-main e91146a   Use ramsey/uuid as a Doctrine field type.
roave/psr-container-doctrine                   5.1.0              Doctrine Factories for PSR-11 Containers

Does this makes more sense to you?

greg0ire commented 4 months ago

Thanks for taking a step back, things are starting to get clearer.

Yet I am getting resource (stream) in the type method that is responsible for converting database value to php value (convertToPhpValue).

Where exactly in the method? Looking at https://github.com/doctrine/dbal/commit/9c36d219140bbbb1e57466f945037ef4fc35f09c, I'd say it's fine to see a stream inside the method, so long as a string is always returned by that method in the end. Looking at the code, I'd say that's always the case.

tasselchof commented 4 months ago

Thanks for taking a step back, things are starting to get clearer.

Yet I am getting resource (stream) in the type method that is responsible for converting database value to php value (convertToPhpValue).

Where exactly in the method? Looking at doctrine/dbal@9c36d21, I'd say it's fine to see a stream inside the method, so long as a string is always returned by that method in the end. Looking at the code, I'd say that's always the case.

I am referring to the method of a custom type that converts a database value to a PHP value. Specifically, I'm investigating why, in my case, it's converting the database value to null.

Judging by this assumption, this pull request is not really doing what it's meant to be doing. Specifically, in that library, at this link:

https://github.com/kochen/uuid-doctrine/blob/b149bea434024b03f3e76681b27891d9a03a492d/src/UuidBinaryOrderedTimeType.php#L70-L87

The function convertToPhp of the type is checking if a string was passed as a value, but it will always be false as a resource is returned.

Am I correct?

greg0ire commented 4 months ago

Ok so to sum up: with DBAL 3, the ORM used to pass a string to convertToPHPValue, with DBAL 4, it passes a stream? I think that's what you need to investigate, and I think the upgrade guide section you referred me to is more about a change in the output of convertToPHPValue than it is about its input (but maybe I'm wrong and the change occurs because of the change to AbstractPlatform::getBinaryTypeDeclarationSQL()

tasselchof commented 4 months ago

I am not sure what ORM used to pass before because I never used this package. I assume that it was a sting as I see it in test:

https://github.com/kochen/uuid-doctrine/blob/b149bea434024b03f3e76681b27891d9a03a492d/tests/UuidBinaryOrderedTimeTypeTest.php#L92-L97

So I guess to add support of dbal 4.0 this should be rebuilt as well.

tasselchof commented 4 months ago

Update: seems like it's mentioned issue and it's valid for PostgreSQL platform: https://github.com/ramsey/uuid-doctrine/issues/225. Just because I used dbal 4 for new project I thought it's related to this update.

@greg0ire isn't it doctrine thing, that it is handled differently for PostgreSQL platform?

There is a pull request that is fixing it (same as I fixed it for myself): https://github.com/ramsey/uuid-doctrine/pull/229.

greg0ire commented 4 months ago

Ok, so this has nothing to do with DBAL v4 support, right?

tasselchof commented 4 months ago

I guess it's not related to the new version of dbal, just a coincidence for me.

greg0ire commented 4 months ago

Ok, then I will hide all my messages, and I suggest you do the same.

ramsey commented 4 months ago

This looks good, but it's failing the static analysis checks, and I don't have time to investigate. Can you look into it?

kochen commented 4 months ago

This looks good, but it's failing the static analysis checks, and I don't have time to investigate. Can you look into it?

@ramsey all the issues are related to the unsupported PHP 7.4 Is there any plan to drop support for it? doctrine/dbal:4 expects PHP > 8.1 anyway...

kochen commented 4 months ago

@ramsey could you approve the workflow again?

ToshY commented 3 months ago

@ramsey Could you spare some time to take a look at this? Thanks in advance.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 95.45455% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 90.25%. Comparing base (ee0b1ef) to head (6b9d3d6). Report is 17 commits behind head on main.

Additional details and impacted files

[![Impacted file tree graph](https://app.codecov.io/gh/ramsey/uuid-doctrine/pull/247/graphs/tree.svg?width=650&height=150&src=pr&token=FVm8VB6YS1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ben+Ramsey)](https://app.codecov.io/gh/ramsey/uuid-doctrine/pull/247?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ben+Ramsey) ```diff @@ Coverage Diff @@ ## main #247 +/- ## ============================================ - Coverage 99.27% 90.25% -9.02% - Complexity 68 75 +7 ============================================ Files 6 7 +1 Lines 138 154 +16 ============================================ + Hits 137 139 +2 - Misses 1 15 +14 ``` | [Files](https://app.codecov.io/gh/ramsey/uuid-doctrine/pull/247?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ben+Ramsey) | Coverage Δ | | |---|---|---| | [src/UuidBinaryOrderedTimeType.php](https://app.codecov.io/gh/ramsey/uuid-doctrine/pull/247?src=pr&el=tree&filepath=src%2FUuidBinaryOrderedTimeType.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ben+Ramsey#diff-c3JjL1V1aWRCaW5hcnlPcmRlcmVkVGltZVR5cGUucGhw) | `94.28% <100.00%> (-5.72%)` | :arrow_down: | | [src/UuidBinaryType.php](https://app.codecov.io/gh/ramsey/uuid-doctrine/pull/247?src=pr&el=tree&filepath=src%2FUuidBinaryType.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ben+Ramsey#diff-c3JjL1V1aWRCaW5hcnlUeXBlLnBocA==) | `87.09% <100.00%> (-12.91%)` | :arrow_down: | | [src/UuidType.php](https://app.codecov.io/gh/ramsey/uuid-doctrine/pull/247?src=pr&el=tree&filepath=src%2FUuidType.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ben+Ramsey#diff-c3JjL1V1aWRUeXBlLnBocA==) | `85.71% <100.00%> (-14.29%)` | :arrow_down: | | [src/UuidV7Generator.php](https://app.codecov.io/gh/ramsey/uuid-doctrine/pull/247?src=pr&el=tree&filepath=src%2FUuidV7Generator.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ben+Ramsey#diff-c3JjL1V1aWRWN0dlbmVyYXRvci5waHA=) | `85.71% <ø> (ø)` | | | [src/GetBindingTypeImplementation.php](https://app.codecov.io/gh/ramsey/uuid-doctrine/pull/247?src=pr&el=tree&filepath=src%2FGetBindingTypeImplementation.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ben+Ramsey#diff-c3JjL0dldEJpbmRpbmdUeXBlSW1wbGVtZW50YXRpb24ucGhw) | `50.00% <50.00%> (ø)` | | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/ramsey/uuid-doctrine/pull/247/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ben+Ramsey)

ramsey / uuid-doctrine