[Config][Exception] Improve Routing Syntax Import Error #11313
Conversation
JanDC
commented
Jul 5, 2014
JanDC
commented
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #11300 |
| License | MIT |
| Doc PR | n/a |
| $message .= ' ' . sprintf('%s', $trimmedMessage) . ' in '; | ||
| } else { | ||
| $message .= ' ' . sprintf('%s', $previous->getMessage()) . ' in '; | ||
| } |
There was a problem hiding this comment.
This is removing a trailing period from the previous exception message if there is one:
Unable to parse at line 5 (near "pattern /").
Becomes just:
Unable to parse at line 5 (near "pattern /")
It looks a little hacky, but it makes sense. I think we should at least add a comment here about what we're doing - I had to stare at this for a minute :).
|
I think you did a great job! But now I kind of think this exception class needs a test to test for these different message possibilities! |
| * @param string $resource The resource that could not be imported | ||
| * @param string $sourceResource The original resource importing the new resource | ||
| * @param int $code The error code | ||
| * @param \Exception $previous A previous exception |
There was a problem hiding this comment.
These changes should be reverted (AFAICS, you just removed spaces here.)
|
|
|
given that we have some logic in there, it should be tested IMO, to ensure that the code is not broken |
|
@JanDC is my colleague, we talked about adding this test |
modified FileLoaderLoadException message to present the previous (Parse)Exception first
|
we rebased and squashed some commits |
| @@ -0,0 +1,74 @@ | |||
| <?php | |||
|
|
|||
| namespace Symfony\Component\Config\Tests\Exception; | |||
|
👍 |
| $message .= sprintf( | ||
| '(which is being imported from "%s")', | ||
| $this->varToString($sourceResource) | ||
| ); |
There was a problem hiding this comment.
Can you move everything on one line here?
|
👍 |
|
ping @symfony/deciders |
|
+1 |
|
Thank you @JanDC. |
…r (Jan Decavele, tvlooy) This PR was merged into the 2.6-dev branch. Discussion ---------- [Config][Exception] Improve Routing Syntax Import Error | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #11300 | License | MIT | Doc PR | n/a Commits ------- fec9a4a removed some more spaces 16134d9 Merge remote-tracking branch 'upstream/master' b099936 - Removed spaces around the concation dots to be more consitent - adjusted some formatting 0459d89 Addition of the symfony license text de43182 Add test and small code fix 8ac5275 ISSUE #11300: Improve Routing Syntax Import Error
