[Routing][Translation] Assert return value for PHP configuration #23522
Conversation
| Q | A |
|---|---|
| Branch? | 2.7 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #23518 |
| License | MIT |
| Doc PR | symfony/symfony-docs#... |
| @@ -37,6 +37,10 @@ public function load($resource, $locale, $domain = 'messages') | |||
|
|
|||
| $messages = require $resource; | |||
|
|
|||
| if (!is_array($messages)) { | |||
| throw new \UnexpectedValueException(sprintf('PHP routing configuration must return an array, got "%s" in "%s".', is_object($messages) ? get_class($messages) : gettype($messages), $resource)); | |||
There was a problem hiding this comment.
Typo. It's not about routing here 😄
There was a problem hiding this comment.
I was faster.. was i?
|
Do any of the other PHP config file loaders have the same behaviour and could profit from such a change too? |
|
Think we're good. Serializer/Validator dont have php file loaders. DI doesnt rely on any return value (strictly it could assert null though, but i dont think we should :)) |
|
I'm 👎 on this one. We don't do such check anywhere else in the code and we rejected similar PRs (not on this specific use case) in the past. The linked ticket is about something very different anyway. The |
|
It doesnt hurt.. but i see your point. It's a safeguard for sure. I do like the better error though, given it contains a clear file ref. But that can be found in trace as well. Closing for now as the issue is fixed. |
|
@ro0NL you closed this PR too fast. I'd love to discuss about this a bit more :( Adding features for DX is tricky. Let me show you two quick examples:
Which format should we show in the Symfony Docs? Both? Very verbose! Only one of them? But which one !? Moreover, people will start using any of those solutions interchangeably, so applications will use both formats and will be a bit more confusing. Besides, this adds another problem: can I mix both formats in the same file? If yes, it may be confusing. If no, why not? etc.
|
|
Fair points! For this PR, i closed it for the same reason as #19960. We dont really have to care, that's reasonable :) For #22176, i hope we can settle with it as is. Errors are annoying, lets try not to add them :P for the end user it doesnt matter at least. Besides it works the same as DI service tags in yaml. I personally favor short notation, and use long if needed. So i would do that in docs; not saying we should update all current docs - we could though - but we do need to mention it somewhere in 3.4 i guess. |