8000 [Serializer] rename exception interface by nicolas-grekas · Pull Request #13798 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Serializer] rename exception interface #13798

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 2, 2015
Merged

[Serializer] rename exception interface #13798

merged 1 commit into from
Mar 2, 2015

Conversation

nicolas-grekas
Copy link
Member
Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

@unkind
Copy link
Contributor
unkind commented Feb 26, 2015

This empty interface looks oddly. Maybe it makes sense to add base class of Serializer exceptions for 3.0 and remove interface completely?

I think extending SPL exceptions like \UnexpectedValueException, \InvalidArgumentException, etc. is pointless: I'd never try to catch them if I know package specific exception like \Symfony\Component\Serializer\Exception\InvalidArgumentException.

@Tobion
Copy link
Contributor
Tobion commented Feb 27, 2015

@unkind the SPL exception give semantic meaning (logic vs runtime). so they are not useless IMO.

@unkind
Copy link
Contributor
unkind commented Feb 27, 2015

I don't say they are useless, I don't like idea of having this structure:
\Symfony\Component\Serializer\Exception\InvalidArgumentException extends \InvalidArgumentException
\Symfony\Component\Serializer\Exception\UnexpectedValueException extends \UnexpectedValueException
etc.
What's the point?

@nicolas-grekas
Copy link
Member Author

The point is domain separation - each domain should have its own domain-specific exceptions, so that you can layer domains and still know which layer is at fault. That's also the reason for wrapping exceptions in domain specific exceptions (thus ->getPrevious() on the base Exception class)

@unkind
Copy link
Contributor
unkind commented Feb 27, 2015

The point is domain separation - each domain should have its own domain-specific exceptions

Right, but why do you need SPL exceptions then? Any case when you would try to catch generic \UnexpectedValueException? I suggest to inherit something like SerializerException instead in order to allow to catch all serializer exceptions (I guess empty exception interface has the same purpose).

My point is that empty interface indicates a problem. I know concept of a marker interface, but it's different case IMO.

@dunglas
Copy link
Member
dunglas commented Mar 1, 2015

👍 for this change.

@dunglas
Copy link
Member
dunglas commented Mar 1, 2015

As explained by @nicolas-grekas, use cases are:

  • being to able to catch only exception thrown by the Serializer Component
  • being able to catch all logic or runtime exceptions thrown by different libraries

Think about a generic Logger, not aware of libraries used in the project, and logging only runtime exceptions (ignoring logic exceptions). Thanks to \RuntimeException, this logger will catch both Assetic\Exception\FilterException and Symfony\Component\Serializer\Exception\RuntimeException.

If Symfony\Component\Serializer\Exception\RuntimeException doesn't extend \RuntimeException it's not possible.

Btw, this is done the same way in other Symfony components: https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Validator/Exception/RuntimeException.php

@unkind
Copy link
Contributor
unkind commented Mar 1, 2015

being able to catch all logic or runtime exceptions thrown by different libraries

I can understand catching just \RuntimeException, because it's a little bit special one: like in Java, I usually use it for exceptions which is not documented in the @throws annotation, i.e. some programmer's mistakes, it's not API's one. By the way, in Java, there are subclasses of RuntimeException like IllegalArgumentException and UnsupportedOperationException which are similar to SPL's ones AFAIU. It makes more sense than SPL's ones \LogicException which extends \Exception, IMO. For me, LogicException is kind of runtime's one.

But seriously, I fail to find any case for \InvalidArgumentException and \UnexpectedValueException. They tell me literally nothing about context: somewhere we've passed invalid value... OK, so what?

@dunglas
Copy link
Member
dunglas commented Mar 1, 2015

@unkind To be able to catch \RuntimeException, we need the Symfony\Component\Serializer\Exception\ExceptionInterface and we need to extend SPL exceptions.

So it's better leverage the SPL and extends the more precise exceptions such as \UnexpectedValueException. Even if there are lesser interest in catching them, what's the point not extending them but extending directly \RuntimeException? For instance \UnexpectedValueException extends \RuntimeException.

@unkind
Copy link
Contributor
unkind commented Mar 1, 2015

OK, I have to clarify my opinion: I don't like idea of inheriting \RuntimeException and \LogicException at all: they are "bad" exceptions, programmer's mistakes, like misconfigured Serializer. Usually, at application level you can't "fix" it, program should fail with error. So you are able to omit them in the @throws annotation. If you are pretty sure that you don't want to fail, you can just catch \Exception:

try {
    return $serializer->serialize($foo);
} catch (\Exception $e) {
    $this->logger->error('We are screwed and I have no idea why', ['exception' => $e]);

    return '';
}

but usually I'd prefer to fail by default, because ignoring error can lead serious bugs in the future (http://en.wikipedia.org/wiki/Fail-fast). Well, you can inherit them, but thanks to they are runtime, you don't have to.

For example, Serializer::normalizeObject throws an UnexpectedValueException, but it's a private method. Who catches it on Serializer component level? No one. Who declares it in the @throws annotation in some public interface of Serializer component? No one. So to be consistent you have to either put @throws UnexpectedValueException in the NormalizerInterface::normalize or remove runtime/logic exceptions from public contracts at all. I like the last one.

But for bad incoming data you have to throw regular (non-runtime) exception which would extend SerializerException.

@nicolas-grekas
Copy link
Member Author

I'm not sure I understand where you want to go @unkind. Anyway, this discussion is not related to this PR. Here I only want to fix a class that does not respect a naming convention applied everywhere otherwise (Interface suffix for interfaces).
Feel free to open an issue, or better, a PR, to expose your arguments.

Taluu
Taluu reviewed Mar 1, 2015
View reviewed changes
src/Symfony/Component/Serializer/Exception/Except 8000 ionInterface.php
*
* @author Johannes M. Schmitt <schmittjoh@gmail.com>
*/
interface ExceptionInterface extends Exception
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be the other way around ? Exception extending the ExceptionInterface ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope: this way allows the best forward/backward compatibility

@fabpot
Copy link
Member
fabpot commented Mar 2, 2015

👍

@fabpot
Copy link
Member
fabpot commented Mar 2, 2015

Before merging, @nicolas-grekas Can you add a note in the CHANGELOG of the component and a note in the UPGRADE file for 3.0?

@nicolas-grekas
Copy link
Member Author

Notes added (is it enough?)

@fabpot
Copy link
Member
fabpot commented Mar 2, 2015

Looks good to me.

@Tobion
Copy link
Contributor
Tobion commented Mar 2, 2015

👍

@dunglas
Copy link
Member
dunglas commented Mar 2, 2015

Thank you @nicolas-grekas.

@dunglas dunglas merged commit c6bf1de into symfony:2.7 Mar 2, 2015
dunglas added a commit that referenced this pull request Mar 2, 2015
@dunglas
minor #13798 [Serializer] rename exception interface (nicolas-grekas)
c491332 
This PR was merged into the 2.7 branch.

Discussion
----------

[Serializer] rename exception interface

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Commits
-------

c6bf1de [Serializer] rename exception interface
dunglas added a commit to dunglas/symfony that referenced this pull request Mar 2, 2015
@dunglas
minor symfony#13798 [Serializer] rename exception interface (nicolas-…
c531546 
…grekas)

This PR was merged into the 2.7 branch.

Discussion
----------

[Serializer] rename exception interface

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Commits
-------

c6bf1de [Serializer] rename exception interface
@nicolas-grekas nicolas-grekas deleted the x-interface branch March 25, 2015 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nicolas-grekas @unkind @Tobion @dunglas @fabpot @Taluu
0