dg / bypass-finals

Removes `final` and `readonly` keywords from source code on-the-fly and allows mocking of final methods and classes. It can be used together with any test tool such as PHPUnit or Mockery.
Other
487 stars 30 forks source link

Fix issue #52 #54

Closed oruborus closed 6 months ago

oruborus commented 7 months ago

Suppresses warning triggered by stat/lstat when file does not exit.

By injecting another error handler which resets error reporting the suppressed warning gets caught and does not appear in error logs.

davideprevosto commented 7 months ago

@oruborus I used your code on a Laravel 10 project is using this repo: https://github.com/nunomaduro/mock-final-classes Your fix worked for me. Thank you.

oruborus commented 7 months ago

Just to add an argument for this approach:

"The problem is caused by an incorrectly written handler, the solution is to fix the handler, not this library."

Originally posted by @dg in https://github.com/dg/bypass-finals/issues/45#issuecomment-1875621116

I agree with the statement that error handlers should be correctly setup.

But in this special case stat/lstat should be totally muted. This behaviour would then be the same as in the php-src itself:

#define IS_EXISTS_CHECK(__t) ((__t) == FS_EXISTS  || (__t) == FS_IS_W || (__t) == FS_IS_R || (__t) == FS_IS_X || (__t) == FS_IS_FILE || (__t) == FS_IS_DIR || (__t) == FS_IS_LINK || (__t) == FS_LPERMS)

// in php_stat()

if (IS_EXISTS_CHECK(type)) {
    flags |= PHP_STREAM_URL_STAT_QUIET;
}

// ...

if (ZSTR_LEN(filename) && !IS_EXISTS_CHECK(type)) {
   php_error_docref(NULL, E_WARNING, "Filename contains null byte");
}

// ...

if (!IS_EXISTS_CHECK(type)) {
   php_error_docref(NULL, E_WARNING, "%sstat failed for %s", IS_LINK_OPERATION(type) ? "L" : "", ZSTR_VAL(filename));
}

https://github.com/php/php-src/blob/fd73681c86bb69163abacc202f96306864f82cbe/ext/standard/filestat.c#L702 https://github.com/php/php-src/blob/fd73681c86bb69163abacc202f96306864f82cbe/ext/standard/filestat.c#L789-L791 https://github.com/php/php-src/blob/fd73681c86bb69163abacc202f96306864f82cbe/ext/standard/filestat.c#L765-L767 https://github.com/php/php-src/blob/fd73681c86bb69163abacc202f96306864f82cbe/ext/standard/filestat.c#L805-L807

If we can observe PHP_STREAM_URL_STAT_QUIET to be defined, stat/lstat should never issue warning, not even a suppressed one.

oruborus commented 7 months ago

I am currently revising my aproach. It occured to me that most of the overloaded functions triggered several warnings that are not triggered in the native versions.

I am positive that this could be achived with the only caveat that some of the overloaded functions will trigger an E_USER_WARNING instead of E_WARNING. The message will be the same.

MarcoLeko commented 6 months ago

Really curious what will come out here - Your fork does work and I hope the changes will be merged to the original repo or at least an alternative solution (like example configuration like phpunit.xml.dist) can be provided for this issue

dg commented 6 months ago

@oruborus Thanks for the beautifully crafted PR, rationale and solution.