Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ORM 3.0 compat #2708

Open
mbabker opened this issue Oct 11, 2023 · 17 comments
Open

ORM 3.0 compat #2708

mbabker opened this issue Oct 11, 2023 · 17 comments

Comments

@mbabker
Copy link
Contributor

mbabker commented Oct 11, 2023

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

@mbabker
Copy link
Contributor Author

mbabker commented Oct 29, 2023

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

  • Call to undefined method Doctrine\ORM\UnitOfWork::clearEntityChangeSet() - The Gedmo\Mapping\Event\Adapter\ORM::clearObjectChangeSet() method relies on being able to call a method on each manager's unit of work to remove a change set for an object. The ORM removed this method in 3.0.
  • Class "Doctrine\ORM\Mapping\ClassMetadataInfo" not found - The Doctrine\ORM\Mapping\ClassMetadata class is now used
  • Undefined constant Doctrine\ORM\Mapping\ClassMetadata::CHANGETRACKING_NOTIFY - There are test fixtures configured with the "NOTIFY" change tracking policy, which was deprecated in 2.9 and removed in 3.0. The ORM suggests "DEFERRED_EXPLICIT" instead, I didn't really dig in here to see if those tests purposefully validate behavior with that policy.
  • Indirect modification of overloaded element of Doctrine\ORM\Mapping\ManyToOneAssociationMapping has no effect - This one's interesting because the stack trace starts at the PHPUnit Bridge's error handler
/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
Copy link
Contributor Author

mbabker commented Nov 14, 2023

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
Copy link
Contributor Author

mbabker commented Dec 14, 2023

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:

  • Test cases using fixtures that are annotation or YAML only (which may or may not uncover new issues as those get adapted to run with the other drivers)
  • The removal of Doctrine\ORM\UnitOfWork::clearEntityChangeSet()
  • The strict XML validation issue (it's been an issue with the MongoDB ODM since its 2.0 release, see Incompatibilities with Doctrine MongoDB ODM 2.0 #2055, and now ORM 3.0 has it)

@mbabker
Copy link
Contributor Author

mbabker commented Jan 28, 2024

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

  • 57 errors due to Call to undefined method Doctrine\ORM\UnitOfWork::clearEntityChangeSet()
  • One Doctrine\ORM\Exception\EntityIdentityCollisionException error
    • 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.
  • One test failure
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
Copy link
Contributor Author

mbabker commented Feb 3, 2024

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.

@andreybolonin
Copy link
Contributor

andreybolonin commented Feb 7, 2024

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
Copy link
Contributor Author

mbabker commented Feb 7, 2024

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

@franmomu
Copy link
Collaborator

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
Copy link
Contributor Author

mbabker commented Feb 12, 2024

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 doctrine/orm#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 #2708 (comment), 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
Copy link
Collaborator

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
Copy link
Contributor Author

mbabker commented Feb 13, 2024

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 d40b449 and 4f69bdd 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
Copy link

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
Copy link
Contributor Author

mbabker commented Mar 12, 2024

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
Copy link
Contributor

@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
Copy link
Contributor Author

mbabker commented Mar 14, 2024

@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 #2502 and it's a DBAL 4.0 blocker.

@klemens-u
Copy link

klemens-u commented Apr 25, 2024

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
Copy link

For a fresh Symfony7, downgrade also:
composer.json

{
    "require": {
        ... 
        "spiriitlabs/form-filter-bundle": "^10",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants