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] change $previous argument for HttpException to \Throwable #30729

Merged
merged 1 commit into from Mar 30, 2019

Conversation

Projects
None yet
6 participants
@sGy1980de
Copy link
Contributor

commented Mar 27, 2019

Q A
Branch? 4.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #30728
License MIT

This will fix #30728 with the suggested solution to change the signature of HttpException and all its descendants from \Exception to \Throwable.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Thanks, LGTM. Should target master as it's a new feature.

@stof

stof approved these changes Mar 28, 2019

@sGy1980de

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

@nicolas-grekas
Could you please explain a little, why you consider this a new feature? To me it is a bug or at least unintended behaviour. Furthermore this won't change any existing application out there, so i really don't get the point here.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

This never worked as you said, so it wasn't supported. Adding support for something that wasn't possible before is almost the definition of a new feature to me.

@sGy1980de

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

So what's next?
@javiereguiluz marked the corresponding issue as bug. Shall i raise it with target master again or let it like that?

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

@sGy1980de I marked it as a bug because the issue description looked as a bug. But I could be wrong, so please don't think that it's definitive.

@sGy1980de

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Hey guys - what about the bug vs. feature discussion on this? Is it discussed internally or are all following @nicolas-grekas opinion and i should raise this with target master again?

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

I tend to agree with @nicolas-grekas here.

@fabpot fabpot changed the base branch from 4.2 to master Mar 30, 2019

@fabpot fabpot force-pushed the sGy1980de:4.2 branch from a51d4d4 to 15cb475 Mar 30, 2019

@sGy1980de sGy1980de requested review from dunglas, lyrixx, sroze and xabbuh as code owners Mar 30, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

Thank you @sGy1980de.

@fabpot fabpot merged commit 15cb475 into symfony:master Mar 30, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build cancelled
Details
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 30, 2019

feature #30729 [HttpKernel] change $previous argument for HttpExcepti…
…on to \Throwable (sGy1980de)

This PR was submitted for the 4.2 branch but it was squashed and merged into the 4.3-dev branch instead (closes #30729).

Discussion
----------

[HttpKernel] change $previous argument for HttpException to \Throwable

| Q             | A
| ------------- | ---
| Branch?       |  4.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #30728
| License       | MIT

This will fix #30728 with the suggested solution to change the signature of `HttpException` and all its descendants from `\Exception` to `\Throwable`.

Commits
-------

15cb475 [HttpKernel] change $previous argument for HttpException to \Throwable

fabpot added a commit that referenced this pull request Mar 30, 2019

feature #30780 Fix some exception previous type hints (fabpot)
This PR was merged into the 4.3-dev branch.

Discussion
----------

Fix some exception previous type hints

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

follow-up for #30729

Commits
-------

f92efeb fixed some exception previous type hints

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.