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 a new report_fields_where_declared config setting #1661

Merged
merged 1 commit into from
May 27, 2023

Conversation

mpdude
Copy link
Contributor

@mpdude mpdude commented May 23, 2023

This adds a new config setting report_fields_where_declared that makes it easy to opt-in to the new mapping driver mode that was added in Doctrine ORM 2.16 and will be mandatory in ORM 3.0. For details, see doctrine/orm#10455.

I think that since this bundle allows to specify the mapping configuration per entity manager, it would make sense to have this new config setting at the entity manager level as well.

@mpdude mpdude changed the title Add a new report_field_where_declared config setting Add a new report_fields_where_declared config setting May 23, 2023
@mpdude mpdude force-pushed the report-fields-where-declared branch 2 times, most recently from 2bd23e1 to c61d3e7 Compare May 23, 2023 21:53
@mpdude mpdude force-pushed the report-fields-where-declared branch from c61d3e7 to 9b3b25a Compare May 23, 2023 21:58
@greg0ire
Copy link
Member

how do I make sure that the config setting is used only for ORM versions that already support it?

You may use feature detection with trait_exists(ReflectionBasedDriver::class)

And once we're at Doctrine 3.0, how to notify users that the setting is a no-op and should be removed?

Likewise, use feature detection based on something hopefully related to this feature.

Copy link
Member

@ostrolucky ostrolucky left a comment

Choose a reason for hiding this comment

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

Docs should be added too (configuration.rst and UPGRADE-2.10.md)

@ostrolucky ostrolucky added this to the 2.10.0 milestone May 24, 2023
@ostrolucky ostrolucky changed the base branch from 2.9.x to 2.10.x May 24, 2023 06:14
@ostrolucky ostrolucky changed the base branch from 2.10.x to 2.9.x May 24, 2023 06:15
@ostrolucky ostrolucky changed the base branch from 2.9.x to 2.10.x May 24, 2023 06:15
@mpdude
Copy link
Contributor Author

mpdude commented May 24, 2023

@ostrolucky Do I need the UPGRADE entry when there is nothing users have to do during the upgrade? They can opt in at any time as long as ORM ^2.16 is available.

Don't get me wrong, I'll update configuration.rst for sure.

@mpdude
Copy link
Contributor Author

mpdude commented May 24, 2023

how do I make sure that the config setting is used only for ORM versions that already support it?

You may use feature detection with trait_exists(ReflectionBasedDriver::class)

And once we're at Doctrine 3.0, how to notify users that the setting is a no-op and should be removed?

Likewise, use feature detection based on something hopefully related to this feature.

Not sure this is going to work, since I cannot tell whether I am before ORM 2.16 (config setting makes no sense, need not be available) or at ORM 3.0 (config setting should be removed).

Can I use https://getcomposer.org/doc/07-runtime.md#knowing-whether-package-x-is-installed-in-version-y – that requires Composer 2.x?

@greg0ire
Copy link
Member

greg0ire commented May 24, 2023

I cannot tell whether I am before ORM 2.16 (config setting makes no sense, need not be available) or at ORM 3.0 (config setting should be removed).

Well you could check if ClassMetadataInfo exists, for instance.

Can I use https://getcomposer.org/doc/07-runtime.md#knowing-whether-package-x-is-installed-in-version-y – that requires Composer 2.x?

Might be fine as well.

@ostrolucky
Copy link
Member

@ostrolucky Do I need the UPGRADE entry when there is nothing users have to do during the upgrade?

I'm basing this on what we have in UPGRADE-2.9.md but IIRC we don't document things that users have to change only, but also things that should be changed. Especially things that help them avoid deprecations. I mean, let's say ORM 3.x or 4.x decides to throw exception if you don't set the value to true. In which UPGRADE-* file would you suggest then to do add this docs once such ORM is released and all users code starts breaking because they didn't change that config?

@mpdude
Copy link
Contributor Author

mpdude commented May 24, 2023

The thing is that users should set the config option once they upgrade doctrine/orm to >= 2.16, and remove it when they upgrade doctrine/orm to >= 3.0. Both times, they probably don't look at DoctrineBundle upgrade notes, but rather at the doctrine/orm ones.

It may happen that when making the DoctrineBundle 2.10 upgrade, ORM 2.16 will not even be released in the first place.

@ostrolucky
Copy link
Member

Again, check how UPGRADE-2.9.md is written. It explains everything, similar like you explained in your last comment, that's how it should be. We are not just going to tell user to set it, but to set it if they use ORM 2.16 and to do it if they want to solve deprecation and prepare for ORM 3.

they probably don't look at DoctrineBundle upgrade notes, but rather at the doctrine/orm ones.

Opposite for me. I don't look at orm notes because they are often meaningless in symfony context. For example orm notes will tell you to set some argument of some class/function, that's meaningless in context of symfony user. Symfony user needs to know which config value they need to flip in doctrine.yaml, since bundle is doing the passing of the argument and not the user.

@dmaicher
Copy link
Contributor

dmaicher commented May 24, 2023

Maybe we can just bump the conflict to <2.16 for the ORM (once released?) to address the challenge you mentioned in the PR description?

@mpdude mpdude force-pushed the report-fields-where-declared branch from 299c32a to 381d5a0 Compare May 25, 2023 18:45
@mpdude
Copy link
Contributor Author

mpdude commented May 25, 2023

I have added an explanatory text in a new UPGRADE-2.10.md file, explaining the changes and possible impact.

Regarding feature detection, what if we simply go without it?

Turning the new flag on for pre-ORM 2.16 does no harm: It adds a new constructor parameter to mapping drivers that will simply be ignored.

For ORM ^2.16, the ORM will trigger a deprecation notice unless the flag is turned on.

In ORM ^3.1, a deprecation notice will be triggered stating that the flag is a no-op and the configuration should be removed.

... so maybe nothing we need to take care of here at all?

@mpdude mpdude force-pushed the report-fields-where-declared branch from 381d5a0 to 34e96aa Compare May 25, 2023 18:50
@mpdude mpdude requested a review from ostrolucky May 25, 2023 20:29
@ostrolucky
Copy link
Member

Yeah I think it's good enough like this. Can you rebase? Then we can merge.

This adds a new config setting `report_fields_where_declared` that makes it easy to opt-in to the new mapping driver mode that was added in Doctrine ORM 2.16 and will be mandatory in ORM 3.0. For details, see doctrine/orm#10455.

I think that since this bundle allows to specify the mapping configuration per entity manager, it would make sense to have this new config setting at the entity manager level as well.
@mpdude mpdude force-pushed the report-fields-where-declared branch from 34e96aa to 04a22aa Compare May 27, 2023 16:07
@mpdude
Copy link
Contributor Author

mpdude commented May 27, 2023

@ostrolucky Rebased, thank you

@ostrolucky ostrolucky merged commit f1d9f82 into doctrine:2.10.x May 27, 2023
@mpdude mpdude deleted the report-fields-where-declared branch May 27, 2023 17:46
@mpdude mpdude mentioned this pull request Jun 2, 2023
5 tasks
@stof
Copy link
Member

stof commented Jun 2, 2023

This misses the update of the XSD file (so the documented configuration for the XML format will break)

mpdude added a commit to mpdude/DoctrineBundle that referenced this pull request Jun 2, 2023
ostrolucky pushed a commit that referenced this pull request Jun 2, 2023
…#1668)

Co-authored-by: Christophe Coevoet <[email protected]>
Fix XSD for the new `report-fields-where-declared´ setting from #1661
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.

5 participants