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

Adding Definition::addError() and a compiler pass to throw errors as exceptions #24290

Closed
wants to merge 6 commits into
base: 3.4
from

Conversation

Projects
None yet
3 participants
@weaverryan
Member

weaverryan commented Sep 22, 2017

Q A
Branch? 3.4
Bug fix? yes & no
New feature? yes
BC breaks? no
Deprecations? yes (very minor)
Tests pass? yes
Fixed tickets #23606
License MIT
Doc PR Not needed

Hi guys!

Very simple: when there is an error with a Definition, we can now call Definition::addError() instead of throwing an exception. Then, a new compiler pass (after removal) actually throws an exception. The advantage is that we can avoid throwing exceptions for services that are ultimately removed from the container. That's important for auto-registration, where we commonly register all services in src/... but then many of them are removed later.

A few interesting notes:

  • We can probably convert more things from exceptions to Definition::addError(). I've only converted autowiring errors and things in CheckArgumentsValidityPass (that was necessary because it was throwing exceptions in some cases due to autowiring failing... which was the true error)
  • Definition can hold multiple errors, but I'm only showing the first error in the exception message. The reason is clarity: I think usually the first error is the most (or only) important. But having Definition::addError() avoids the possibility of a later error overriding an earlier one

Cheers!

@javiereguiluz javiereguiluz changed the title from Adding Definition::addError() and a compiler pass to throw errors as to Adding Definition::addError() and a compiler pass to throw errors as exceptions Sep 22, 2017

@@ -11,6 +11,8 @@
namespace Symfony\Component\DependencyInjection\Compiler;
@trigger_error('The '.__NAMESPACE__.'\AutowireExceptionPass class is deprecated since version 3.4 and will be removed in 4.0. Use the DefinitionErrorExceptionPass class instead.', E_USER_DEPRECATED);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 23, 2017

Member

missing annotation on the class

@nicolas-grekas

nicolas-grekas Sep 23, 2017

Member

missing annotation on the class

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
Show outdated Hide outdated ...ny/Component/DependencyInjection/Compiler/CheckArgumentsValidityPass.php

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Sep 23, 2017

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Sep 23, 2017

Member

Good suggestions! They're all handled :)

Member

weaverryan commented Sep 23, 2017

Good suggestions! They're all handled :)

weaverryan added some commits Sep 24, 2017

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Sep 24, 2017

Member

Tests pass, fabbot failure is false! Should be ready to go :)

Member

weaverryan commented Sep 24, 2017

Tests pass, fabbot failure is false! Should be ready to go :)

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas
Member

nicolas-grekas commented Sep 25, 2017

Thank you @weaverryan.

nicolas-grekas added a commit that referenced this pull request Sep 25, 2017

feature #24290 Adding Definition::addError() and a compiler pass to t…
…hrow errors as exceptions (weaverryan)

This PR was squashed before being merged into the 3.4 branch (closes #24290).

Discussion
----------

Adding Definition::addError() and a compiler pass to throw errors as exceptions

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes & no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes (very minor)
| Tests pass?   | yes
| Fixed tickets | #23606
| License       | MIT
| Doc PR        | Not needed

Hi guys!

Very simple: when there is an error with a Definition, we can now call `Definition::addError()` instead of throwing an exception. Then, a new compiler pass (after removal) actually throws an exception. The advantage is that we can avoid throwing exceptions for services that are ultimately removed from the container. That's important for auto-registration, where we commonly register all services in `src/`... but then many of them are removed later.

A few interesting notes:
- We can probably convert more things from exceptions to `Definition::addError()`. I've only converted autowiring errors and things in `CheckArgumentsValidityPass` (that was necessary because it was throwing exceptions in some cases due to autowiring failing... which was the true error)
- `Definition` can hold multiple errors, but I'm only showing the first error in the exception message. The reason is clarity: I think usually the first error is the most (or only) important. But having `Definition::addError()` avoids the possibility of a later error overriding an earlier one

Cheers!

Commits
-------

a85b37a Adding Definition::addError() and a compiler pass to throw errors as exceptions

@weaverryan weaverryan deleted the weaverryan:definition-add-error branch Sep 25, 2017

nicolas-grekas added a commit that referenced this pull request Oct 2, 2017

minor #24399 [DI] Fix missing CHANGELOG update + typo (ogizanagi)
This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Fix missing CHANGELOG update + typo

| Q             | A
| ------------- | ---
| Branch?       | 3.4 <!-- see comment below -->
| Bug fix?      | no
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | #24290 <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A

Commits
-------

f9aeee5 [DI] Fix missing CHANGELOG update + typo

nicolas-grekas added a commit that referenced this pull request Oct 2, 2017

feature #24398 [DI] Remove AutowireExceptionPass (ogizanagi)
This PR was merged into the 4.0-dev branch.

Discussion
----------

[DI] Remove AutowireExceptionPass

| Q             | A
| ------------- | ---
| Branch?       | master <!-- see comment below -->
| Bug fix?      | no
| New feature?  | yes <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | yes
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | #24290 <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A

Commits
-------

2ada558 [DI] Remove AutowireExceptionPass

This was referenced Oct 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment