Closed (fixed)
Project:
Drupal core
Version:
8.5.x-dev
Component:
other
Priority:
Normal
Category:
Plan
Assigned:
Unassigned
Reporter:
Created:
15 Apr 2017 at 17:41 UTC
Updated:
21 Jan 2018 at 19:14 UTC
Jump to comment: Most recent
Problem/Motivation
Drupal core has a deprecation process documented here: https://www.drupal.org/core/deprecation
What's missing is a standard way to deprecate hooks.
The following hooks are already marked as deprecated: hook_rest_type_uri_alter(), hook_rest_relation_uri_alter()
Proposed resolution
The deprecation process:
- Mark the hook as
@deprecatedin the*.api.phpfile that documents it. - Convert the invocation of the hook to use
ModuleHandlerInterface::deprecatedInvoke()or its siblings instead ofinvoke(). Alter hooks would calldeprecatedAlter(). These methods onModuleHandlerwill then call@trigger_error()if the installation has any implementations of that hook. Implementation issue for this strategy is here: #2866779: Add a way to trigger_error() for deprecated hooks - Add change records for deprecated hooks explaining how to accomplish use-cases without the hook.
Comments
Comment #2
mile23Comment #3
alexpottI was thinking this might be one way of doing it - but an issue is that other places can invoke the hook too - I was wondering if the module handler shouldn't get a list of deprecated hooks and trigger an error if one is used.
Comment #4
mile23#2866779: Add a way to trigger_error() for deprecated hooks adds methods to
ModuleHandlerInterfacelikeinvokeDeprecated().So you find places where the hooks are invoked and switch them to
invokeDeprecated()which signals with a@trigger_error()if there are any implementations.That issue is currently postponed because there's not yet a good way to test whether it works. :-)
Comment #5
dawehnerWe could also introduce 'deprecated' on
hook_hook_info). Personally I don't like the idea ofhook_hook_info()but at least this here seem to be a good usage.Comment #6
joachim commented> but an issue is that other places can invoke the hook too
I would say that if you're invoking a hook that is not invented by your own module, you're doing it wrong.
Hooks fall into these groups:
- alter hooks, which change a data structure managed by a module. You should be using the API that gets the overall data
- reaction hooks. You should be calling the API that causes the action, eg entity_save()
- declaration hooks (we still have a few of these it seems!) Same as alter hooks, you should be using the API that gathers the things being declared.
Comment #7
alexpott@dawehner hook_hook_info() seems like a really good idea.
@joachim I agree it is a bad idea but that has not stopped people from doing it.
Comment #8
mile23What about when we want to deprecate hook_hook_info? :-)
Comment #9
alexpott@Mile23 at that point Drupal no longer has hooks so we're good to go :)
Comment #10
joachim commented> @joachim I agree it is a bad idea but that has not stopped people from doing it.
True, but are we then responsible for notifying these people in their code? And also, if they don't care about working around APIs like this, then how likely are they to care about not using deprecated APIs?
Comment #11
alexpott@joachim they're not likely to care at all - but maybe people who use a contrib module that does this will appreciate that they can discover this module is using a deprecated hook.
Comment #12
mile23I hear you, but we have to deprecate it before it's gone. :-)
Comment #13
dawehnerGuess what, we adapt the
ModuleHandlerand call it a day.Comment #14
catchhook_hook_info() is optional. It was added to put hooks into a separate file, not to document the hooks. Even if we're not ready to deprecate it yet (I think Views is the big info hook user mostly remaining), we shouldn't encourage people to use it either.
I think I prefer adding the methods though to be honest. Then you just delete the invocations and the hook_*() documentation and you're done when you're ready to drop bc.
Comment #15
mile23So shall we move forward with the implementation? I think as @catch points out
hook_hook_info()is optional, and thedeprecatedInvoke*()methods are a more consistent conversion during the deprecation process.Comment #16
dawehnerI agree, its maybe a bit more explicit what is going on.
Comment #17
cilefen commentedComment #18
mile23Moving forward on #2866779: Add a way to trigger_error() for deprecated hooks
Comment #19
mile23Added a change notice: https://www.drupal.org/node/2881531
It'll need updating after #2866779: Add a way to trigger_error() for deprecated hooks is finalized, tagging and work occurring there.
Comment #21
larowlanThis looks good, once its in we'll need to update the deprecation policy
Comment #22
larowlanDocs were updated, adding issue credits and marking fixed
Final documentation https://www.drupal.org/core/deprecation#how-hook
Comment #23
mile23Added follow-up for REST hooks: #2934994: Complete deprecation of hook_rest_type_uri_alter(), hook_rest_relation_uri_alter()