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

[DoctrineBridge] [PropertyInfo] Catch Doctrine\ORM\Mapping\MappingException #17152

Closed
wants to merge 2 commits into from

Conversation

dunglas
Copy link
Member

@dunglas dunglas commented Dec 28, 2015

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

Sometimes the Doctrine ORM ClassMetadataFactory throws a Doctrine\Common\Persistence\Mapping\MappingException exception, sometime a Doctrine\ORM\Mapping\MappingException.
This PR catch both.

Port of dunglas/php-property-info#10.

@Tobion
Copy link
Member

Tobion commented Dec 28, 2015

Doctrine is missing to document the possible exception: https://github.com/doctrine/common/blob/master/lib/Doctrine/Common/Persistence/Mapping/ClassMetadataFactory.php#L47

As it does not specify which exception can be thrown, it would make more sense the catch all exceptions (\Exception).

@nicolas-grekas
Copy link
Member

Catching all exceptions to ignore them could start a debugging nightmare...

@stof
Copy link
Member

stof commented Dec 28, 2015

The ORM MappingException case here can happen when using older versions of the ORM (in newer versions, the ORM exceptions extends from the Common one and so is catched already).

@Tobion an invalid mapping always throws a MappingException. Other exceptions could be thrown because of bugs, but these one should be kept uncaught here.

@Tobion
Copy link
Member

Tobion commented Dec 28, 2015

@stof I don't see that ORM MappingException extends the common one: https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Mapping/MappingException.php#L27

@stof
Copy link
Member

stof commented Dec 28, 2015

oh, I thought it did. This looks weird.

@nicolas-grekas
Copy link
Member

👍

1 similar comment
@xabbuh
Copy link
Member

xabbuh commented Dec 29, 2015

👍

@fabpot
Copy link
Member

fabpot commented Dec 29, 2015

Thank you @dunglas.

@fabpot fabpot closed this Dec 29, 2015
fabpot added a commit that referenced this pull request Dec 29, 2015
…\MappingException (dunglas)

This PR was squashed before being merged into the 2.8 branch (closes #17152).

Discussion
----------

[DoctrineBridge] [PropertyInfo] Catch Doctrine\ORM\Mapping\MappingException

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

Sometimes the Doctrine ORM `ClassMetadataFactory` throws a `Doctrine\Common\Persistence\Mapping\MappingException` exception, sometime a `Doctrine\ORM\Mapping\MappingException`.
This PR catch both.

Port of dunglas/php-property-info#10.

Commits
-------

ceded10 [DoctrineBridge] [PropertyInfo] Catch Doctrine\ORM\Mapping\MappingException
@fabpot fabpot mentioned this pull request Jan 14, 2016
@fabpot fabpot mentioned this pull request Feb 3, 2016
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

7 participants