Problem/Motivation

#2866779: Add a way to trigger_error() for deprecated hooks added ways to mark hooks deprecated by introducing new API methods. This means we needed one new API method for every method hooks can be invoked, adding three new methods for one small purpose, as well as putting the deprecation burden not on the hook owner, but the owner of every single invocation of deprecated hooks, and while we tend to wrap hook invocations in their own API methods, invoking hooks without API wrappers used to be common practice for a long time.

Proposed resolution

We can deprecate these three ::*Deprecated() methods and put the burden of deprecation solely with hook owners by marking hooks deprecated with #[DeprecatedHook].
This will take the following parameters:
hook
provider
version deprecated
version removed
message
cr

HookCollectorPass will trigger errors for any implementations of deprecated hooks.

Remaining tasks

Decide if we want to delete some deprecated implementations or the ignore is enough.

Should we add dynamic patterns here or in a follow up.

User interface changes

None.

API changes

ModuleHandlerInterface::*Deprecated() will become deprecated.

Data model changes

Issue fork drupal-3001190

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

Xano created an issue. See original summary.

xano’s picture

Title: Deprecate ModuleHandlerInterface::Deprecated() in favor of hook metadata » Deprecate ModuleHandlerInterface::*Deprecated() in favor of hook metadata
Status: Active » Needs review
StatusFileSize
new2.51 KB
xano’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 2: drupal_3001190_2.patch, failed testing. View results

xano’s picture

Status: Needs work » Needs review
StatusFileSize
new2.2 KB

Status: Needs review » Needs work

The last submitted patch, 5: drupal_3001190_5.patch, failed testing. View results

xano’s picture

Status: Needs work » Needs review
StatusFileSize
new6.97 KB
new6.82 KB

Status: Needs review » Needs work

The last submitted patch, 7: drupal_3001190_7.patch, failed testing. View results

xano’s picture

Assigned: xano » Unassigned
Status: Needs work » Needs review
Issue tags: +Needs change record
StatusFileSize
new8.32 KB
new9.84 KB

Status: Needs review » Needs work

The last submitted patch, 9: drupal_3001190_9.patch, failed testing. View results

xano’s picture

Status: Needs work » Needs review
StatusFileSize
new2.31 KB
new10.66 KB

Status: Needs review » Needs work

The last submitted patch, 11: drupal_3001190_11.patch, failed testing. View results

xano’s picture

Hmmz, I can't figure out what the patch does to trigger all these Install profile will be a mandatory parameter in Drupal 9.0. errors... Looks like ExtensionInstallStorage is always instantiated by the container, which indicated the install_profile container parameter may not always be set.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dpi’s picture

Something to pay attention to https://wiki.php.net/rfc/deprecated_attribute

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

nicxvan’s picture

Well, hook_hook_info is now deprecated is there another way to do this?

nicxvan’s picture

Status: Needs work » Postponed (maintainer needs more info)

I don't see anyway to do this now that we don't have hook hook info.

I think invocation time is the way to do it anyway.

nicxvan’s picture

Issue summary: View changes
Status: Postponed (maintainer needs more info) » Active
nicxvan’s picture

nicxvan’s picture

Title: Deprecate ModuleHandlerInterface::*Deprecated() in favor of hook metadata » Deprecate ModuleHandlerInterface::*Deprecated() in favor of #[DeprecatedHook]
Issue tags: -Needs change record
berdir’s picture

I understand the idea and generally it makes sense, but at a glance, a few things jump out to me as awkward:

a) the location of the attribute. Putting them on a hooks class seems weird. What if your module doesn't implement any hooks? This would be different if we'd have explicit Hook attribute classes, then we could deprecate those, which would also directly expose that information to anyone using those hooks (assuming that IDE's have a concept of deprecated attributes). Attributes are great because it allows you to add metadata next to the actual thing, but there is no thing here. Maybe this could use a yaml file, but that brings me to the second point.

b) Dynamic hooks. What if we deprecate hook_form_FORM_ID_alter(), or hook_ENTITY_TYPE_load()? What if only _some_ of those are deprecated? Lets say we deprecate hook_form_BASE_FORM_ID_alter() but not hook_form_FORM_ID_alter()? Even if we'd support regex or wildcard, this will be imperfect and will possibly flag certain hooks incorrectly as deprecated that aren't.

nicxvan’s picture

Thank you for taking a look.

I had considered this.

a) yes I understand, but those issues exist with the current method as well with two added problems that this solves.
1 you can deprecate without using module handler.
2 It keeps the hook metadata in attributes extending a pattern that will get more familiar. Creating an empty class isn't a huge burden, I did that for the core hook deprecations.

I also think this is much nicer than yaml, you get hints when adding the attribute and you don't have to worry about indentation.

b) While this may be valid, it is an issue with the current process too, you could not independently deprecate the dynamic stacked form alters. We could likely create a custom dynamic deprecation.

In the end this cleans up an already overloaded service and gives us a cleaner way to do a rarely done task.

One thing we might want to do is add another parameter for dynamic hooks with a pattern.

nicxvan’s picture

@dww on slack said we could delete the hook implementations.

However since there are explicit tests for them I think we should ignore the build time deprecations for those.

We could also remove the ones that are in system and leave the tests, then just ignore those specific deprecations too.

nicxvan changed the visibility of the branch 3001190-test-cs to hidden.

nicxvan’s picture

Issue summary: View changes
Status: Active » Needs review
nicxvan’s picture

Issue summary: View changes
nicxvan’s picture

Issue summary: View changes
needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

nicxvan’s picture

Status: Needs work » Needs review

Wrong issue

nicxvan’s picture

Status: Needs review » Needs work
nicxvan’s picture

Status: Needs work » Needs review

Rebased

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

nicxvan’s picture

Status: Needs work » Needs review
Issue tags: +no-needs-review-bot

I think the bot is wrong, I literally just rebased and the UI doesn't show any conflicts. I think it ran on the previous botched rebase.

smustgrave’s picture

Status: Needs review » Needs work

Believe the merge conflict is valid now in core/.deprecation-ignore.txt. Bot occasionally has it's moments.

nicxvan’s picture

Issue tags: -no-needs-review-bot

Yes, this is real now

nicxvan changed the visibility of the branch 11.x to hidden.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.