[DependencyInjection] Dump and read the first error of service for XML and YAML format #34928
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…L and YAML format
…onPass removing pass
fancyweb
commented
Dec 10, 2019
| @@ -81,6 +87,16 @@ private function getContainerBuilder(): ContainerBuilder | |||
| $refl = new \ReflectionProperty($parameterBag, 'resolved'); | |||
| $refl->setAccessible(true); | |||
| $refl->setValue($parameterBag, true); | |||
|
|
|||
| // To keep in sync with the default removing passes of PassConfig minus DefinitionErrorExceptionPass | |||
| $container->getCompilerPassConfig()->setRemovingPasses([ | |||
There was a problem hiding this comment.
Setting everything like this seems like the easier and faster option.
fancyweb
commented
Dec 10, 2019
| /** | ||
| * @internal | ||
| */ | ||
| public function getFirstError(): ?string |
There was a problem hiding this comment.
@nicolas-grekas This new internal method avoid to potentially compute several error messages, to finally only use the first one in all of our usage.
fancyweb
commented
Dec 10, 2019
| @@ -134,6 +134,7 @@ | |||
| <xsd:attribute name="decoration-priority" type="xsd:integer" /> | |||
| <xsd:attribute name="autowire" type="boolean" /> | |||
| <xsd:attribute name="autoconfigure" type="boolean" /> | |||
| <xsd:attribute name="error" type="xsd:string" /> | |||
There was a problem hiding this comment.
Reading the error is good for us, but that also mean it can be defined by users with this new attribute (preferred to keep things simple with an attribute btw). We open this new possibility. I'm not sure it's good 🤔 That makes it a feature too? That means it can't really go on 4.4 and that we can't fix the linked issue like this? I need some input on all of this, thanks.
fabpot
added a commit
that referenced
this pull request
Jan 7, 2020
…. services (fancyweb) This PR was merged into the 4.4 branch. Discussion ---------- [FrameworkBundle][ContainerLintCommand] Only skip .errored. services | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - This PR partially reverts #34935 that was a kind of a mistake. Skipping all removed ids cannot work because all private services are in the "removed" ids. So the command ends up not validating much. To still fix the original issue #34858 we need to skip errored services. However, definition errors are not dumped / read (see #34928). So instead, even if it's bad, we can maybe rely on the name for this particular error since it's hardcoded 😕 At least it fixes this case in a simple way 😕 Commits ------- d38cdc9 [FrameworkBundle][ContainerLintCommand] Only skip .errored. services
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Firstly, we dump the first error message in the XML and YAML container definition file (like it is "more or less" currently done for the PHP dumper).
Then, when we load those files, we are able to set an error on the definition.
Finally, since the container we use for the container lint command was dumped with errored services (because it is dumped before any removing passes), we need to remove the
DefinitionErrorExceptionPasspass or it will throw.At this point, shouldn't we use a dedicated valid XML container for this command, or simply just rebuild it everytime?