[Policy, no patch] Determine and document best practices for providing backwards compatibility code paths expected to be removed in the next major version.

Created on 22 July 2019, about 5 years ago
Updated 29 May 2024, 3 months ago

Problem/Motivation

Often, in the course of improving code in Drupal, it becomes necessary to provide code paths that are used specifically to maintain our backwards compatibility promise. It's not appropriate to mark the code as deprecated, as it may include conditionals that are expected to be run, or it may include data transformations that update legacy data to match current expectations while remaining idempotent for modern data, and are thus run on every data set currently.

A common example is providing for dependencies that are added to constructors in a backwards compatible way. In this case, a new optional parameter is added to the end of the constructor, and then a BC layer is added to detect if the parameter is missing, and load it from the container if it is, usually triggering a deprecation error. There are numerous other examples in core. The migration system deprecated the use of CckFieldPlugins in favor of the replacement FieldPlugins, and requires a fair amount of code to manage still-existing CckFieldPlugins that is not technically @deprecated, but should be removed when the Drupal 9 branch is open. Views has a method View::fixTableNames() which exists to update views which may have been saved prior to the 8.3 revisionable entity updates, and imported. Eventually this method should be removed, perhaps in Drupal 9, perhaps later. This method is currently marked deprecated, but should not be, as it is correctly still in use.

Several other examples have been noticed while working on cleaning up deprecations in #3038170: Drupal core's own deprecation testing results β†’

Currently we handle this in a few ways. Sometimes we mark methods as deprecated, which is not correct, and won't work as we are now enforcing proper use of the @deprecated tag. Sometimes there is a comment in the code pointing out that the code exists as a BC layer and should be removed. Sometimes we file follow-ups, and sometimes we do nothing to mark that the code needs to be removed in a later version.

The current documentation on this is at https://www.drupal.org/core/deprecation#how-codepath β†’ and says only:

For example when we have a code path handling a deprecated key in a YAML file.
Add @trigger_error('...', E_USER_DEPRECATED) at the start of the code block that handles the backwards compatibility layer. DO NOT add an @deprecated phpdoc annotation to the method or function docblock that contains the code path as this causes IDEs to mark the entire method as deprecated. For example:

// @todo

Other documentation on that page suggests where to trigger an error in certain situations, but does not document ways to mark a particular code block as deprecated and needing to be removed. The main purpose of this policy is to designate a way to mark these code blocks as being scheduled for removal in the code so that we can easily create and use automated tools to make sure that these paths are removed when we open a new major version branch.

The below proposal is a starting point for discussion on how to reign this code in, mark it properly, and make sure it is easy to remove in the next major version.

Proposed resolution

First and foremost, any code that is provided for a bc layer and expected to be removed should have a follow-up issue filed to remove it at the time it is committed. The code should contain an @todo describing what is to be removed/changed, and a link to the issue. Should we have a new tag for this so we can scan the code when the next major branch is open? @bc or something?

When possible, BC layer code should be surrounded by a conditional detecting if it is needed, and if so, an E_USER_DEPRECATED error should be triggered.

If the needed logic is more complicated than one or two lines, it should be moved into its own method (where appropriate). This method should be marked @internal, private, and final. @bc? In this case, both the method and the location from which it is called should be commented with the @todo tag, and with a link to the follow-up issue.

Ideally a patch to remove the code could be posted to the follow-up issue at the time it is created, but keeping the patch rolled correctly may prove too much.

Remaining tasks

Discuss if such a policy is even needed.
Determine best practices
Document

User interface changes

none

API changes

none

Data model changes

none

Release notes snippet

🌱 Plan
Status

Active

Version

11.0 πŸ”₯

Component
OtherΒ  β†’

Last updated about 7 hours ago

Created by

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

Production build 0.71.5 2024