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.

Sign up for GitHub

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

Deprecated the usage of the Version class #3861

Merged
merged 1 commit into from
Jan 30, 2020
Merged

Deprecated the usage of the Version class #3861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions UPGRADE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Upgrade to 2.11

## Deprecated `Doctrine\DBAL\Version` class

The usage of the `Doctrine\DBAL\Version` class is deprecated as internal implementation detail. Please refrain from checking the DBAL version at runtime.

## Deprecated `ExpressionBuilder` methods

The usage of the `andX()` and `orX()` methods of the `ExpressionBuilder` class has been deprecated. Use `and()` and `or()` instead.
Expand Down
3 changes: 3 additions & 0 deletions lib/Doctrine/DBAL/Version.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@

/**
* Class to store and retrieve the version of Doctrine.
*
* @internal
* @deprecated Refrain from checking the DBAL version at runtime.
Copy link
Member

Choose a reason for hiding this comment

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

I think this will result in a deprecation for users of https://github.com/symfony/error-handler/blob/master/DebugClassLoader.php . Wouldn't marking it as @internal be enough to warn users against using it, and maybe more semantically correct?

Copy link
Member

Choose a reason for hiding this comment

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

It is removed in master. AFAIK master is going to be 4.0, so the deprecation message should contain the information when this class will be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this will result in a deprecation for users of https://github.com/symfony/error-handler/blob/master/DebugClassLoader.php.

The problem is not the deprecation but how it's being handled. In our case, we're just adding an annotation purely for documentation purposes. If a 3rd party component turns this annotation into an undesired runtime side effect, the problem is in the component. Do users use it in production? The "debug" name suggests they shouldn’t.

Wouldn't marking it as @internal be enough to warn users against using it, and maybe more semantically correct?

I don't know. Both annotations are standard and correspond to the messages I want to communicate:

  1. @intermal — the class is an implementation detail and shouldn’t be relied upon outside of the package. It should have been marked like this since the introduction.
  2. @deprecated — the class is going to be removed in a following major release.

It is removed in master. AFAIK master is going to be 4.0, so the deprecation message should contain the information when this class will be removed.

We don't usually do that.

Copy link
Member

Choose a reason for hiding this comment

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

If it's removed on master, then @deprecated should stay 👍

Copy link
Member

Choose a reason for hiding this comment

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

Do users use it in production? The "debug" name suggests they shouldn’t.

You're right, that probably makes this quite ok.

Copy link
Member

Choose a reason for hiding this comment

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

I have no problem with both @internal and @deprecated, since they are different. The explanation @morozov gave above is perfectly correct.

As for the version, these are my thoughts:

  • If we deprecate something and don't know when it will be released, we should say "in a future major release" as that's what will happen
  • Once we know what release removes the member, we should update the deprecation message to state the version that removes it. Even if we deprecate in 2.x for removal in 4.0, we should mention that the class will be removed in 4.0. Users that are interested in the deprecations will appreciate the information as it lets them decide when to take care of the deprecation.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we deprecate something and don't know when it will be released, we should say "in a future major release" as that's what will happen

I'm fine with this definition of “when” — “in a future major release”, however, isn’t implied by semver? It definitely cannot happen earlier.

Once we know what release removes the member, we should update the deprecation message to state the version that removes it.

This is why I don't want to specify any exact versions. Specifically in DBAL, we don’t know that upfront and I don’t want to introduce additional work on updating deprecation comments.

Users that are interested in the deprecations will appreciate the information as it lets them decide when to take care of the deprecation.

I didn’t think people may be really interested in the exact version. What is the practical reason for interest in a specific removal version? In our case, even if we mention a specific version, we don’t have any release dates for those versions.

*/
class Version
{
Expand Down