Problem/Motivation

When using PHP 7.4, these notices appear if no connection to Varnish could be established:

Notice: Trying to access array offset on value of type bool in varnish_get_status() (line 258 of .../varnish/varnish.module).
Notice: Trying to access array offset on value of type null in varnish_get_status() (line 258 of .../varnish/varnish.module).

Errors would also occur if the 'sockets' extension was not loaded:

Warning: array_shift() expects parameter 1 to be array, bool given in varnish_get_status() (line 257 of /var/www/html/sites/all/modules/contrib/varnish/varnish.module).

And after fixing the one above:

Warning: Invalid argument supplied for foreach() in varnish_requirements() (line 20 of .../varnish/varnish.install).

Steps to reproduce

_varnish_terminal_run() returns FALSEif the 'sockets' extension was not loaded. If no connection could be established, $stat['status'] will be FALSE.

In this case, the code in varnish_get_status() does not handle such a return value. This leads to the PHP notices listed above.

    $status = _varnish_terminal_run(array('status'));
    $terminals = explode(' ', variable_get('varnish_control_terminal', '127.0.0.1:6082'));
    foreach ($terminals as $terminal) {
      $stat = array_shift($status);
      $results[$terminal] = ($stat['status']['code'] == 200) ? VARNISH_SERVER_STATUS_UP : VARNISH_SERVER_STATUS_DOWN;
    }

Proposed resolution

return NULL; if $status is FALSE.

Check if isset when looping for status is up or down.

Add code to install file to check for the sockets requirement.

Remaining tasks

Review and test patch
Commit

CommentFileSizeAuthor
#11 varnish-php74-3187031-6.patch2.46 KBmax lovric
#9 varnish-php74-3187031-5.patch348.18 KBmax lovric
#4 varnish-php74-3187031-4.patch2.17 KBsolideogloria
#3 varnish-php74-3187031-3.patch1.5 KBsolideogloria
#2 varnish-php74-3187031-2.patch1.52 KBsolideogloria

Comments

solideogloria created an issue. See original summary.

solideogloria’s picture

solideogloria commented
Status: Active » Needs review
StatusFileSize
new varnish-php74-3187031-2.patch1.52 KB
solideogloria’s picture

solideogloria commented
StatusFileSize
new varnish-php74-3187031-3.patch1.5 KB

Uploaded wrong patch file. This one returns NULL instead of VARNISH_SERVER_STATUS_DOWN, when the sockets extension is not found.

solideogloria’s picture

solideogloria commented
StatusFileSize
new varnish-php74-3187031-4.patch2.17 KB

Updated return types in function doc comments.

Consideration: these patches do not take advantage of static caching in varnish_get_status() because of the return value. I figured it'd be better to keep existing behavior, and the sockets won't actually be called in this case anyway. Since that's why the cache is there, it doesn't seem necessary to cache empty or NULL values.

gerzenstl’s picture

gerzenstl commented

The patch works fine.

For the tests I used:
* PHP: 7.4.16
* Drupal: 7.81
* Varnish module: 1.9

pobster’s picture

pobster commented

For the requirements check, it'd be better if you outright check using https://www.php.net/manual/en/function.extension-loaded.php for sockets being present?

Also, just an opinion, but this;

     $status = _varnish_terminal_run(array('status'));
+    if (!$status) {
+      return NULL;
+    }

Would be a lot neater if it just wrapped and returned the empty array like this;

-     $status = _varnish_terminal_run(array('status'));
+    if (!$status = _varnish_terminal_run(array('status'))) {
       $terminals = explode(' ', variable_get('varnish_control_terminal', '127.0.0.1:6082'));
       foreach ($terminals as $terminal) {
         $stat = array_shift($status);
-        $results[$terminal] = ($stat['status']['code'] == 200) ? VARNISH_SERVER_STATUS_UP : VARNISH_SERVER_STATUS_DOWN;
+        $results[$terminal] = (isset($stat['status']['code']) && $stat['status']['code'] == 200) ? VARNISH_SERVER_STATUS_UP : VARNISH_SERVER_STATUS_DOWN;
+     }
     }

Then there's no need to alter the return typehint.

steven jones’s picture

steven jones commented
Status: Needs review » Reviewed & tested by the community

Patch seems to work fine. I'm marking as RTBC because while not perfect, it does at least solve the problem of the notices.

The module maintainer can decide if they want to reject the fix in favour of a better/different solution.

steven jones’s picture

steven jones commented
max lovric’s picture

max lovric commented
StatusFileSize
new varnish-php74-3187031-5.patch348.18 KB

Ignore

max lovric’s picture

max lovric commented
max lovric’s picture

max lovric commented
StatusFileSize
new varnish-php74-3187031-6.patch2.46 KB

7.x-1.1 patch

  • drupalfan79 committed c70ec53 on 7.x-1.x 
    git commit -m 'Issue #3187031 by solideogloria, Max Lovric: PHP 7.4...
tree2009’s picture

tree2009 commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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