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

Make Reflection available to ConvertMappingCommand #9619

Conversation

bartholdbos
Copy link
Contributor

@bartholdbos bartholdbos commented Mar 31, 2022

Fixes #9659

This fixes an exception when exporting mapping information using the
ConvertMappingCommand from Attributes. The AttributeDriver needs a
ReflectionClass in the loadMetadataForClass method. The
ConvertMappingCommand sets up the ClassMetadataFactory as an instance of
DisconnectedClassMetadataFactory which does not return ReflectionClasses

Copy link
Member

@beberlei beberlei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The disconnected mf is required for xml/yml schemas. You can't just change it

@bartholdbos
Copy link
Contributor Author

Okay, could this be a fix?

?? new ReflectionClass($metadata->name);

@derrabus
Copy link
Member

derrabus commented Apr 4, 2022

Can you please open an issue that describes and reproduces your problem first? I'm having a hard time understanding this change.

@bartholdbos
Copy link
Contributor Author

I have opened an issue with how to reproduce the bug #9659

@beberlei
Copy link
Member

The AnnotationDriver has the same kind of code to allow mapping conversion. Its an unfortunate side effect of our abstraction being sub optimal

@beberlei
Copy link
Member

@bartholdbos yes this is the fix, can you duplicate it into AttributeDrivwr?

@bartholdbos
Copy link
Contributor Author

Yes I have duplicated the fix into AttributeDriver

@derrabus derrabus changed the base branch from 2.11.x to 2.12.x April 26, 2022 13:06
@bartholdbos bartholdbos force-pushed the no-reflection-class-in-attribute-convert-mapping-command branch from d946ed4 to 21dfe6a Compare May 9, 2022 09:03
This fixes an exception when exporting mapping information using the
ConvertMappingCommand from Attributes. The AttributeDriver needs a
ReflectionClass in the loadMetadataForClass method. The
ConvertMappingCommand sets up the ClassMetadataFactory as an instance of
DisconnectedClassMetadataFactory which does not return ReflectionClasses
@bartholdbos bartholdbos force-pushed the no-reflection-class-in-attribute-convert-mapping-command branch from 21dfe6a to b2bebd2 Compare May 9, 2022 09:28
@bartholdbos
Copy link
Contributor Author

I added the PHPStan warning to the baseline as was done for the AnnotationDriver

@bartholdbos
Copy link
Contributor Author

The pipeline is green, I think it's ready to be reviewed (and merged hopefully)

@derrabus derrabus requested a review from beberlei May 10, 2022 17:00
@bartholdbos
Copy link
Contributor Author

@beberlei Hi Benjamin, could you maybe review the fix please?

@derrabus derrabus added this to the 2.12.3 milestone Jun 13, 2022
@derrabus derrabus merged commit 09ff36c into doctrine:2.12.x Jun 13, 2022
@derrabus
Copy link
Member

Thank you @bartholdbos!

@bartholdbos bartholdbos deleted the no-reflection-class-in-attribute-convert-mapping-command branch June 15, 2022 08:44
@bartholdbos bartholdbos restored the no-reflection-class-in-attribute-convert-mapping-command branch June 15, 2022 08:44
derrabus added a commit to derrabus/orm that referenced this pull request Jun 17, 2022
* 2.13.x:
  Deprecate omitting the alias in QueryBuilder (doctrine#9765)
  Run tests on PHP 8.2 (doctrine#9840)
  PHPStan 1.7.13 (doctrine#9844)
  Flip conditional extension of legacy AnnotationDriver class (doctrine#9843)
  PHP CodeSniffer 3.7 (doctrine#9842)
  Make Reflection available to ConvertMappingCommand (doctrine#9619)
  Add missing property declaration
  Use proper API for introspection of tables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConvertMappingCommand does not work from attributes to xml
3 participants