8000 [HttpKernel] Inherit Throwable in HttpExceptionInterface by nesk · Pull Request #33809 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[HttpKernel] Inherit Throwable in HttpExceptionInterface #33809

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

Merged
merged 1 commit into from
Oct 3, 2019

Conversation

nesk
Copy link
Contributor
@nesk nesk commented Oct 2, 2019
Q A
Branch? master
Bug fix? no
New feature? no
Deprecations? no
BC BREAKS? YES
Tickets n/a
License MIT
Doc PR n/a

When using a static analysis tools, it is not possible to do this:

if ($exception instanceof HttpExceptionInterface) {
    $exception->getStatusCode();
    $exception->getHeaders();
    $exception->getMessage(); // ❌ Will fail here
}

This is due to getMessage() not being a method declared in HttpExceptionInterface. Since Symfony now requires PHP 7.1+ to run, it is safe to inherit from the Throwable interface (added in PHP 7.0).

About backward compatibility

Adding new methods to HttpExceptionInterface breaks BC, however this interface shouldn't be used on a class other than an exception, so this shouldn't affect much code.

About tests

I'm not sure this really needs tests, but maybe I'm wrong? Tell me what to test if you think this is required.

Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

the PR should target 4.4, as a new "feature"

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 2, 2019
@nesk nesk force-pushed the http-exception-throwable branch from b2d201b to 2ac3fbf 8000 Compare October 3, 2019 08:14
@nesk nesk changed the base branch from master to 4.4 October 3, 2019 08:15
@nesk
Copy link
Contributor Author
nesk commented Oct 3, 2019

the PR should target 4.4, as a new "feature"

Did it, and removed the upgrade line. However, I do not understand why this is considered a new feature instead of a BC break. 🤔

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Oct 3, 2019

If it's considered as a BC break, then merging on master is equally wrong: we don't do hard BC breaks that don't trigger a prior notice in 4.4.

Now we can discuss the BC break nature of this.

This is a BC break only if some class implements HttpExceptionInterface without actually extending a base exception. I think that doesn't exist. So we're in the "yes" of the table on https://symfony.com/bc

@nesk
Copy link
Contributor Author
nesk commented Oct 3, 2019

This is a BC break only if some class implements HttpExceptionInterface without actually extending a base exception. I think that doesn't exist. So we're in the "yes" of the table on symfony.com/bc

I see your point, I don't think anyone did this too.

@nicolas-grekas nicolas-grekas changed the title Inherit Throwable in HttpExceptionInterface [HttpKernel] Inherit Throwable in HttpExceptionInterface Oct 3, 2019
@fabpot
Copy link
Member
fabpot commented Oct 3, 2019

Thank you @nesk.

fabpot added a commit that referenced this pull request Oct 3, 2019
… (nesk)

This PR was merged into the 4.4 branch.

Discussion
----------

[HttpKernel] Inherit Throwable in HttpExceptionInterface

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| **BC BREAKS?**| **YES**
| Tickets       | n/a
| License       | MIT
| Doc PR        | n/a

When using a static analysis tools, it is not possible to do this:

```php
if ($exception instanceof HttpExceptionInterface) {
    $exception->getStatusCode();
    $exception->getHeaders();
    $exception->getMessage(); // ❌ Will fail here
}
```

This is due to `getMessage()` not being a method declared in `HttpExceptionInterface`. Since Symfony now requires PHP 7.1+ to run, it is safe to inherit from the `Throwable` interface (added in PHP 7.0).

### About backward compatibility

Adding new methods to `HttpExceptionInterface` [breaks BC](https://symfony.com/doc/current/contributing/code/bc.html#changing-interfaces), however this interface shouldn't be used on a class other than an exception, so this shouldn't affect much code.

### About tests

I'm not sure this really needs tests, but maybe I'm wrong? Tell me what to test if you think this is required.

Commits
-------

2ac3fbf Inherit Throwable in HttpExceptionInterface
@fabpot fabpot merged commit 2ac3fbf into symfony:4.4 Oct 3, 2019
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
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.

5 participants
0