Skip to content

[Contributing] Add more information on deprecations #12075

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

Merged
merged 1 commit into from
Aug 8, 2019
Merged

[Contributing] Add more information on deprecations #12075

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
93 changes: 74 additions & 19 deletions contributing/code/conventions.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,31 +79,47 @@ must be used instead (where ``XXX`` is the name of the related thing):

.. _contributing-code-conventions-deprecations:

Deprecations
------------
Deprecating code
----------------

From time to time, some classes and/or methods are deprecated in the
framework; that happens when a feature implementation cannot be changed
because of backward compatibility issues, but we still want to propose a
"better" alternative. In that case, the old implementation can simply be
**deprecated**.

Deprecations must only be introduced on the next minor version of the impacted
component (or bundle, or bridge, or contract).
They can exceptionally be introduced on previous supported versions if they are critical.

A new class (or interface, or trait) cannot be introduced as deprecated, or
contain deprecated methods.

A new method cannot be introduced as deprecated.

A feature is marked as deprecated by adding a ``@deprecated`` phpdoc to
relevant classes, methods, properties, ...::

/**
* @deprecated since vendor-name/package-name 2.8, to be removed in 3.0. Use XXX instead.
* @deprecated since Symfony 2.8.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the rewording here?

cc @greg0ire

Copy link
Contributor

@greg0ire greg0ire Aug 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I added this after talking with @nicolas-grekas, what's the deal?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes you added the vendor/... thingy which now gets removed, that’s my point

Copy link
Contributor

Choose a reason for hiding this comment

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

I know, I was more asking @fancyweb or @nicolas-grekas to explain the sudden change of heart

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 the misunderstanding is because this is the "Contribute to Symfony" doc ... so it's not a generic-purpose doc and we don't need things like "vendor-name/package-name" because "vendor" is always "Symfony" and the "package-name" is not usually included in the Symfony deprecation messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding the package name, there is not always a way to guess it from a class name, for instance here:

https://github.com/symfony/var-exporter/blob/8539c2cec7202d244058075351c61aa852ffa344/Internal/Exporter.php#L120

Also, why remove the Use XXX instead. part? If there is a replacement, it should absolutely be mentioned IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "use XXX instead" is not removed. This example is just below.

Copy link
Member

Choose a reason for hiding this comment

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

@greg0ire this link points to a notice, not to a deprecation :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right my bad, here is another example:

https://github.com/symfony/symfony/blob/38143c604f68038e28cf2fa2fbb61c04fc68b98c/src/Symfony/Component/Serializer/Encoder/CsvEncoder.php#L51

Copy link
Member

Choose a reason for hiding this comment

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

bad one indeed :)

*/

The deprecation message must indicate the version in which the feature was deprecated,
and whenever possible, how it was replaced::

/**
* @deprecated since Symfony 2.8, use Replacement instead.
*/

The deprecation message should indicate the version when the class/method was
deprecated, the version when it will be removed, and whenever possible, how
the feature was replaced.
When the replacement is in another namespace than the deprecated class, its FQCN must be used::

/**
* @deprecated since Symfony 2.8, use A\B\Replacement instead.
*/

A PHP ``E_USER_DEPRECATED`` error must also be triggered to help people with
the migration starting one or two minor versions before the version where the
feature will be removed (depending on the criticality of the removal)::
A PHP ``E_USER_DEPRECATED`` error must also be triggered to help people with the migration::

@trigger_error('XXX() is deprecated since vendor-name/package-name 2.8 and will be removed in 3.0. Use XXX instead.', E_USER_DEPRECATED);
@trigger_error(sprintf('The "%s" class is deprecated since Symfony 2.8, use "%s" instead.', Deprecated::class, Replacement::class), E_USER_DEPRECATED);

Without the `@-silencing operator`_, users would need to opt-out from deprecation
notices. Silencing swaps this behavior and allows users to opt-in when they are
Expand All @@ -114,19 +130,58 @@ the Web Debug Toolbar or by the PHPUnit bridge).

When deprecating a whole class the ``trigger_error()`` call should be placed
between the namespace and the use declarations, like in this example from
`ArrayParserCache`_::
`ServiceRouterLoader`_::

namespace Symfony\Component\ExpressionLanguage\ParserCache;
namespace Symfony\Component\Routing\Loader\DependencyInjection;

@trigger_error('The '.__NAMESPACE__.'\ArrayParserCache class is deprecated since version 3.2 and will be removed in 4.0. Use the Symfony\Component\Cache\Adapter\ArrayAdapter class instead.', E_USER_DEPRECATED);
use Symfony\Component\Routing\Loader\ContainerLoader;

use Symfony\Component\ExpressionLanguage\ParsedExpression;
@trigger_error(sprintf('The "%s" class is deprecated since Symfony 4.4, use "%s" instead.', ServiceRouterLoader::class, ContainerLoader::class), E_USER_DEPRECATED);

/**
* @author Adrien Brault <adrien.brault@gmail.com>
*
* @deprecated since Symfony 3.2, to be removed in 4.0. Use the Symfony\Component\Cache\Adapter\ArrayAdapter class instead.
* @deprecated since Symfony 4.4, use Symfony\Component\Routing\Loader\ContainerLoader instead.
*/
class ArrayParserCache implements ParserCacheInterface
class ServiceRouterLoader extends ObjectRouteLoader

.. _`ServiceRouterLoader`: https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/Routing/Loader/DependencyInjection/ServiceRouterLoader.php

The deprecation must be added to the ``CHANGELOG.md`` file of the impacted component::

4.4.0
-----

* Deprecated the `Deprecated` class, use `Replacement` instead.

It must also be added to the ``UPGRADE.md`` file of the targeted minor version
(``UPGRADE-4.4.md`` in our example)::

DependencyInjection
-------------------

* Deprecated the `Deprecated` class, use `Replacement` instead.

Finally, its consequences must be added to the ``UPGRADE.md`` file of the next major version
(``UPGRADE-5.0.md`` in our example)::

DependencyInjection
-------------------

* Removed the `Deprecated` class, use `Replacement` instead.

All these tasks are mandatory and must be done in the same pull request.

Removing deprecated code
------------------------

Removing deprecated code can only be done once every 2 years, on the next major version of the
impacted component (``master`` branch).

When removing deprecated code, the consequences of the deprecation must be added to the ``CHANGELOG.md`` file
of the impacted component::

5.0.0
-----

* Removed the `Deprecated` class, use `Replacement` instead.

.. _`ArrayParserCache`: https://github.com/symfony/symfony/blob/3.2/src/Symfony/Component/ExpressionLanguage/ParserCache/ArrayParserCache.php
This task is mandatory and must be done in the same pull request.