function foo($paramUnusedBeforeUsed, $paramUsed, $paramUnused) {
    $index = 1;
    echo $paramUsed;
}

Both the unused $paramUnusedBeforeUsed and $paramUnused as well as the unused $index currently have the same error code: UnusedVariable.

While I realize that the allowUnusedFunctionParameters and allowUnusedParametersBeforeUsed properties exist, those can only be set for a complete code base, while differentiation via error codes gives more control, i.e. you can include/exclude select files for certain error codes.

<rule ref="VariableAnalysis"/>
<rule ref="VariableAnalysis.CodeAnalysis.VariableAnalysis.ErrorCode">
    <exclude-pattern>/src/specific-directory/*\.php$</exclude-pattern>
    <exclude-pattern>/src/specific-file\.php$</exclude-pattern>
</rule>

Unused parameters cannot always be "solved" as the method signature of a class method where the class extends a parent and/or implements an interface, has to be compatible with the parent class/interface.

Unused variables defined within a function is another matter.

With that in mind, I'd like to propose to split the UnusedVariable error code into three distinct error codes:

• UnusedParameterBeforeUsed
• UnusedParameter[AfterUsed]
• UnusedVariable

This would also pave the way to removing the allowUnusedFunctionParameters and allowUnusedParametersBeforeUsed properties.

As the next version is intended to be a major tag, now would probably be a good time to make such a change.

To make the BC-break smaller, I'd like to suggest for the properties to be deprecated in this major and only removed in the next major release.

For the time being, the properties could then still be checked to determine whether the UnusedParameter... errors should be thrown or not, giving people some time to switch over to using the error code exclusion instead of the properties.

Hi

Yesterday when testing VA, I found that unused function parameters are not always discovered. Here is the corresponding code part:

	private function get_hidden_span($user_type, $text, $testvar)
	{
		global $config;
		
		if (!$config['lfwwh_create_hidden_info'])
		{
			return '';
		}
		return '<span class="lfwwh_info_' . (($user_type != USER_IGNORE || $config['lfwwh_disp_bots'] == 1) ? 'u' : 'b') . '" style="display: none;">' . $text . '</span>';
	}

VA should actually find $testvar here, which is not the case. After many attempts, I found that VA found this variable when the allowUnusedParametersBeforeUsed switch was set to 0.

There would be other code examples if desired.

DrupalPractice.CodeAnalysis.VariableAnalysis.UnusedVariable gives a false positive for the $uri variable in the following example:

\Drupal::httpClient()->get($input, [
  'on_stats' => function (TransferStats $stats) use (&$uri) {
    $uri = $stats->getEffectiveUri();
  },
]);

Another false positive occurs for the following code snippet which modifies an array by reference:

 80 | WARNING | Unused variable $entry.
    |         | (DrupalPractice.CodeAnalysis.VariableAnalysis.UnusedVariable)

foreach ($derivatives as &$entry) {
  $entry += $base_plugin_definition;
}

The sniff doesn't detect that the $uri variable is actually in use since it is passed by reference to the anonymous function.

Originally posted in https://www.drupal.org/project/coder/issues/3065679.

I'm not completely sure why, but when I tried to run the unit tests locally, most were failing.

This minor tweak in how PHPCS is instantiated fixes it. It also removes the need for the getSniffFiles() method as there is only one sniff in this standard anyway.

I have a base class like this:

abstract class Shortcode {

	abstract public static function callback( $attrs, $content = '' );
}

And a subclass like this:

class Thumbnail_Gallery extends Shortcode {

	public static function callback( $attrs, $content = '' ) {
		return '';
	}
}

Since v2.8.2, $attrs and $content are (unexpectedly?) flagged as unused variables. However, removing them causes a fatal error.

You do not need to register/create a variable before assigning indexes. Instead, you can directly create a variable as an array. You can also see this in PHP's array example 12.

$a['index'] = 'value';
print_r( $a ); // Array ( [index] => value )

$b[] = 'value';
print_r( $b ); // Array ( [0] => value )

$c['index'][]['deep'] = 'value';
print_r( $c ); // Array ( [index] => Array ( [0] => Array ( [deep] => value ) ) )

However, this warning is thrown at the variable assignment, and whenever later you reuse the variable:

Variable $a is undefined.
(VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedVariable)

Now, do keep in mind that PHP's documentation doesn't explicitly state if and why this would work. So, whether this is a best practice or not is up for debate, but I don't think this specific warning should be thrown—it should be mitigated elsewhere.

As for examples in the wild, I found one in WordPress:

• https://github.com/WordPress/WordPress/blob/7112ee71329533c9a168e4736582f537e30f7068/wp-admin/edit-comments.php#L286

Say I have this function:

function updateGlobal($newVal) {
  global $myGlobal;
  $myGlobal = $newVal;
}

I will get the following message:

Unused global variable $myGlobal.

It seems like this is an erroneous message. Or at least it should be configurable.

Currently, I'm able to get around the warning this way:

function updateGlobal($newVal) {
  global $myGlobal;
  $myGlobal;
  $myGlobal = $newVal;
}

But this is not ideal.

Take the following code as an example:

<?php
namespace Whatever;

use Symfony\Component\HttpKernel\Exception\HttpException;
use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface;
use Laravel\Lumen\Exceptions\Handler as ExceptionHandler;

final class LumenExceptionHandler extends ExceptionHandler {
    public function render($request, \Exception $e) {
        $status = 500;
        if ($e instanceof HttpExceptionInterface) {
            $status = $e->getStatusCode();
        }

        return response()->json(['message' => $e->getMessage()], $status);
    }
}

The render function takes 2 arguments. A request and an exception. We have to accept these two arguments because we're overwriting a method and we want to match it's signature.

In our implementation, we don't actually care about the $request param. PHPCS gives me a warning because $request is unused.

I want to be able to tell PHPCS that this argument is unused and that this is OK.

With Rubocop (a ruby linter), I can prefix an argument with _ to mark it as unused and to let the linter know that this is OK. Here's the relevant doc: https://rubocop.readthedocs.io/en/latest/cops_lint/#lintunusedmethodargument

We could have something like this:

public function render($_request, \Exception $e) {
  // ...
}

This would not generate any warnings since we've explicitly said that the argument should be unused.

GH Actions: fix use of deprecated set-output

GitHub has deprecated the use of set-output (and set-state) in favour of new environment files.

This commit updates workflows to use the new methodology.

Refs:

• https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/
• https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#environment-files

~~GH Actions: update the xmllint-problem-matcher~~

~~The xmllint-problem-matcher action runner has released a new version which updates it to use node 16. This gets rid of a warning which was shown in the action logs.~~

~~Note: I've suggested to the author to use long-running branches for the action runner instead, which would make this update redundant, but no telling if or when they'll respond to that, let alone if they will follow my suggestion.~~

Refs:

• https://github.com/korelstar/xmllint-problem-matcher/releases/tag/v1.1

🆕 GH Actions: harden the workflow against PHPCS ruleset errors

If there is a ruleset error, the cs2pr action doesn't receive an xml report and exits with a 0 error code, even though the PHPCS run failed (though not on CS errors, but on a ruleset error).

This changes the GH Actions workflow to allow for that situation and still fail the build in that case.

👉🏻 Note: this won't get rid of all warning yet as a lot of predefined action runners also use set-output, but most of those are in the process of updating and/or have released a new version already, so the other warnings should automatically disappear over the next few weeks.

After upgrading from v2.9.0 to v2.10.2 I got a lot of false positives and I had to downgrade again. For example:

$var = NULL;

function foo(){
    global $var;
    // var used here
}

causes a VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable.

Also

if(($val = func()) !== FALSE){
    $val->foo();
}

causes a VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedVariable.

Given the following code

  $suggestion = 'block';
  while ($part = array_shift($parts)) {
    $suggestions[] = $suggestion .= '__' . strtr($part, '-', '_');
  }

$suggestion will be marked as unused.

Where a variable is available within the File scope, it will not be available within a function scope without the use of global $varname;.

At the moment it's possible to either set:

• validUndefinedVariableNames, but this will incorrectly apply within function scope where the variable is not available to be used; or
• allowUndefinedVariablesInFileScope, but this is an on/off value and may not be accurate.

It should be possible to define which variables are allowed to be used within the file scope.

As reported in https://github.com/sirbrillig/phpcs-variable-analysis/pull/234

Array functions like array_merge() and array_values() are "expensive".

Hey,

Do you have an ETA for when v3 can have a stable tag? arrow functions are not possible if I recall correctly due to constrains in this package

Regards, Levi

Trying to avoid this example BAD (due to refactoring, or simple human error):

$has_free_tag = has_tag( 'free' );

if ( $has_free_tag ) {
  ...
}

BETTER:

if ( has_tag( 'free' ) ) {
  ...
}

function whileLooopAssignWithUndefinedShift() {
  $suggestions = [];
  while ($part = array_shift($parts)) { // undefined variable parts
    $suggestions[] = $part;
  }
  return $suggestions;
}

$parts is not marked as undefined. This is happening because array_shift counts as an assignment... but clearly not all assignments should be counted as definitions if the expected variable is an array or object.

This may mean that we need to annotate all the pass-by-reference functions in both PHP and WordPress by what types of variables they expect. 😩

In PHP there are a number of functions which can set variables in the current scope. Sometimes by reference to a passed parameter (first examples), sometimes just plainly in the current scope (second examples).

While the first set of examples is handled correctly by the sniff, the second set of examples is not.

Now, I'm not saying it will be easy to handle this or that I have a ready-made solution for this, but I figured opening an issue as a reminder to have a look at this at some point would be a good idea nevertheless.

1. Functions setting a variable by reference.

	parse_str($str, $output);
	echo $output['first']; 

	preg_match('`.*`', $first, $matches);
	var_dump($matches);

The sniff correctly doesn't throw an error for $matches or $output being used.

2. Functions setting variables in the current scope.

function foo() {
	$str = "first=value&second=value&arr[]=foo+bar&arr[]=baz";
	parse_str($str);
	echo $first;  // value
	echo $second;  // value
	echo $arr[0]; // foo bar
	echo $arr[1]; // baz
	
	$array = [
		'a' => 1,
		'b' => 'test',
	];
	extract($array);
	echo $a, $b;
}

The sniff as-is will throw errors for use of "undefined" $first, $second, $arr, $a, $b variables, while these are in actual fact all defined.