Only variables can be passed by reference warning in node.tpl.php

[markabur]

Description of the bug

My IDE is complaining that "Only variables can be passed by reference" when it sees lines such as this in template files:

hide($content['links']);

Steps To Reproduce

To reproduce the behavior:

1. Ensure PHPStorm has the PHP inspections "Pass parameter by reference" and "Invalid string offset usage" enabled
2. Open node.tpl.php
3. Scroll down to line 109
4. Hover over the error-highlighted code ($content['links'])

Actual behavior

It shows two warnings with the same wording, which is confusing, but also strange since there is no actual coding error in the first place. (The template works fine and does not generate a notice in watchdog.)

Expected behavior

In Drupal 7 there is no warning

Additional information

So, this is more of a problem/limitation with PHPStorm since it's reporting an error where is there isn't one, but on the other hand, it only happens in Backdrop and is easy to fix there.

I traced it back to layout-content-form.tpl.php -- if we stop treating $content as a string there, the inspection warning in node.tpl.php goes away! PHPStorm is getting confused because $content is used as both a string and an array in the global scope.

Here is the simplest possible way to reproduce the issue in PHPStorm:

Anyway, here is the fix for layout-content-form.tpl.php

Before:

<?php
/**
 * @file
 * Output the main layout content editor form.
 */
$actions = backdrop_render($form['actions']);
$content = backdrop_render($form['content']);
?>
<div id="layout-edit">
  <div class="layout-settings">
    <?php print backdrop_render_children($form); ?>
  </div>
  <div class="layout-wrapper clearfix">
    <?php print $content; ?>
  </div>
  <?php print $actions; ?>
</div>

After:

<?php
/**
 * @file
 * Output the main layout content editor form.
 */
$actions_rendered = backdrop_render($form['actions']);
$content_rendered = backdrop_render($form['content']);
?>
<div id="layout-edit">
  <div class="layout-settings">
    <?php print backdrop_render_children($form); ?>
  </div>
  <div class="layout-wrapper clearfix">
    <?php print $content_rendered; ?>
  </div>
  <?php print $actions_rendered; ?>
</div>

Alternatively, this is simpler and seems to work fine:

<?php
/**
 * @file
 * Output the main layout content editor form.
 */
?>
<div id="layout-edit">
  <div class="layout-settings">
    <?php print backdrop_render_children($form); ?>
  </div>
  <div class="layout-wrapper clearfix">
    <?php print backdrop_render($form['content']); ?>
  </div>
  <?php print backdrop_render($form['actions']); ?>
</div>

I don't know if there's a benefit to rendering those parts of the form prior to backdrop_render_children() -- if someone can let me know one way or the other, I can do a PR.

[indigoxela]

@markabur is there any way to reproduce a problem without PHPStorm? It might just be a false positive. Which version of PHPStorm, BTW?

[findlabnet]

I can confirm this IDE behavior, also any PHP variables within the template (such as $node, $classes etc) are marked as undefined by the same way. Was not too critical for me to investigate.

PhpStorm 2024.3.1.1 Build #PS-243.22562.233, built on December 19, 2024

[indigoxela]

... any PHP variables within the template (such as $node, $classes etc) are marked as undefined by the same way

That sounds like some missing or incomplete doc block somewhere in the rendering flow.

Is anyone of you @markabur or @findlabnet willing to dig a bit deeper? I'm assuming, this only pops up with some newer PHP versions?

[findlabnet]

PhpStorm have included bundled Drupal plugin, so maybe because it @markabur do not see nagging in D7 (not checked myself). About PHP version: I use 7.4 as base.

[avpaderno]

I tested it on PHP online editor. The following code cause a warning in recent PHP versions.

ini_set('display_errors', '1');
error_reporting(E_ALL);

$str = 'I love programming in PHP';
echo end(explode(' ', $str));

The following code does not cause any warning.

ini_set('display_errors', '1');
error_reporting(E_ALL);

$values = array('one' => 1, 'two' => 2);
unset($values['two']);

In the latter case, changing the code to the following one would not make sense.

ini_set('display_errors', '1');
error_reporting(E_ALL);

$values = array('one' => 1, 'two' => 2);
$value = $values['two'];
unset($value);

It would unset $value, not $values['two'].

[avpaderno]

In the documentation for unset(), the following is the example code to unset a global variable from inside a function.

function foo() 
{
    unset($GLOBALS['bar']);
}

$bar = "something";
foo();

That is also the only way to unset a global variable from inside a function. The following code would not work.

function destroy_foo() 
{
    global $foo;
    unset($foo);
}

$foo = 'bar';
destroy_foo();
echo $foo;

[indigoxela]

I did a little check with PHPstan - to get a "second opinion" - and it also fails to recognize, that all the variables have been set in the preprocessor and got extracted by theme_render_template.

It is a false positive, both tools seem unable to follow the rendering workflow in B.

If this isn't the case with PHPStorm and D7, there might be some tweak to get this working. But as I'm not using that IDE, I'm unable to figure out.

Changing the rendering workflow, to make an IDE (PHPStorm) or static analyzer (PHPStan) happy ... doesn't seem like an appropriate solution to me. :grin:

[markabur]

Oh, sorry for not chiming in earlier. I thought I was subscribed to e-mails for thread updates but I wasn't!

For the second option, one way to look at it is an optimization. Why create two new variables when we don't need to? Also, the pattern elsewhere in Backdrop is to use render() down in the output area rather than up at the top of the tpl. So this would also improve consistency.

It's only a false positive because $content gets redefined partway through the flow of the program. In the layout template it's an array, and then when layout-content-form.tpl.php gets loaded, it becomes a string. Of course PHPStorm doesn't know about the flow of the program, so it flags the discrepancy as an error.

[markabur]

... any PHP variables within the template (such as $node, $classes etc) are marked as undefined by the same way

That sounds like some missing or incomplete doc block somewhere in the rendering flow.

Sort of, though this is happening outside of a function so there is no doc block.

Is anyone of you @markabur or @findlabnet willing to dig a bit deeper? I'm assuming, this only pops up with some newer PHP versions?

I am using PHP 7.4. I don't know if that counts as newer or older at this point. :-)

It's true that other variables such as $node are marked as undefined. That is a different issue and I resolve it in my theme by adding variable type hints at the top of my tpl files. I think the IDE inspections are really valuable and like having a green light for my PHP files whenever possible.

The "Only variables can be passed by reference" warning is a different issue and cannot be fixed with a variable type hint.

In other words if I put this at the top of node.tpl.php:

/** @var Node $node */
/** @var array $content */

Then the undefined variable warning goes away for $node but the reference warning for $content doesn't. That was the first thing I tried and when it didn't work it took me down a deep rabbit hole that finally led to layout.tpl.php. Debugging by bisection still works!

PhpStorm have included bundled Drupal plugin, so maybe because it @markabur do not see nagging in D7 (not checked myself). About PHP version: I use 7.4 as base.

The reason it doesn't happen in Drupal is because Drupal doesn't have layout-content-form.tpl.php. However if you go into html.tpl.php (or any other php file) and add $content='some string'; at the top, then node.tpl.php will have the same reference warnings as Backdrop.

This is definitely not a bug in Backdrop, but this type of thing is certainly a potential bug in other contexts, so I think it's correct for the IDE to call it out.