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

Revert "Do not override error handler" #271

Merged
merged 1 commit into from
May 23, 2016

Conversation

bighappyface
Copy link
Collaborator

Reverts #262

@bighappyface bighappyface merged commit 449bb4d into master May 23, 2016
@bighappyface bighappyface deleted the revert-262-uri-retriever-error-handler branch May 23, 2016 21:59
@bighappyface
Copy link
Collaborator Author

@Maks3w should I bump to 2.0.4 or is that something else that is wrong to you?

@Maks3w
Copy link
Contributor

Maks3w commented May 23, 2016

IIRC semver said in this case a major version should be released. But knowing how Composer work it's better to be released as minor or patch.

We could consider this like a bug instead of HTTP support drop so release as 2.0.4

@dmitry-varennikov-eventbase
Copy link
Contributor

Can anybody point me to the place where $context variable used as we reverted to the original code?

@dmitry-varennikov-eventbase
Copy link
Contributor

dmitry-varennikov-eventbase commented May 24, 2016

Also would you mind to use my original proposal to use error control operator @ to suppress errors for file_get_contents instead of redefining error handler?

@dmitry-varennikov-eventbase
Copy link
Contributor

I didn't find any errors mentioning for curl-exec. So I'm not sure what your question is.

Also I believe Curl and FileGetContents retrievers should behave the same and both should NOT override php error handler but just throw ResourceNotFoundException

@Maks3w
Copy link
Contributor

Maks3w commented May 24, 2016

@dmitry-varennikov-eventbase Error supression with @ its almost not allowed by any project. One option is allow inject a custom error handler.

public $errorHandlersCallables = ['start' => callable, 'end' => callable]

$this->errorHandlersCallables['start']()
....
$this->errorHandlersCallables['end']()

@dmitry-varennikov-eventbase
Copy link
Contributor

@Maks3w don't you think it requires too much code for every caller to retrieve a JSON schema file?

@Maks3w
Copy link
Contributor

Maks3w commented May 25, 2016

custom error handlers are optional. By default current behavior (set_error_handler) is used.

IMO let this as is and come back when you have an specific real case.

@dmitry-varennikov-eventbase
Copy link
Contributor

@Maks3w
Here's a specific real case I described in my original PR and suggested a solution to use error control operator @ instead of overriding error handler. Your answer was

Error supression with @ its almost not allowed by any project.

Is it the only argument NOT to use my suggestion?

@Maks3w
Copy link
Contributor

Maks3w commented May 25, 2016

May this could work.

$error = false;
set_error_handler(function ($a, $b, ...) use (&$error) { $error = [$a, $b, ...];}
file_get_contents
restore_error_handler
if ($error) {throw Exception($error['a'], $error['b'])}

@dmitry-varennikov-eventbase
Copy link
Contributor

Yeah as long as an exception is thrown outside of the re-defined error handler I'm happy with it

@dmitry-varennikov-eventbase
Copy link
Contributor

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.

3 participants