When creating a Drupal 8 module, I attempted to use the views hook hook_views_pre_build(), which has the following function signature according to views.api.php:
function hook_views_pre_build(ViewExecutable &$view) {)

I implemented the hook in the following way:

function mymodule_views_pre_build(ViewExecutable &$view) {
  // Do nothing.
}

Simply implementing the hook generates the following error:

Warning: Parameter 1 to mymodule_views_pre_build() expected to be a reference, value given in Drupal\Core\Extension\ModuleHandler->invokeAll() (line 285 of core/lib/Drupal/Core/Extension/ModuleHandler.php).

This indicates that when Views runs ModuleHandler->invokeAll(), it doesn't pass the $view argument by reference. Modifying the call to invokeAll(), which is at line 1029 of /core/modules/views/lib/Drupal/views/ViewExecutable.php fixes the error.

Original:

    // Let modules modify the view just prior to building it.
    $module_handler = \Drupal::moduleHandler();
    $module_handler->invokeAll('views_pre_build', array($this));

Modification:

    // Let modules modify the view just prior to building it.
    $module_handler = \Drupal::moduleHandler();
    $module_handler->invokeAll('views_pre_build', array(&$this));

I'd be happy to submit a patch for this, but I'd first like to get some feedback about this approach—I have a hazy recollection of passing arguments by reference in this manner as being deprecated. Additionally, if this change is made for hook_views_pre_build(), it will need to be made for other hooks as well.

CommentFileSizeAuthor
#18 views-hooks-2073507-18.patch6.06 KBgrasmash
#14 views-hooks-2073507-14.patch6.1 KBgrasmash
#6 views-hooks-2073507-6.patch5.68 KBgrasmash
#2 views-hooks-2073507-2.patch3.5 KBgrasmash
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

grasmash’s picture

grasmash CreditAttribution: grasmash commented

More thought on the issue of passing by reference at call time. @see http://php.net/manual/en/language.references.pass.php:

Note: There is no reference sign on a function call - only on function definitions. Function definitions alone are enough to correctly pass the argument by reference. As of PHP 5.3.0, you will get a warning saying that "call-time pass-by-reference" is deprecated when you use & in foo(&$a);. And as of PHP 5.4.0, call-time pass-by-reference was removed, so using it will raise a fatal error.

Nevertheless, I believe that calling $module_handler->invokeAll(array(&$view)) is valid because the array is not being passed by reference, just one of the array items. I can confirm that this does not cause a fatal error in PHP 5.4.

The odd thing here is that I thought objects were always passed by reference in PHP 5.3. Is that not right?

grasmash’s picture

grasmash CreditAttribution: grasmash commented
Status: Active » Needs review
FileSize
views-hooks-2073507-2.patch3.5 KB

Attaching patch.

grasmash’s picture

grasmash CreditAttribution: grasmash commented

After some more research, I found this:
http://stackoverflow.com/questions/9331519/php-object-by-reference-in-ph...

Which explains that by default, objects in PHP 5 are not really passed by reference or by value—they're passed by identifier (a reference to a reference).

It follows that we do not actually need to pass objects by reference when calling invokeAll() in Views. Passing the variables as-is will allow hooks to modify the passed $view object with accessors.

There are two paths to take:

  1. Modify Views to actually pass objects by reference, OR
  2. Modify hook documentation by removing '&' prefix from parameters (so that PHP doesn't expect a reference)

The problem with leaving Views as-is will be that we will still have this problem with non-object parameters, of which there are a few.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

Since php 5 objects get passed by reference, remove the & from both

grasmash’s picture

grasmash CreditAttribution: grasmash commented

Since php 5 objects get passed by reference, remove the & from both

That isn't technically true. If they were, the PHP warning would not appear. @see http://php.net/manual/en/language.oop5.references.php:

One of the key-points of PHP 5 OOP that is often mentioned is that "objects are passed by references by default". This is not completely true.

@larowlan Regardless of the technicalities, removing the "&" from the function signature may be a good solution for objects, but it doesn't address views hooks that are intended pass strings and arrays by reference. Notably hook_views_pre_view().

grasmash’s picture

grasmash CreditAttribution: grasmash commented
Issue tags: +VDC
FileSize
views-hooks-2073507-6.patch5.68 KB

Rerolling. Fixing Views API documentation, passing strings and arrays by reference. Adding tag at @timplunkett's request.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, views-hooks-2073507-6.patch, failed testing.

grasmash’s picture

grasmash CreditAttribution: grasmash commented
Status: Needs work » Needs review
Issue tags: +VDC

#6: views-hooks-2073507-6.patch queued for re-testing.

grasmash’s picture

grasmash CreditAttribution: grasmash commented
Title: Views calls to ModuleHandler->invokeAll() do not pass object by reference » Parameters passed and documented incorrectly in Views hooks

Changing title.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

The current patch looks good. Objects don't need references at all, while strings/arrays do need if they are intended to be changed. I don't really get why you would like to change a display ID?

grasmash’s picture

grasmash CreditAttribution: grasmash commented

I changed the display ID to be passed by reference because the function signature in views.api.php indicates that the display_id is expected to be passed by reference. It is documented in the same way in D7 Views.

grasmash’s picture

grasmash CreditAttribution: grasmash commented

@dawehner would you say this issue is RTBC?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Could we fix that here, as there is no value in there at all. It would just break everything if it is changed.

grasmash’s picture

grasmash CreditAttribution: grasmash commented
FileSize
views-hooks-2073507-14.patch6.1 KB

@dawehner Re-rolled patch. $display_id is no longer passed by reference. This required changing the example for hook_views_preview() in views.api.php. At @merlinofchaos' suggestion, I added an example that modifies the $args array.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.php
@@ -1444,7 +1444,8 @@ public function preExecute($args = array()) {
+    $module_handler = \Drupal::moduleHandler();
+    $module_handler->invokeAll('views_pre_view', array($this, $display_id, &$this->args));

Is there a reason for this change? We can still call the module handler directly ...

grasmash’s picture

grasmash CreditAttribution: grasmash commented

Just being consistent with the other calls to the invokeAll() method in the same file. Every other method in that file sets $method_handler first.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Well, the reason for the other places are that the variable is reusable ...

grasmash’s picture

grasmash CreditAttribution: grasmash commented
FileSize
views-hooks-2073507-18.patch6.06 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Perfect!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Great catches.

Committed and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.