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
| Comment | File | Size | Author |
|---|
Issue fork drupal-3001190
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
Comment #2
xanoComment #3
xanoComment #5
xanoComment #7
xanoComment #9
xanoComment #11
xanoComment #13
xanoHmmz, 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
ExtensionInstallStorageis always instantiated by the container, which indicated theinstall_profilecontainer parameter may not always be set.Comment #21
dpiSomething to pay attention to https://wiki.php.net/rfc/deprecated_attribute
Comment #24
nicxvan commentedWell, hook_hook_info is now deprecated is there another way to do this?
Comment #25
nicxvan commentedI 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.
Comment #26
nicxvan commentedComment #27
nicxvan commentedComment #29
nicxvan commentedComment #30
berdirI 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.
Comment #31
nicxvan commentedThank 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.
Comment #32
nicxvan commented@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.
Comment #35
nicxvan commentedComment #36
nicxvan commentedComment #37
nicxvan commentedComment #38
needs-review-queue-bot commentedThe 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.
Comment #39
nicxvan commentedWrong issue
Comment #40
nicxvan commentedComment #41
nicxvan commentedRebased
Comment #42
needs-review-queue-bot commentedThe 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.
Comment #43
nicxvan commentedI 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.
Comment #44
smustgrave commentedBelieve the merge conflict is valid now in core/.deprecation-ignore.txt. Bot occasionally has it's moments.
Comment #45
nicxvan commentedYes, this is real now