Skip to content

Conversation

@GrahamCampbell
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
License MIT

What's in this PR?

Adds a missing import to fix fatal errors.

Why?

image

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
  • Documentation pull request created (if not simply a bugfix)

To Do

  • If the PR is not complete but you want to discuss the approach, list what remains to be done here


namespace Http\Client\Common\Plugin;

use Exception;
Copy link
Member

Choose a reason for hiding this comment

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

Conflicts with the other exception import. Also, our convention is not to import root namespaces classes, but use them directly.

@sagikazarmark
Copy link
Member

This might not be that easy though. The Journal only accepts Httplug exceptions, so we need to check the exception type in the rejection handler.

@Nyholm you created this part AFAIK. What was the convention? The Journal only collects HTTP related exceptions, right? (So we can show them in the debug toolbar?) Any other exception is out of scope IMO.

@sagikazarmark
Copy link
Member

Thanks @GrahamCampbell. So Styleci uses HTTPlug now? 😄 Exciting. Our code is used to test our code?

@GrahamCampbell
Copy link
Contributor Author

Hi, yeh, the link is KnpLabs/php-github-api#409. I was testing the new library on StyleCI (now in production). If we can't get this resolved soonish, I will need to rollback the deployment.

@GrahamCampbell
Copy link
Contributor Author

How does this look now, I've taken off the typehint so we can deal with any eventuality, and added an instanceof check.

@GrahamCampbell
Copy link
Contributor Author

Hmm, maybe we don't actually need to change anything here. Looks like the other library is actually broken here:

image

Throwing another exception there should not be allowed, right?

@GrahamCampbell
Copy link
Contributor Author

@GrahamCampbell
Copy link
Contributor Author

Closing this. There is no bug in your library. The other library is just misusing it, right?

@GrahamCampbell GrahamCampbell deleted the patch-1 branch July 30, 2016 10:20
@GrahamCampbell
Copy link
Contributor Author

How does KnpLabs/php-github-api#410 look to you please?


Thanks for the cool library btw. :)

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