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

RequestSchema constructor fails silently on bad filename #1216

Closed
StrykeSlammerII opened this issue Mar 24, 2023 · 4 comments
Closed

RequestSchema constructor fails silently on bad filename #1216

StrykeSlammerII opened this issue Mar 24, 2023 · 4 comments
Assignees
Labels
confirmed bug Something isn't working
Milestone

Comments

@StrykeSlammerII
Copy link

Current Behavior:

Sending an invalid filename to new RequestSchema($path); appears to result in an empty Schema.
(There should be a warning in the PHP error log, but I'm unable to confirm this right now.)

The presenting issue in chat was $transformer->transform returning an unexpected blank array, due to a bad path or filename in the $schema sent to $transformer.

Expected Behavior:

The UF docs use a bad filename as an example of a runtime error.
UF should catch this and throw a runtime error, as PHP appears to treat this with just a warning.

Discussion:

If I'm reading the code correctly, the RequestSchema constructor uses file_get_contents($path). Per PHP.net, An E_WARNING level error is generated if filename cannot be found.
That warning doesn't seem to be caught by UF, so it should just end up in the PHP error log.

I'm surprised that PHP only creates an E_WARNING here, but that's outside this project's scope.

I'm currently having some troubles with my PHP error log, and getting intermittent UF error-renderer failures as well. Potentially this error should be more visible and I simply have something additional broken.

Possible Solutions:

  1. If this is "working as-intended", I'll be happy to add a note to the Learn repository as a clue for others.
  2. Additional error-handling could be added to new RequestSchema($path); to escalate this up from E_WARNING to something more noticeable.
  3. Possibly I'm misreading the code and my UF error handler is more broken than I realize. If new RequestSchema('schema://BadDirectory/ThisIsNotARealFile.yaml'); shows an error rather than failing silently, feel free to close this and I will fix my install instead :)
@lcharette
Copy link
Member

lcharette commented Mar 25, 2023

file_get_contents should return false :

The function returns the read data or false on failure.

Which should throw an exception in YamlFileLoader
https://github.com/userfrosting/framework/blob/80bb8080506c16431a5e81080fafc301d3dc1e35/src/Support/Repository/Loader/YamlFileLoader.php#L31-L33

You could test it manually using

$loader = new YamlFileLoader('BadDirectory/ThisIsNotARealFile.yaml');
$items = $loader->load();

and post the result.

@lcharette
Copy link
Member

lcharette commented Mar 25, 2023

I think RequestSchema should load with the $skipMissing flag as false here :
https://github.com/userfrosting/framework/blob/80bb8080506c16431a5e81080fafc301d3dc1e35/src/Fortress/RequestSchema.php#L42

$skipMissing is true by default here :
https://github.com/userfrosting/framework/blob/80bb8080506c16431a5e81080fafc301d3dc1e35/src/Support/Repository/Loader/FileRepositoryLoader.php#L78-L82

And won't throw an exception here :
https://github.com/userfrosting/framework/blob/80bb8080506c16431a5e81080fafc301d3dc1e35/src/Support/Repository/Loader/FileRepositoryLoader.php#L53

Moving it to false would be a big change that could impact other system though. I should probably test for this situation on the V5 branch.

@lcharette
Copy link
Member

lcharette commented Mar 25, 2023

Per PHP.net, An E_WARNING level error is generated if filename cannot be found. That warning doesn't seem to be caught by UF, so it should just end up in the PHP error log.

FYI, most PHP install will suppress E_WARNING by default. try/catch doesn't work because a warning is not an exception, so there's no way to handle this, only the false return type.

lcharette added a commit to userfrosting/framework that referenced this issue Mar 29, 2023
@lcharette
Copy link
Member

I fixed this in userfrosting/framework@4bfc0fc on the V5 branch. A FileNotFoundException will be thrown if the path is not found, as shown in new test. Note the path can still be null without an exception being thrown.

I don't plan on fixing for V4, but a PR would be considered.

@lcharette lcharette self-assigned this Mar 29, 2023
@lcharette lcharette added the confirmed bug Something isn't working label Mar 29, 2023
@lcharette lcharette added this to the 5.0 milestone Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants