doctrine-extensions / DoctrineExtensions

Doctrine2 behavioral extensions, Translatable, Sluggable, Tree-NestedSet, Timestampable, Loggable, Sortable
MIT License
4.04k stars 1.27k forks source link

ORM 3.0 compat #2708

Closed mbabker closed 4 months ago

mbabker commented 1 year ago

The ORM now has a 3.0 beta release so it's probably time to start looking at that upgrade guide.

mbabker commented 1 year ago

There are a few things that are coming up fairly regularly:

/vendor/symfony/phpunit-bridge/DeprecationErrorHandler.php:132
/src/Tree/Strategy/ORM/Closure.php:462
/src/Tree/Strategy/ORM/Closure.php:286
/src/Tree/TreeListener.php:247

And this is on top of the removal of the annotation and YAML mapping drivers, which a lot of the tests use.

mbabker commented 12 months ago

https://github.com/doctrine-extensions/DoctrineExtensions/compare/main...mbabker:DoctrineExtensions:orm-3-merge has a branch with a lot of small things chipped away at. A few of the commits are merges from other PRs, a few are new things tackled today which can be extracted out later.

Big things listed earlier still apply for the most part (more may be hidden since a fair number of tests are currently skipped on that branch as they rely on annotation and YAML mapping support), as well as the XML validation issue from #2613 now being a bigger thing.

mbabker commented 11 months ago

In that catch-all branch, I've got the Indirect modification of overloaded element error taken care of now. So really it's just down to three huge focus areas:

mbabker commented 9 months ago

With the tests run against the ORM 3.0 RC, we're down to:

1) Gedmo\Tests\Sluggable\Issue\Issue104Test::testShouldThrowAnExceptionWhenMappedSuperclassProtectedProperty
Failed asserting that exception of type "Doctrine\ORM\Mapping\MappingException" matches expected exception "Gedmo\Exception\InvalidMappingException". Message was: "Duplicate definition of column 'title' on entity 'Gedmo\Tests\Sluggable\Fixture\Issue104\Car' in a field or discriminator column mapping." at
vendor/doctrine/orm/src/Mapping/MappingException.php:420
vendor/doctrine/orm/src/Mapping/ClassMetadata.php:1191
vendor/doctrine/orm/src/Mapping/ClassMetadata.php:1882
vendor/doctrine/orm/src/Mapping/Driver/AttributeDriver.php:334
vendor/doctrine/orm/src/Mapping/ClassMetadataFactory.php:163
vendor/doctrine/persistence/src/Persistence/Mapping/AbstractClassMetadataFactory.php:343
vendor/doctrine/persistence/src/Persistence/Mapping/AbstractClassMetadataFactory.php:225
vendor/doctrine/orm/src/EntityManager.php:216
tests/Gedmo/Tool/BaseTestCaseORM.php:66
tests/Gedmo/Tool/BaseTestCaseORM.php:66
tests/Gedmo/Sluggable/Issue/Issue104Test.php:34
mbabker commented 9 months ago

We've got the stable release out, now. I truthfully don't know what to do with the errors around Doctrine\ORM\UnitOfWork::clearEntityChangeSet() being gone. For reference, these are the last few frames in each stack trace (basically at the point the ORM emits an event and execution comes into this package's listeners) where it's an issue:

src/Mapping/Event/Adapter/ORM.php:179
src/Translatable/TranslatableListener.php:756
src/Translatable/TranslatableListener.php:432
src/Tree/Strategy/ORM/Nested.php:161
src/Tree/TreeListener.php:165

For the nested tree strategy, it uses the clearEntityChangeSet() call after https://github.com/doctrine-extensions/DoctrineExtensions/commit/9a072a911105bf28aaeab3a9a6d9166e9cdb1359. And if I traced the translatable listener right, https://github.com/doctrine-extensions/DoctrineExtensions/commit/5f75c7ddb76d934d05c22a14be866ce553d596f2 is when that call was introduced. So these are some rather old behaviors at this point that need revisiting in some way.

andreybolonin commented 9 months ago

I am using TimestampableEntity, and update from ORM v2 to v3:


[Semantical Error] The class "Doctrine\ORM\Mapping\Column" is not annotated with @Annotation.                                                              
  Are you sure this class can be used as annotation?                                                                                                         
  If so, then you need to add @Annotation to the _class_ doc comment of "Doctrine\ORM\Mapping\Column".                                                       
  If it is indeed no annotation, then you need to add @IgnoreAnnotation("ORM\Column") to the _class_ doc comment of property App\Entity\Funnel::$createdAt
mbabker commented 9 months ago

ORM 3.0 does not support annotations anymore, you will need to update your mapping to use attributes.

franmomu commented 9 months ago

We've got the stable release out, now. I truthfully don't know what to do with the errors around Doctrine\ORM\UnitOfWork::clearEntityChangeSet() being gone. For reference, these are the last few frames in each stack trace (basically at the point the ORM emits an event and execution comes into this package's listeners) where it's an issue:

src/Mapping/Event/Adapter/ORM.php:179
src/Translatable/TranslatableListener.php:756
src/Translatable/TranslatableListener.php:432
src/Tree/Strategy/ORM/Nested.php:161
src/Tree/TreeListener.php:165

For the nested tree strategy, it uses the clearEntityChangeSet() call after 9a072a9. And if I traced the translatable listener right, 5f75c7d is when that call was introduced. So these are some rather old behaviors at this point that need revisiting in some way.

I've tried to take a look at the usage in the Translatable extension, but I couldn't make it work without it, I'll give it a try again, but in the meantime if we just want to skip this, I guess we can use somehow UnitOfWork::getEntityChangset() which returns a reference and modify the returned value.

mbabker commented 9 months ago

I guess we can use somehow UnitOfWork::getEntityChangset() which returns a reference and modify the returned value.

For the sake of moving things forward, I've just replaced the clearObjectChangeSet() implementation in the ORM event adapter with reflection. It's ugly but gets the job done for now.

The 57 errors around the removal of Doctrine\ORM\UnitOfWork::clearEntityChangeSet() are gone, and in its place are 20 new type errors:

TypeError: Doctrine\DBAL\Types\StringType::getSQLDeclaration(): Argument #1 ($column) must be of type array, Doctrine\ORM\Mapping\FieldMapping given, called in src/Translatable/Query/TreeWalker/TranslationWalker.php on line 324

vendor/doctrine/dbal/src/Types/StringType.php:15
src/Translatable/Query/TreeWalker/TranslationWalker.php:324
src/Translatable/Query/TreeWalker/TranslationWalker.php:129
vendor/doctrine/orm/src/Query/Parser.php:342
vendor/doctrine/orm/src/Query.php:216
vendor/doctrine/orm/src/Query.php:245
[...]

This one would be from https://github.com/doctrine/orm/pull/10607 with the ORM in 3.0 using more DTOs and less arrays. Except, in this case, the mapping info can no longer be passed directly into the DBAL since Doctrine\DBAL\Types\Type::getSQLDeclaration() typehints the $column argument as an array. Adding an inline typecast in the translation walker fixes the issue and we're down to only the entity collision and mapping exceptions noted in https://github.com/doctrine-extensions/DoctrineExtensions/issues/2708#issuecomment-1913708430, but it looks like the ORM might also benefit from an explicit toArray() method on this DTO to help explicitly support passing the computed field mapping between the ORM and DBAL.

franmomu commented 9 months ago

but it looks like the ORM might also benefit from an explicit toArray() method on this DTO to help explicitly support passing the computed field mapping between the ORM and DBAL.

I guess casting the object to array (array) $fieldMapping would work

mbabker commented 9 months ago

Added the typecast to my branch, then skipped the test where the mapping exceptions are different since there looks to be a change between 2.x and 3.x and the test won't fail inside our code anymore.

That just leaves one erroring test on the branch, then working out porting it all over in a B/C manner (since I've only been running the tests locally on that branch with ORM 3.0 installed):

1) Gedmo\Tests\Sluggable\SluggableIdentifierTest::testShouldPersistMultipleNonConflictingIdentifierSlugs
   Doctrine\ORM\Exception\EntityIdentityCollisionException: While adding an entity of class Gedmo\Tests\Sluggable\Fixture\Identifier with an ID hash of "__id__" to the identity map,
   another object of class Gedmo\Tests\Sluggable\Fixture\Identifier was already present for the same ID. This exception
   is a safeguard against an internal inconsistency - IDs should uniquely map to
   entity object instances. This problem may occur if:

- you use application-provided IDs and reuse ID values;
- database-provided IDs are reassigned after truncating the database without
  clearing the EntityManager;
- you might have been using EntityManager#getReference() to create a reference
  for a nonexistent ID that was subsequently (by the RDBMS) assigned to another
  entity.

Otherwise, it might be an ORM-internal inconsistency, please report it.

vendor/doctrine/orm/src/Exception/EntityIdentityCollisionException.php:15
vendor/doctrine/orm/src/UnitOfWork.php:1540
vendor/doctrine/orm/src/UnitOfWork.php:1386
vendor/doctrine/orm/src/UnitOfWork.php:932
vendor/doctrine/orm/src/UnitOfWork.php:1805
vendor/doctrine/orm/src/UnitOfWork.php:1763
vendor/doctrine/orm/src/EntityManager.php:435
tests/Gedmo/Sluggable/SluggableIdentifierTest.php:56

This one's the fun one. The sluggable listener's prePersist hook will set __id__ as the value for an identifier field, then in its onFlush hook it handles setting the generated slug, see https://github.com/doctrine-extensions/DoctrineExtensions/commit/d40b449d6a4271ac9607fffbdf5929c3538b0f2d and https://github.com/doctrine-extensions/DoctrineExtensions/commit/4f69bdd856e03eccd5bba68c1a96db6ea3574b3b for reference. Looking at it today and not knowing that was a feature, my first thought is "why wasn't this done as an ID generator instead?", but making that shift now is a bit of a B/C break. The next best idea I can think of is to generate unique values (i.e. '__sluggable_placeholder_' . random_int(1, PHP_INT_MAX) . '__' instead of the fixed __id__ then the checks change to something like str_starts_with('__sluggable_placeholder_').

Maclay74 commented 8 months ago

ORM 3.0 does not support annotations anymore, you will need to update your mapping to use attributes.

@mbabker could you please elaborate on that? I have the same problem with Translatable:

[Semantical Error] The class "Doctrine\ORM\Mapping\MappedSuperclass" is not annotated with @Annotation.
Are you sure this class can be used as annotation?
If so, then you need to add @Annotation to the _class_ doc comment of "Doctrine\ORM\Mapping\MappedSuperclass".
If it is indeed no annotation, then you need to add @IgnoreAnnotation("ORM\MappedSuperclass") to the _class_ doc comment of class Gedmo\Translatable\Entity\MappedSuperclass\AbstractTranslation.

I use Symfony 6.4 with attributes.

Thanks.

mbabker commented 8 months ago

It is just as I said, ORM 3.0 removed support for annotations. Compare the MappedSuperclass class in ORM 2.19.0 with the version in ORM 3.1.0 and you'll see that all of the doc blocks configuring the class as an annotation class have been removed, hence the The class "Doctrine\ORM\Mapping\MappedSuperclass" is not annotated with @Annotation. opening to that error message.

As this library does not yet support ORM 3.0, you will need to downgrade to ORM 2.x until compatibility is finished.

Chris53897 commented 8 months ago

@mbabker Thanks for all your work to support orm v3. Is there a TODO-List to follow? What can we do to help?

I guess it is one part to allow fresh installs with support of orm v3. And another to migrate existing projects. I know that there was a issue here, that had some parts of the solution to migrate data.

mbabker commented 8 months ago

@mbabker Thanks for all your work to support orm v3.

Is there a TODO-List to follow?

What can we do to help?

Test pull requests, test the branch I linked earlier that has the raw changes I've done to get the test suite to pass with only ORM 3 running. A lot of the needed code changes are there now, it's mostly just working back through them to apply them in a B/C friendly way.

I guess it is one part to allow fresh installs with support of orm v3.

And another to migrate existing projects. I know that there was a issue here, that had some parts of the solution to migrate data.

That's https://github.com/doctrine-extensions/DoctrineExtensions/issues/2502 and it's a DBAL 4.0 blocker.

klemens-u commented 6 months ago

As this library does not yet support ORM 3.0, you will need to downgrade to ORM 2.x until compatibility is finished.

For a fresh Symfony7 project this way of downgrading worked for me: composer.json

{
    "require": {
        ...      
        "doctrine/dbal": "^3",
        "doctrine/doctrine-bundle": "^2.5",        
        "doctrine/orm": "^2.5",

Then run composer update

elementaire commented 5 months ago

For a fresh Symfony7, downgrade also: composer.json

{
    "require": {
        ... 
        "spiriitlabs/form-filter-bundle": "^10",
franmomu commented 5 months ago

@mbabker did a great job adding support for ORM 3 ❤️ and now it's merged in the main branch, please give a try before creating a release.

cameronmurphy commented 5 months ago

Working nicely 👍

Chris53897 commented 5 months ago

I just use Blameable and Logable. This works fine. Helper to test it, if you use stof/doctrine-extensions-bundle you need to add this to composer.json

"gedmo/doctrine-extensions": "dev-main as 3.16",

vencakrecl commented 4 months ago

I can confirm that SoftDeleteable works for entities and documents.

dmaicher commented 4 months ago

I can confirm Timestampable works fine for me with dev-main and ORM 3

franmomu commented 4 months ago

Closing here since there is a new release https://github.com/doctrine-extensions/DoctrineExtensions/releases/tag/v3.16.0

thanks everybody for trying and thanks @mbabker again!

alcohol commented 4 months ago

I am confused, the errors regarding annotations are still present with the release of v3.16.0.

In AnnotationException.php line 36:

  [Semantical Error] The class "Doctrine\ORM\Mapping\Column" is not annotated with @Annotation.
  Are you sure this class can be used as annotation?
  If so, then you need to add @Annotation to the _class_ doc comment of "Doctrine\ORM\Mapping\Column".
  If it is indeed no annotation, then you need to add @IgnoreAnnotation("ORM\Column") to the _class_ doc comment of property MyEntity::$createdAt.

Those annotations are in the traits provided by gedmo/doctrine-extensions. Why mark the release as compatible with ORM v3 when evidence seems to suggest otherwise?

7system7 commented 4 months ago

@alcohol I had the same issue. Remove all doctrine/* packages and reinstall them w/ composer $ composer require symfony/orm-pack has been solved for me.

alcohol commented 4 months ago

Why would I do that? My composer dependencies are not the issue here.

7system7 commented 4 months ago

Then, don't even do what I wrote.

alcohol commented 4 months ago

Adding @Doctrine\Common\Annotations\Annotation\IgnoreAnnotation("ORM\Column") to all our entity classes solved this for now. Though I wonder/imagine if it would be possible at all to just tell doctrine to not use or read annotations (since we use attributes anyway). I'll have to dive deeper into the configuration options that the symfony framework bundle provides and whether or not they have anything that maps to an internal configuration parameter (should one exist).

mbabker commented 4 months ago

You can add this line somewhere in your bootstrapping code, too, instead of needing to add an ignore annotation in every file:

AnnotationReader::addGlobalIgnoredNamespace('Doctrine\ORM\Mapping');

It's needed in the test bootstrap for this package, too.

Without dropping annotations mapping support entirely, there isn't a clean way to get around the need for that line if your application is using ORM 3 and has the Annotations library installed. For B/C, the feature detection code inside this library will try to use annotations when no explicit reader (either the Doctrine annotation reader or this package's attribute reader) is set and the Annotations library is installed. I know there's a recent PR in the bundle to inject the attribute reader in some cases, I don't remember if that's in a tagged release yet.