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

Introduce ResponseParsingException and throw this exception in … #825

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

tandhika
Copy link

…AbstractProvider::parseResponse() if necessary

@tandhika
Copy link
Author

This PR aims at resolving issue #626

@tandhika
Copy link
Author

The failed test was due to a bug in json_decode of php5.6, which accepts empty string as a valid json.

$ php -a
Interactive mode enabled

php > echo phpversion() . "\n"; json_decode('',true); echo json_last_error_msg();
5.6.40-24+ubuntu16.04.1+deb.sury.org+1
No error
php > 

$ php -a
Interactive mode enabled

php > echo phpversion() . "\n"; json_decode('',true); echo json_last_error_msg();
7.4.3
Syntax error
php > 

I'll fix the test and resubmit the PR

@codecov
Copy link

codecov bot commented Nov 3, 2020

Codecov Report

Merging #825 (b7841a4) into master (77def43) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##              master      #825   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       180       184    +4     
===========================================
  Files             20        21    +1     
  Lines            464       476   +12     
===========================================
+ Hits             464       476   +12     
Impacted Files Coverage Δ
src/Provider/AbstractProvider.php 100.00% <100.00%> (ø)
...rc/Provider/Exception/ResponseParsingException.php 100.00% <100.00%> (ø)

@tandhika
Copy link
Author

tandhika commented Nov 3, 2020

I have fixed some broken changes after merging from current master.

src/Provider/AbstractProvider.php Outdated Show resolved Hide resolved
src/Provider/Exception/ResponseParsingException.php Outdated Show resolved Hide resolved
src/Provider/Exception/ResponseParsingException.php Outdated Show resolved Hide resolved
src/Provider/Exception/ResponseParsingException.php Outdated Show resolved Hide resolved
src/Provider/AbstractProvider.php Outdated Show resolved Hide resolved
$e
);
// for any other content types
if ($this->mayThrowResponseParsingException) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we always throw the exception, no matter what?

I know this suggestion is somewhat of a BC break, but if you have ResponseParsingException extend UnexpectedValueException, then anyone catching that exception will start catching the new exceptions and can decide what to do about them.

@shadowhand, @stevenmaguire, thoughts?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your suggestion. I can implement the changes if no objection coming from @shadowhand and @stevenmaguire

@tandhika
Copy link
Author

tandhika commented Nov 7, 2020

So, I have incorporated reviewer suggestions into the last commit, but I kept the BC until there is a need to change that as well. Thx @ramsey for the suggestions. If I should squash all my commits, please let me know.

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.

2 participants