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

Add regression test for #8108 #11523

Draft
wants to merge 1 commit into
base: 3.2.x
Choose a base branch
from
Draft

Conversation

Jean85
Copy link
Contributor

@Jean85 Jean85 commented Jun 25, 2024

As reported in #8108 (comment), this is a regression test for a bug that I encounter when upgrading from ORM 2 to 3. I'm not sure how this should be fixed.

[EDIT] I should add that this doesn't happen if the relation is coming from a trait instead of an abstract class.

@Jean85
Copy link
Contributor Author

Jean85 commented Jul 1, 2024

Ok I've debugged the issue, that lies in \Doctrine\ORM\Mapping\Driver\ReflectionBasedDriver::isRepeatedPropertyDeclaration:

  • that method should return true, skipping the whole "double-mapping"
  • ...but it doesn't because:
    • the reflection says that the property is defined in an (abstract) class which is not declared as an entity (Issue8108WithRelation)
    • while the mapping says that the property is declared in the first entity of the chain (Issue8108Base)

Any suggestion on how we should fix this?

[EDIT] In short, it seems that the issue stems from the fact that I'm having this chain of inheritance:

  1. class A (NOT declared as an entity) with a relation / field declared on a property
  2. class B extends A, declared as mapped superclass
  3. class C extends B, declared as child entity of B

@Jean85
Copy link
Contributor Author

Jean85 commented Jul 2, 2024

Bisecting and using this as the reproducer, I've discovered that my chain of inheritance was considered "bad" but accepted in 2.x only thanks to the reportFieldsWhereDeclared set to false.

Should I consider this as invalid and close?

@greg0ire
Copy link
Member

greg0ire commented Jul 2, 2024

I've discovered that my chain of inheritance was considered "bad

By what? The schema validator? If yes, then we should probably close, and I think we might want to work on making it impossible to write regression tests for an invalid schema 🤔

@Jean85
Copy link
Contributor Author

Jean85 commented Jul 2, 2024

Locally, the schema validator is what led me to this reproducer, but it's an exception, not a validation error.

This reproducer, ported to the 2.x branch, fails if reportFieldsWhereDeclared is true (with the same exception), passes otherwise. I don't know if it's a validation error or the bug itself, but this is the stack trace:

Doctrine\ORM\Mapping\MappingException: Property "createdBy" in "TmpTest\Issue8108Extending" was already declared, but it must be declared only once                                                         
                                                                                                                                                                                                            
doctrine2/src/Mapping/MappingException.php:395                                                                                                                                     
doctrine2/src/Mapping/ClassMetadataInfo.php:3826                                                                                                                                   
doctrine2/src/Mapping/ClassMetadataInfo.php:3012                                                                                                                                   
doctrine2/src/Mapping/ClassMetadataInfo.php:2980                                                                                                                                   
doctrine2/src/Mapping/Driver/AnnotationDriver.php:619                        
doctrine2/src/Mapping/Driver/AnnotationDriver.php:442                                                                                                                              
doctrine2/src/Mapping/ClassMetadataFactory.php:149                           
doctrine2/vendor/doctrine/persistence/src/Persistence/Mapping/AbstractClassMetadataFactory.php:343                                                                                 
doctrine2/vendor/doctrine/persistence/src/Persistence/Mapping/AbstractClassMetadataFactory.php:207
doctrine2/src/EntityManager.php:318                                                                                                                                                
doctrine2/tests/Tests/OrmFunctionalTestCase.php:381                          
doctrine2/tests/Tests/OrmFunctionalTestCase.php:379                          
doctrine2/tests/Tests/OrmFunctionalTestCase.php:351                                                                                                                                
doctrine2/Issue8108Test.php:32

@greg0ire
Copy link
Member

greg0ire commented Jul 2, 2024

but it's an exception, not a validation error

Then it might be to severe to merely be marked as invalid only to be allowed later on.

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

Successfully merging this pull request may close these issues.

2 participants