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

New method for getting the content #734

Closed
marineusde opened this issue Jul 29, 2024 · 4 comments
Closed

New method for getting the content #734

marineusde opened this issue Jul 29, 2024 · 4 comments

Comments

@marineusde
Copy link

In the FileGetContents-Class is this code:

$response = file_get_contents($uri);

That do some problems in unit tests, cause you cant mock or fake the schema files. My example idea is this in Laravel, if someone want to test the code later (or extend the file in a seperate file):

$fileGetContents = new class extends FileGetContents
{
  protected function getContents(string $uri): false|string
  {
     if (!File::exists($uri)) {
       throw new ResourceNotFoundException('JSON schema not found at ' . $uri);
     }

     return File::get($uri);
  }
};

$factory = new Factory();
$uriRetriever = $factory->getUriRetriever();
$uriRetriever->setUriRetriever($fileGetContents);

In the unit test, you can mock it:

File::shouldReceive('exists')
  ->with('test.json')
  ->andReturn(true);

  File::shouldReceive('get')
  ->with('file://test.json')
  ->andReturn('{...}');
@marineusde
Copy link
Author

I separated the methods for this issue in #735

@DannyvdSluijs
Copy link
Collaborator

I've already left some question in the PR bu now that I'm seeing the code here I wonder why not create a mock or concrete implementation of the UriRetrieverInterface ? Setting in interface implementation on the factory is supported. It is actually how it is by design as there are: FileGetContents, 'CurlandPredefinedArray`.

You could even use your own Laravel version running in production and not only in tests.

@marineusde
Copy link
Author

I dont want to override the whole FileGetContents-Class, but if you dont like the merge request I must do it ;)

@DannyvdSluijs
Copy link
Collaborator

Wel the thing is the Interface has been put in place for a reason. To extend this library to the specific needs of you application. Having a method that can be overloaded means this method becomes the one people will start relying on. If we where to change this method issues could pop up with users.

From my perspective there are two options:

  1. You add a LaravelFileGet which extends the AbstractRetriever (or implements the UriRetrieverInterface directly)
  2. You can create an open source repository like laravel-json-schema which requires this lib and laravel and put the above mentioned LaravelFileGet in there and others that use Laravel can benefit from that package as well.

For now "i'll close the issue and PR as this doesn't fit in this package (for now). Feel free to comment if you have any additional insights/thoughts that could change the decision on this issue.

@DannyvdSluijs DannyvdSluijs closed this as not planned Won't fix, can't repro, duplicate, stale Aug 5, 2024
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

No branches or pull requests

2 participants