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

Deprecate reliance on non-optimal defaults #8931

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

greg0ire
Copy link
Member

What was optimal 10 years ago no longer is, and things might change in
the future. Using AUTO is still the best solution in most cases, and it
should be easy to make it mean something else when it is not.

Closes #8893, and maybe we should close doctrine/dbal#5614 as well.

@greg0ire
Copy link
Member Author

@derrabus I'm a bit unfamiliar with symfony recipes, would it be possible to add a configuration key iff the platform family is postgresql?

@derrabus
Copy link
Member

@derrabus I'm a bit unfamiliar with symfony recipes, would it be possible to add a configuration key iff the platform family is postgresql?

No, but you could add a piece of configuration and comment it out. That would make it discoverable.

That being said, how do we want to leverage this setting in DoctrineBundle? I'd imagine it would be something like this:

doctrine:
    orm:
        identity_generation_preferences:
            oracle: identity
            postgresql: identity

And that block is something we could add as-is to the recipe. If an application would use e.g. MySQL, those settings would just become no-ops.

@greg0ire
Copy link
Member Author

Yes that's how I imagine it should look like (except oracle does not support identity I think)

@morozov
Copy link
Member

morozov commented Aug 24, 2021

I believe the current set of GENERATOR_TYPE_* constants mixes together a set of different concerns:

  1. Which identifiers will be generated automatically? A sequence of integers (1, 2, 3, …) or UUIDs.
  2. How the generation of numbers is implemented at the target platform (identity column or sequence).
  3. Who decides on everything above (the user, the target platform, or the ORM).

It seems to me that each of the aspects should be represented by a separate abstraction at the ORM level. Mixing them together makes it challenging to reason about.

@derrabus derrabus changed the base branch from 2.10.x to 2.11.x October 4, 2021 14:09
@greg0ire greg0ire mentioned this pull request Jan 29, 2022
@greg0ire greg0ire force-pushed the gh-8893 branch 3 times, most recently from d5ee212 to aaa9f9e Compare January 29, 2022 12:56
@greg0ire greg0ire changed the base branch from 2.11.x to 2.12.x January 29, 2022 13:05
@greg0ire
Copy link
Member Author

I'm resuming this… indeed, there is a mix of concerns here. It's not clear to me however how to fix it or whether it should block this PR (which does introduce more usage of these constants).

What was optimal 10 years ago no longer is, and things might change in
the future. Using AUTO is still the best solution in most cases, and it
should be easy to make it mean something else when it is not.
@greg0ire greg0ire merged commit c5137da into doctrine:2.17.x Oct 11, 2023
59 checks passed
@greg0ire greg0ire deleted the gh-8893 branch October 11, 2023 08:38
@greg0ire greg0ire added this to the 2.17.0 milestone Oct 11, 2023
@DannyvdSluijs
Copy link

@greg0ire Thanks for all the work and I understand the reasoning. I do think the deprecation is a bit to opmisticly implemented. As for a Postgres platform it will always through the deprecation if no IdentityGenerationPreferences has been set in the configuration. Currently thecomposer require doctrine/doctrine-bundle doesn't have support for the IdentityGenerationPreferences.

As a shortcut I would add the correct strategy #[ORM\GeneratedValue(strategy: 'SEQUENCE')] attribute but this leads to the deprecation being thrown as the predefined value on the Entity attributes isn't inspected. Is there another way around this?

@greg0ire
Copy link
Member Author

Currently thecomposer require doctrine/doctrine-bundle doesn't have support for the IdentityGenerationPreferences.

You still probably have access to the configuration object though, right? Maybe in src/Kernel.php?

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

Successfully merging this pull request may close these issues.

Deprecate AUTO identifier generator strategy GeneratedValue strategy AUTO should use IDENTITY on PostgreSQL
4 participants