Do not override error handler

[dmitry-varennikov-eventbase]

What does this PR Do?

Make use of error control operator to suppress E_WARNING for file_get_contents if JSON schema not found.

Where should the reviewer start?

Check JsonSchema\Uri\Retrievers\FileGetContents::retrieve method and run unit test vendor/bin/phpunit tests/JsonSchema/Tests/Uri/Retrievers/FileGetContentsTest.php

Any background context you want to provide?

It's a nice way to suppress php errors using set_error_handler function and make use of exceptions instead but it comes with one architectural flaw: exceptions thrown from overriden error handler bypass application stack trace.
In my code I use a middleware which takes care of thrown exceptions on the top of my application stack and outputs nice response to a client. In case of overriding error handler (and even restoring it back right away) the error thrown from it bypasses my error handler thus resulting in PHP Fatal error: Uncaught exception 'JsonSchema\\Exception\\ResourceNotFoundException' with message 'JSON schema not found at file:...
Use of error control operator allows to preserve functionality intact and gives a caller code ability to catch this exception within application stack.

UPD: I know I can wrap a call to resolver in try-catch but I expect a more predictable behaviour.

try {
    $schema = $refResolver->resolve('file://'.$events_create_schema);
} catch (ResourceNotFoundException $e) {
    // @TODO: take care of exception here
}

[jojo1981]

@dmitry-varennikov-eventbase Suppressing errors is not a good thing to do. A better solution would be to check a certain condition and throw an exception.

[jojo1981]

Why not:

    if (!file_exists($uri)) {
        throw new ResourceNotFoundException('JSON schema not found at ' . $uri);
    }

    $response = file_get_contents($uri);

[dmitry-varennikov-eventbase]

That also will do

[mirfilip]

Agree with @jojo1981. Nevertheless, the problem is that \JsonSchema\UriRetrieverInterface doesn't state what every particular retriever implementation should do when it cannot retrieve. The behaviour should absolutely be the same for every retriever (or one would have to adjust error handling if a retriever is changed).

@dmitry-varennikov-eventbase I don't want to derail your PR, so I suggest you add @throws \ResourceNotFoundException to the interface and implement the behaviour to just FileGetContents. That'd give us the solid base for further refinements.

[dmitry-varennikov-eventbase]

I took a second look at JsonSchema\Uri\Retrievers\FileGetContents::retrieve method. There is unused stream $context. Just curious if it has to be used at all?!

[mirfilip]

@dmitry-varennikov-eventbase It looks like I has been prepared to be passed to file_get_contents as a stream. The question is whether FileGetContents should work with external URIs - there is Curl for that. It's important because Validator::SCHEMA_MEDIA_TYPE needs to be honoured or not. Also, $context is bound to HTTP protocol. @jojo1981 any ideas?

[jojo1981]

@mirfilip I think $context can be removed. It's not used and it has no purpose in the JsonSchema\Uri\Retrievers\FileGetContents class.

I have no idea why there are 3 classes which implement the JsonSchema\Uri\Retrievers\UriRetrieverInterface. By default always the JsonSchema\Uri\Retrievers\FileGetContents will be used and this one will do it's job for local and remote files. See: https://github.com/justinrainbow/json-schema/blob/master/src/JsonSchema/Uri/UriRetriever.php#L77

[dmitry-varennikov-eventbase]

I removed stream context and used file_exists suggested by @jojo1981

[jojo1981]

@bighappyface I think this one can be merged.

[bighappyface]

@dmitry-varennikov-eventbase would you mind squashing this down into a single commit?

[dmitry-varennikov-eventbase]

Done

[bighappyface]

+1

[jojo1981]

+1

[mirfilip]

@jojo1981 It wasn't clear if FileGetContents is for local files or remote ones. This $context looked like it was meant for both. I think we should keep FileGetContents for local files only, Curl for remote files.

[Maks3w]

Another BC Break PR merged. Another semantic version violation.

This should be released as v3.0.0 not as v2.0.3. Please revert

[bighappyface]

@Maks3w please explain why this refactor is a major release. Accusers carry the burden of proof and you aren't presenting anything.

[Maks3w]

@bighappyface What is the purpose of FileGetContents? What are his responsabilities?

What kind of resources is able to manage file_exists and what kind of resources manage file_get_contents.

The magic word is HTTP

[mirfilip]

@bighappyface Unfortunately, indeed there is a BC break. Early file_exists check breaks "possibility" to use it with external schemas. It went unnoticed because majority of people use it in a local file manner.

@Maks3w Complaining cause you had to patch up mistakes of others people is understandable, going back to the maintainers and bitching about is not. You contributed to ZF2 so you've probably seen it break BC couple of times. The point is, no one did it on purpose. If @bighappyface had noticed the change makes FileGetContents useless HTTP-wise, he'd have probably had it reworked.

Anyway, back to technical stuff, have you seen FileGetContentsTest? You cannot tell what the responsibility of this class is, the tests are crap. This has been the state of matters when you started using this project. To be honest, there is a CurlRetriever that I'd argue you should've used in the first place. The fact FileGetContents used to handle external links could've been as well considered a bug and removed. Fixing such bugs wouldn't have been considered a BC. Seen that in bigger projects.

Just to fresh the air, I have started a major rework in Retrievers department, it will clear up such things.

[bighappyface]

@mirfilip thanks for clarifying. Honestly, I have been super busy and rely on you and others to help me determine what is merged, but we don't really have a discussion around bumping the version and BC concerns.

While I understand why there is a BC issue, I am not 100% sure I think it merits a major release; however, that is in direct contradiction to semantic versioning so I am flexible and will proceed as the group decides.

The reason I am not sure it merits a major release is I see it as more of a fix than a change to the API. While a radical analogy, I see this as similar to adding a safety to a handgun, and just because the previous model didn't have a safety doesn't make the gun "backwards incompatible." It still fires the same, it still operates the same, but an internal change has taken place. This is, of course, my opinion, and will proceed as we decide.

I would prefer to rectify this by further adjusting FileGetContents to achieve BC and stay within the 2.x.x series, but if we do determine a major bump is the best course of action, I think it will set precedent to make most releases major releases as to avoid discussions like this and any potential unseen BC issues. I would prefer that route than suffering the wrath of angry programmers like @Maks3w 😉

[mirfilip]

@bighappyface I'd be awesome to have the project tested in such a way we can blindly rely on whether tests pass to accept the PR. The fact is tests are really bad here. I see no other option but to be extra careful and actually push the PR creator to cover the missing ground. E.g. in this case, we all should stop and document FileGetContents responsibility. I know, it hard to ask external people to dig but ¯_(ツ)_/¯

@bighappyface I'd rewrite your analogy to: you're working with a gun, it shoots. Some day you discover it also scratches your back well. An awesome, undocumented feature. You keep doing that with all the models of the pistol. One day you buy a new one and it's all round, sucks at scratching. The conclusion being: you were using a wrong thing to do the job, but it was undocumented that you cannot use it for scratching. There is no other way but to refer to common sense here. It's not viable to bump the major version everytime you spot the feature is not used properly and you patch it.

I've seen similar situations with ZF2 and Symfony 2.x. Almost always the explanations were "this wasn't supposed to be used this way" and the thing wasn't considered a BC break.