Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue #52 #54

Merged
merged 2 commits into from
May 16, 2024
Merged

Fix issue #52 #54

merged 2 commits into from
May 16, 2024

Conversation

oruborus
Copy link
Contributor

@oruborus oruborus commented Apr 8, 2024

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.

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
Copy link

@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
Copy link
Contributor Author

oruborus commented Apr 9, 2024

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 #45 (comment)

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.

This adds `file_exists`, `is_writable`, `is_readable`, `is_executable`,
`is_file` and `is_link` to the test.
@oruborus oruborus marked this pull request as draft April 12, 2024 19:09
@oruborus
Copy link
Contributor Author

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
Copy link

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
Copy link
Owner

dg commented May 16, 2024

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

@dg dg marked this pull request as ready for review May 16, 2024 17:14
@dg dg merged commit 8616a09 into dg:master May 16, 2024
dg pushed a commit that referenced this pull request May 16, 2024
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.

As certain testing frameworks establish error handlers to pick up
on suppressed errors, exceptions and warnings, the warning generated
by `stat`/`lstat` when provided an unknown path is recorded.
dg pushed a commit that referenced this pull request May 16, 2024
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.

As certain testing frameworks establish error handlers to pick up
on suppressed errors, exceptions and warnings, the warning generated
by `stat`/`lstat` when provided an unknown path is recorded.
dg pushed a commit that referenced this pull request May 16, 2024
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.

As certain testing frameworks establish error handlers to pick up
on suppressed errors, exceptions and warnings, the warning generated
by `stat`/`lstat` when provided an unknown path is recorded.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants