Codeception / AspectMock

The most powerful and flexible mocking framework for PHPUnit / Codeception.
MIT License
790 stars 129 forks source link

Error when mocking built-in function with pass-by-reference argument and argument with default value (PHP 8) #201

Open jimbonator opened 2 years ago

jimbonator commented 2 years ago

This is for AspectMock 4.1.1.

During our migration from PHP 7.4 to PHP 8.1, we've encountered this pattern a few times:

When our PHP-Unit tests mock a PHP built-in function that has at least one argument passed by reference and one argument with a default value, the mock fails with the error ParseError: syntax error, unexpected token "=", expecting ")".

The stack track looks like this:

/tmp/flockoipmSG:4
/petabox/www/common/vendor/codeception/aspect-mock/src/AspectMock/Core/Mocker.php:275
/petabox/www/common/vendor/codeception/aspect-mock/src/AspectMock/Core/Registry.php:38
/petabox/www/common/vendor/codeception/aspect-mock/src/AspectMock/Test.php:248

Examining the temporary file reveals the problem:

$ cat /tmp/flockoipmSG
<?php
namespace Atomic;
if (!function_exists('Atomic\flock')) {
    function flock($p0, int $p1, &$p2 = null=NULL) {
         $args = [];
         switch(count(func_get_args())) {
             case 3: $args = [$p0, $p1, &$p2]; break;
             case 2: $args = [$p0, $p1]; break;
             case 1: $args = [$p0]; break;
         }
         if (($__am_res = __amock_before_func('Atomic','flock', $args)) !== __AM_CONTINUE__) {
             return $__am_res;
         }
         return call_user_func_array('flock', $args);
    }
}

The function declaration has two default values for an argument (function flock($p0, int $p1, &$p2 = null=NULL).

Other functions which have given us this problem are:

The last one is interesting because, unlike the others, the passed-by-reference arguments don't have default values. Only the final argument does:

stream_select(
    ?array &$read,
    ?array &$write,
    ?array &$except,
    ?int $seconds,
    ?int $microseconds = null
): int|false

That's what leads me to believe the problem is the existence of both in the function signature, but not necessarily that a single argument needs to be both pass-by-reference and have a default value for this to fail.

We don't see this under PHP 7.4, so I assume this related to the PHP version.

BotRoy commented 2 years ago

Hi @jimbonator , I'm testing out your goaop/parser-reflection pull request to make AspectMock php 8 compatible (running php 8.0.17, I made your PR into a patch for goaop/parser-reflection, and I'm using --ignore-platform-reqs).

I see this issue too, and I found a temporary fix. I don't really understand all the code surrounding this, so there's a decent chance I broke something else with this.

For example, the exec function, when mocked, contains the line function exec(string $p0, &$p1 = null=NULL, &$p2 = null=NULL) {

Based on the man page for exec, it seems that PHP is appending the = null. In src/AspectMock/Intercept/FunctionInjector.php, aspect-mock appends =NULL because the function is internal and the parameter is optional:

if ($internal && $parameter->isOptional()) {
    $text .= "=NULL";
}

If I remove the above code, the errors go away and no additional errors/failures are introduced. I am still trying to understand this code, the commit/PR it was introduced in, what other things this could possibly break, and what a better fix is.