Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 12, 2020

Conversation

AhmedRaafat14
Copy link

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets #37186
License MIT
Doc PR n/a

@AhmedRaafat14 AhmedRaafat14 force-pushed the framworkbundle-rip-xml-console branch 2 times, most recently from fbfb026 to 1d88889 Compare June 11, 2020 14:24
Copy link
Member

@javiereguiluz javiereguiluz left a 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!

@AhmedRaafat14 AhmedRaafat14 force-pushed the framworkbundle-rip-xml-console branch from c680793 to 071c02a Compare June 11, 2020 15:01
service('translation.extractor'),
param('translator.default_path'),
abstract_arg('twig.default_path'),
abstract_arg('Translator paths'),
Copy link
Member

@yceruto yceruto Jun 11, 2020

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 use abstract_arg() here, or we should add a new collection_abstract_arg() or something else to set [] as default.

The tests can be solved by setting [] for both collection arguments.

Copy link
Member

@yceruto yceruto Jun 11, 2020

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!

Copy link
Author

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.

Copy link
Author

@AhmedRaafat14 AhmedRaafat14 Jun 11, 2020

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 every collection argument to an empty array [] with a comment.

Also even for the console.command.translation_debug the unit tests failed with using abstract_arg('twig.default_path') it throws an exception that it expects string but got an AbstractArgument.
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.

@AhmedRaafat14 AhmedRaafat14 force-pushed the framworkbundle-rip-xml-console branch 2 times, most recently from 9b07f74 to ef01839 Compare June 11, 2020 20:51
@AhmedRaafat14 AhmedRaafat14 requested a review from yceruto June 12, 2020 11:45
Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@fabpot
Copy link
Member

fabpot commented Jun 12, 2020

Thank you @AhmedRaafat14.

fabpot added a commit that referenced this pull request Oct 26, 2021
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants