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 type comment attribute from configuration #947

Merged
merged 2 commits into from Apr 12, 2019

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Apr 5, 2019

Supersedes #628.

The commented attribute will be removed in 2.0. To allow people to migrate properly and not have to deal with deprecations until actually switching to 2.0, the default value of commented changes from true to null so we know whether a type was explicitly marked as commented or not. The connection factory now takes into account whether a type itself requires a comment and only explicitly marks it as commented if

Depending on the scenario, appropriate deprecation notices will be thrown according to the table below:

Type commented No commented attribute commented: true commented: false
Yes No change Deprecation notice to remove attribute Deprecation notice to remove attribute
No Implicitly marked commented to preserve BC, deprecation notice to update type or add commented: false Deprecation notice: update type to be commented No deprecation notice - commented attribute will have to be dropped for 2.0.

Note that users that want to use a type without comments will have to explicitly specify commented: false in the configuration. At the same time, we will not be triggering a deprecation notice for this case. This is because in order to maintain BC, not setting the commented option will implicitly mark the type as commented, even when the type itself doesn't do so. Users that want to have uncommented types will have to remove the commented attribute when upgrading to DoctrineBundle 2.0. If anyone has a suggestion how to best solve this, please let me know as I didn't find any alternative.

@alcaeus alcaeus added this to the 1.11.0 milestone Apr 5, 2019
@alcaeus alcaeus self-assigned this Apr 5, 2019
ConnectionFactory.php Outdated Show resolved Hide resolved
ConnectionFactory.php Outdated Show resolved Hide resolved
ConnectionFactory.php Outdated Show resolved Hide resolved
@stof
Copy link
Member

stof commented Apr 6, 2019

note that in the Configuration class, we should deprecate passing true, but we should not deprecate passing false (as that's necessary to register a non-commented type).
And this means that 2.0 cannot remove the config option. It can only restrict it to accept only false and deprecate it.
We don't want to force devs to make changes between a deprecation-free last minor and the next major version.

@stof
Copy link
Member

stof commented Apr 6, 2019

and yes, removing an option while keeping the behavior of a non-default value requires a 2-step deprecation process. We did that multiple times in Symfony already.

@alcaeus
Copy link
Member Author

alcaeus commented Apr 6, 2019

I was wondering how to best solve the use-case of having commented: false which is necessary. Keeping the option around and only deprecating it completely in 2.0 makes sense. With this PR, commented: true causes deprecation notices in all cases:

  • if the type being registered is commented for the platform, it triggers a deprecation letting the user know that the setting is obsolete
  • if the type being registered is not commented for the platform, the deprecation notice tells the user to update the type to mark itself as commented. This also applies when the commented attribute is omitted and thus implicitly assumed to be true

With that in mind, I don't think we need to deprecate anything in the Configuration class directly, but only deprecate the commented option in 2.0 once the behaviour changes.

@stof
Copy link
Member

stof commented Apr 6, 2019

Yeah, for 1.x, your deprecations cover all cases here.

@alcaeus alcaeus added this to 1.11 in Roadmap Apr 6, 2019
@alcaeus alcaeus requested a review from stof April 6, 2019 18:28
@alcaeus alcaeus added the ⭐️ EUFOSSA Hackathon https://symfony.com/blog/the-symfony-and-api-platform-hackathon-is-coming label Apr 7, 2019
ConnectionFactory.php Outdated Show resolved Hide resolved
ConnectionFactory.php Outdated Show resolved Hide resolved
ConnectionFactory.php Show resolved Hide resolved
@alcaeus alcaeus merged commit 5c79bbc into doctrine:master Apr 12, 2019
@alcaeus alcaeus deleted the deprecate-type-commenting branch April 12, 2019 04:32
GwendolenLynch added a commit to GwendolenLynch/postgresql-for-doctrine that referenced this pull request Aug 4, 2019
GwendolenLynch added a commit to GwendolenLynch/postgresql-for-doctrine that referenced this pull request Aug 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⭐️ EUFOSSA Hackathon https://symfony.com/blog/the-symfony-and-api-platform-hackathon-is-coming Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants