-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
[FrameworkBundle][Translation] allow register custom resources paths. #14399
Conversation
->children() | ||
->arrayNode('fallbacks') | ||
->beforeNormalization()->ifString()->then(function ($v) { return array($v); })->end() | ||
->prototype('scalar')->end() | ||
->defaultValue(array('en')) | ||
->end() | ||
->arrayNode('paths') | ||
->beforeNormalization()->ifString()->then(function ($v) { return array($v); })->end() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it really needed ?
8684447
to
308e6f5
Compare
foreach ($config['paths'] as $path) { | ||
if (is_dir($path)) { | ||
$dirs[] = $path; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really a good idea to silently ignore values not representing an array? This would make debugging a bit harder I guess. What about doing validation in the Configuration
class instead and providing a meaningful error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not possible to add validation it in the Configuration
because we need to resolve parameters before checking directories, so I'll throw an exception here instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, that could still become an issue if a parameter were changed in a compiler pass, couldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's true, is it safe to move it into TranslatorPass
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TranslatorPass
is executed before the optimization passes (in which parameters are resolved). To be on the safe side, the compiler pass must be registered as a PassConfig::TYPE_BEFORE_REMOVING
pass.
308e6f5
to
baae750
Compare
what do you think about adding paths:
- { priority: 125, path: ../Resources/translations} # defaul is -10
|
are you agree with adding |
closed in favor of #14561. |
This allow to add custom resources path via prepend config or in config.yml