Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Move console configuration to PHP#37216
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[FrameworkBundle] Move console configuration to PHP#37216
Uh oh!
There was an error while loading.Please reload this page.
Conversation
AhmedRaafat14 commentedJun 11, 2020
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | yes |
| Deprecations? | no |
| Tickets | #37186 |
| License | MIT |
| Doc PR | n/a |
1a68ab3 todd8b2cdCompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
fbfb026 to1d88889Compare
javiereguiluz left a comment
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.
@AhmedRaafat14 you were fast! This looks ready to merge. Thanks!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
c680793 to071c02aCompare| service('translation.extractor'), | ||
| param('translator.default_path'), | ||
| abstract_arg('twig.default_path'), | ||
| abstract_arg('Translator paths'), |
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.
<argument type="collection" />
Well, it seems we can't useabstract_arg() here, or we should add a newcollection_abstract_arg() or something else to set[] as default.
The tests can be solved by setting[] for both collection arguments.
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.
@AhmedRaafat14 I afraid we need to change it again to[], // Translator paths, sorry, same for Twig paths argument (same for both commands), thanks!
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.
I was I did that it fixes the tests then I thought it might be me mistaken.
Thank you for confirming that I will update the PR ASAP.
AhmedRaafat14Jun 11, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
@javiereguiluz@yceruto
To fix the testing I had to revert back everycollection argument to an empty array[] with a comment.
Also even for theconsole.command.translation_debug the unit tests failed with usingabstract_arg('twig.default_path') it throws an exception that it expects string but got anAbstractArgument.
I will give it sometime tomorrow to try to fix it :) if I can. (in seperate PR for sure)
What do you think?
Exception Example:
Symfony\Component\DependencyInjection\Exception\InvalidParameterTypeException: Invalid definition for service "console.command.translation_debug": argument 5 of "Symfony\Bundle\FrameworkBundle\Command\TranslationDebugCommand::__construct" accepts "string", "Symfony\Component\DependencyInjection\Argument\AbstractArgument" passed.9b07f74 toef01839Compare
yceruto left a comment
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.
LGTM, thanks!
fabpot commentedJun 12, 2020
Thank you@AhmedRaafat14. |
…lt is disabled (GromNaN)This PR was merged into the 5.3 branch.Discussion----------[Framework][Secrets] Fix service definition when local vault is disabled| Q | A| ------------- | ---| Branch? | 5.3| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -Local vault can be disabled with the following configuration:```framework: secrets: local_dotenv_file: ~```This removes the service `secrets.local_vault` ([code](https://github.com/symfony/symfony/blob/5.3/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L1669)). The `secrets:...` commands support this by having `$localVault` argument nullable.When configuration was moved from XML to PHP in#37216, the attribute `on-invalid="ignore"` of some services was left.Commits-------0e11f5a Fix commands when local vault is disabled