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

[HttpKernel] Add ability to configure catching exceptions for Client #22890

Merged
merged 1 commit into from Sep 26, 2017
Merged

[HttpKernel] Add ability to configure catching exceptions for Client #22890

merged 1 commit into from Sep 26, 2017

Conversation

kbond
Copy link
Member

@kbond kbond commented May 24, 2017

Q A
Branch? 3.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR todo

Debugging exceptions in functional tests is difficult as you need to look at the logs to see which exception was thrown. Disabling catching of exceptions in the client would allow the exception to bubble up to phpunit and make it easier to see what exception was thrown.

@nicolas-grekas
Copy link
Member

new features should be submitted against 3.4

@kbond kbond changed the base branch from master to 3.4 May 24, 2017 15:07
@@ -31,6 +31,7 @@
class Client extends BaseClient
{
protected $kernel;
protected $catchExceptions = true;
Copy link
Member

Choose a reason for hiding this comment

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

should be private

*
* @param bool $catchExceptions Whether to catch exceptions
*/
public function catchExceptions($catchExceptions = true)
Copy link
Member

Choose a reason for hiding this comment

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

having a default value here does not very useful to me

@kbond
Copy link
Member Author

kbond commented Sep 26, 2017

Anything preventing this from making it into 3.4?

Copy link
Member

@ogizanagi ogizanagi left a comment

Choose a reason for hiding this comment

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

Missing CHANGELOG.md entry ? :)

*
* @return bool
*/
public function isCatchingExceptions()
Copy link
Member

Choose a reason for hiding this comment

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

Is this method really useful?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really, I was just matching Symfony\Component\BrowserKit\Client::isFollowingRedirects()

Copy link
Member

Choose a reason for hiding this comment

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

let's remove it :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@kbond
Copy link
Member Author

kbond commented Sep 26, 2017

I added a CHANGELOG.md entry.

Let me know if you want me to remove Client::isCatchingExceptions()

@ogizanagi
Copy link
Member

Thank you @kbond.

@ogizanagi ogizanagi merged commit 4812e60 into symfony:3.4 Sep 26, 2017
ogizanagi added a commit that referenced this pull request Sep 26, 2017
…ons for Client (kbond)

This PR was merged into the 3.4 branch.

Discussion
----------

[HttpKernel] Add ability to configure catching exceptions for Client

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | todo

Debugging exceptions in functional tests is difficult as you need to look at the logs to see which exception was thrown. Disabling catching of exceptions in the client would allow the exception to bubble up to phpunit  and make it easier to see what exception was thrown.

Commits
-------

4812e60 add ability to configure catching exceptions
This was referenced Oct 18, 2017
robfrawley added a commit to robfrawley/liip-imagine-bundle that referenced this pull request May 7, 2018
…anged

browser kit client to not catch exceptions to fix our test suite.

References:
  - https://symfony.com/blog/new-in-symfony-4-1-deprecated-the-bundle-notation
  - symfony/symfony#26085
  - symfony/symfony#22890

Signed-off-by: Rob Frawley 2nd <rmf@src.run>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants