Problem/Motivation

Upgrading to PHP 8.4 and am running code scans to verify compatibility. There is a warning:

FILE: /var/www/html/web/modules/contrib/redis/src/Client/Predis.php
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 62 | WARNING | Since PHP 7.0, functions inspecting arguments, like func_get_args(), no longer report the original value as passed to a parameter, but will instead provide
    |         | the current value. The parameter "$options" was used, and possibly changed (by reference), on line 52.
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Steps to reproduce

Run phpcs with 8.4 standard compatibility.

Issue fork redis-3616368

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jennypanighetti created an issue. See original summary.

jennypanighetti opened merge request !102

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs work

That is not the same, the code there is explicitly to support options as an array and separate parameters, because that is how the underlying library works.

leandro713’s picture

leandro713 commented

I had a look at Berdir's feedback. I think this can be fixed with a small
change by capturing the original arguments before $options is processed,
while keeping support for both array options and separate parameters.

I'm going to prepare a small follow-up change based on that approach.

leandro713’s picture

leandro713 commented

I updathed the change so that func_get_args() is
called before $options is processed.

This keeps the original argument list intact, including any additional
parameters passed after $options, while preservin the existing array
normalization logic.

The previous func_get_args() call inside the else branch is no longer
needed, because the arguments are now captured once at the beginning of the
method. Removing it does not change the behaviour, it only avoids reading the
arguments after $options may have been processed.

leandro713’s picture

leandro713 commented
Status: Needs work » Needs review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Version: 2.0.0-alpha2 » 2.x-dev
Status: Needs review » Needs work

This report is clearly bogus, it just doesn't understand it well enough. options is never changed and definitely not when func_get_args() is called. I'm sure it's only minor overhead but it's still overhead to call it first and then replace it again. Id' rather either ignore this report or silence it.

leandro713’s picture

leandro713 commented

Thanks, that makes sense.

I was treating the PHPCompatibility warning itself as something worth
avoiding, rather than suggesting that $options is actually modified at
runtime.

If this is a false positive and changing the argument handling would only
add unnecessary complexity, then silencing this specific report seems like
the better approach.

I'll look into whether we can suppress just this PHPCompatibility warning
without changing the current behaviour.