Don't change error reporting level #88

Closed
wants to merge 3 commits into from

3 participants

@johannac

Removing the lines which change the error reporting level to zero (off). This completely masks any problems with the URL, request, or response from the server, making debugging difficult. Using error_get_last in the next line to throw an exception does not always provide relevant information as file_get_contents will throw several errors on failure. Most errors seem to be warnings, so will not halt execution of the program and will allow you to see useful debugging information in the logs. Since the FileGetContents stream is the default one, I would expect a lot of people, especially new users like myself, to come up against this problem. This seems to be the only instance where the error reporting level is changed, so perhaps its ok to just get rid of it and let the users control the error reporting levels themselves?

Johanna Cherry Do not change error reporting level as it doesn't affect they intende…
…d behaviour of throwing the exception.
5010d0c
@stof

The issue is that this makes the behavior of Buzz inconsistent if you have an error handler turning warning into exceptions (which is the case in Symfony in debug mode for instance): you would get a different exception in debug mode and in non-debug mode, making it hard to catch them.

@johannac

That seems ok to me. That is the built in functionality of Symfony, to have warnings turned into exceptions when in debug mode, so the user should expect different exceptions in each mode. Its not something that Buzz can control, and I don't think it should try to control it. Since Buzz is a library, and has no bootstrapping or global error handler, I think those things should all be left up to the framework/library using Buzz. I am using it in Behat by btw.

Johanna Cherry Reason phrase is not required, as per the HTTP spec, so set the first…
… 2 parts even if the 3rd is not present.
d5af87a
@johannac

Per the HTTP spec, here http://www.w3.org/Protocols/rfc2616/rfc2616-sec6.html the response message is not required in the response headers, yet Buzz\Message\Response was requiring it.

@kriswallsmith

I don't see anything in the spec that says the reason phrase is optional, only that the client is not required to display it.

I am open to removing the change in error handling if it makes it easier to debug issues, but can't merge only that change because you've submitted two changes here.

@johannac johannac Revert "Reason phrase is not required, as per the HTTP spec, so set t…
…he first 2 parts even if the 3rd is not present."

This reverts commit d5af87a8c9688e6656c40af79636c46f67c3ec61.
7c39672
@johannac

I can't actually remember why I added that bit about the reason phrase... may have been a rage commit. Have reverted now.
Regarding the error reporting, I'm not actually using Buzz anymore as I've left that project I used it on. So if its better for the symfony integration/the project as a whole to leave the error handling in then please feel free to just close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment