CleanCode\UndefinedVariable thrown unnecessarily with preg_match_all()

[leewillis77]

• PHPMD version: dev-master (SHA b9eaa0f)
• PHP Version: 7.1.31
• Installation type: composer
• Operating System / Distribution & Version: MacOS X

Current Behavior

UndefinedVariable error thrown unnecessarily when variable is guaranteed to be defined by preg_match_all()

Expected Behavior

UndefinedVariable error not thrown for third argument to preg_match_all() when used.

Steps To Reproduce:

The following code triggers the following warnings:

test.php:5	Avoid using undefined variables such as '$matches' which will lead to PHP notices.
test.php:6	Avoid using undefined variables such as '$matches' which will lead to PHP notices.

However, PHP won't throw warnings in this case, so the error is spurious.

<?php

function test()
{
    preg_match_all('/./', 'Text', $matches);
    foreach ($matches[0] as $match) {
        echo $match . PHP_EOL;
    }
}

There's also an inconsistency as the same code when not wrapped in a function does not throw the warnings (as I'd expect), e.g. the following code (correctly IMHO) does not generate an UndefinedVariable warning from PHPMD:

<?php

preg_match_all('/./', 'Text', $matches);
foreach ($matches[0] as $match) {
    echo $match . PHP_EOL;
}

## Checks before submitting
* [x] Be sure that there isn't already an issue about this. See: [Issues list](https://github.com/phpmd/phpmd/issues)
* [x] Be sure that there isn't already a pull request about this. See: [Pull requests](https://github.com/phpmd/phpmd/pulls)
* [x] I have added every step to reproduce the bug.
* [x] If possible I added relevant code examples.
* [x] This issue is about 1 bug and nothing more.
* [x] The issue has a descriptive title. For example:  "JSON rendering failed on Windows for filenames with space".

[kylekatarnls]

Thanks. It will probably need PDepend adjustments to detect passing by reference and interpret it as variable assignment instead of variable access.

[leewillis77]

It's strange that the second example doesn't flag it though if that's the issue?

[kylekatarnls]

Most of the PHPMD rules assert that you follow the best practice: 1 file for 1 symbol (class or function). So rules start to list symbols and search into them. Rules like UndefinedVariable just skip the code that is not encapsulated.

As the project initially based its rules on PMD it even assumed that project uses OOP, this could be improved to support procedural-style files but it's not on our top-priorities list.

[MarkVaughn]

the second example could be valid like so:

<?php 
$matches = [];
include('secondexample.php');

So phpmd doesn't know the context of your example.

[ravage84]

Similar case with str_replace's &$count parameter:

$content = str_replace('__SALT__', $newKey, $content, $count);
if ($count === 0) {
    return;
}

[ravage84]

We are tracking this in pdepend/pdepend#444

[kylekatarnls]

This should be fixed by #784 as for str_replace issue.

You can yet test it requiring "phpmd/phpmd": "dev-feature/issue-672-passing-by-reference" in your composer.json.

[leewillis77]

@kylekatarnls Confirmed that fixes my original problematic code.

[kylekatarnls]

Thanks for confirmation, normally this should be released in 2.9.0.

[baumerdev]

The problem still exists for me with dev-master as soon as I'm within a namespace, e.g. the following still gives me the same warnings:

<?php
namespace X;
function test()
{
    preg_match_all('/./', 'Text', $matches);
    foreach ($matches[0] as $match) {
        echo $match . PHP_EOL;
    }
}

test.php:5	Avoid using undefined variables such as '$matches' which will lead to PHP notices.
test.php:6	Avoid using undefined variables such as '$matches' which will lead to PHP notices.

[kylekatarnls]

Indeed the FQN wrongly resolves to X\preg_match_all

[kylekatarnls]

Fix proposed.

[VladimirCatrici]

Same here for openssl_encrypt function and $tag parameter: https://www.php.net/manual/en/function.openssl-encrypt.php