CRM_Utils_Hook: deprecation warning and short array syntax #17995
Conversation
|
(Standard links)
|
| @@ -155,15 +155,14 @@ public function invoke( | |||
| if (!is_array($names)) { | |||
| // We were called with the old contract wherein $names is actually an int. | |||
| // Symfony dispatcher requires some kind of name. | |||
| // TODO: Emit a warning, eg | |||
| // error_log("Warning: hook_$fnSuffix does not give names for its parameters. It will present odd names to any Symfony event listeners."); | |||
| Civi::log()->warning("hook_$fnSuffix should be updated to pass an array of parameter names to CRM_Utils_Hook::invoke().", ['civi.tag' => 'deprecated']); | |||
There was a problem hiding this comment.
@colemanw the CRM_Core_Error::deprecatedFunctionWarning( wrapper function might not be perfect - but it's more greppable so I think it's preferred
|
test this please |
|
I'm gonna merge this - it would be better to have the more grepable string but it does the same thing |
|
@jaapjansma FYI civirules is now hitting this deprecation notice - looks like it needs a small change like this civicrm/org.civicrm.volunteer#547 to avoid deprecation notices (test fails - https://test.civicrm.org/job/CiviCRM-Ext-Matrix/BKPROF=min,BUILDTYPE=git,CIVIVER=master,EXTKEY=org.civicoop.civirules,label=bknix-tmp/ ) @colemanw is calling this hook from outside core a supported thing? If so is a docs update needed? |
|
I know from Sepa, Documents, CiviRules, Form Processor, Data Processor extensions which are invoking own hooks. So those are going to break with the new release. |
|
@jaapjansma this change does not "break" anything. It's just logging a warning to help you keep your code clean so it does not break in the future. As the example shows, you just need to change the first param passed to the function. Change it from a number to an array with a list of param names. The array should have the same length as the number of params passed to the hook. |
|
@jaapjansma passing in an array has been recommended since 2017 - see
So this means that if you are calling the function in the non-recommended way then from 5.29 it will noisily accept it rather than silently accept it. This also means that tests will fail - as tests are intended to have a low tolerance for things like this & pick them up so you can take action. Deprecated warnings show up in test environments and developer environments. They don't show up when production level php error-logging is configured. This function is not officially part of our api & not officially support to be called from outside of core - so we don't have any docs outside the code comments to update on this - per the screenshot the code comments were updated 3 years ago. @colemanw I do think we should consider having a documented supported way for extensions to add hooks as it is useful. I'm not sure if it would involve this function though |
|
So the Civirules failures are unrelated? |
|
@jaapjansma they are related because the tests are tuned to fail on an php notices, warnings and deprecation notices - it's deliberate that tests & dev environments are more sensitive than production environments |
|
Ah ok, that make sense then. Excuses for my fuss about this. |
|
@jaapjansma no that's fine - I hope it makes sense now |
|
Thanks for your explanation! |
Overview
This fixes a fixme in
CRM_Utils_Hookto log a deprecation notice when hooks are invoked in the old way.Before
No warning.
After
Warning.
Technical Details
Also fixes some old array syntaxes.
Comments
I can't find any old-style hook invokes in core or extensions, except for CiviVolunteer which I've submitted civicrm/org.civicrm.volunteer#547