Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] Move AddConsoleCommandPass from FrameworkBundle to Console.#19443
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
c2deecc todffd5e6Comparemcfedr commentedJul 27, 2016
Maybe worth merging (or reviewing)#19305 first so that it doesnt conflict. |
| "symfony/class-loader":"~3.2", | ||
| "symfony/dependency-injection":"~3.2", | ||
| "symfony/config":"~2.8|~3.0", | ||
| "symfony/console":"~3.2", |
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.
This can be removed. Because of the class_exists call, if the installed component is an older version or isn't installed it will still work. Maybe should you add a another test to not load the deprecated pass if the component is not installed at all (to prevent throwing an illegitimate deprecation warning).
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.
Removed the dependency.
b6f8cc5 to94a19f3Compare| { | ||
| publicfunction__construct() | ||
| { | ||
| @trigger_error(sprintf('%s is deprecated as of 3.2 and will be removed in 4.0. Use Symfony\Component\Console\DependencyInjection\AddConsoleCommandPass instead.',__CLASS__),E_USER_DEPRECATED); |
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.
this should be jus below the namespace declaration, see other places in the code base
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.
Good point, changed.
94a19f3 to1094537Compare| namespaceSymfony\Bundle\FrameworkBundle\DependencyInjection\Compiler; | ||
| @trigger_error(sprintf('%s is deprecated as of 3.2 and will be removed in 4.0. Use Symfony\Component\Console\DependencyInjection\AddConsoleCommandPass instead.',__CLASS__),E_USER_DEPRECATED); |
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.
AddConsoleCommandPass::class instead of__CLASS__ (which is undefined)
1094537 to17385a7Compare| "psr/log":"~1.0" | ||
| }, | ||
| "suggest": { | ||
| "symfony/dependency-injection":"", |
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 would remove this suggestion. The DI component does not allow to add more features to the Console component.
The compiler pass is useful only for people already using the component. So the suggestion is just useless noise.
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.
Makes sense; changed.
17385a7 to6e57865Comparefabpot commentedSep 14, 2016
Tests do not pass because of the deprecation. |
6e57865 todb03053Comparebcremer commentedSep 15, 2016
Tests are now all fixed. |
UPGRADE-3.2.md Outdated
| * The`Controller::getUser()` method has been deprecated and will be removed in | ||
| Symfony 4.0; typehint the security user object in the action instead. | ||
| * The`Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\AddConsoleCommandPass` has been deprecated. Use`Symfony\Component\Console\DependencyInjection\AddConsoleCommandPass` 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.
Please also add this to theUPGRADE-4.0.md (but reword it to "removed" there).
| $container->compile(); | ||
| } | ||
| publicfunctiontestHttpKernelRegisterCommandsIngoreCommandAsAService() |
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.
This whole test case looks a bit suspicious to me. We don't have anything HttpKernel specific here, do we?
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 test was copied 1 to 1 from the original location\Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\Compiler\AddConsoleCommandPassTest::testHttpKernelRegisterCommandsIngoreCommandAsAService.
Any suggestions how to name the testcase 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.
Imo this test case should have never lived here. It actually does not test test compiler pass, but the bundle class from the HttpKernel component and therefore should live there.
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.
see#20273
| * | ||
| * @deprecated since version 3.2, to be removed in 4.0. Use Symfony\Component\Console\DependencyInjection\AddConsoleCommandPass instead. | ||
| */ | ||
| class AddConsoleCommandPassimplements CompilerPassInterface |
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.
Wouldn't it be better to let this class extend the new compiler pass instead of having to maintain the same code twice?
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.
This would introduce a new dependencysymfony/console tosymfony/framework-bundle that is not yet defined in composer.json.
Should the new dependency be added to composer.json or can should this fail during runtime?
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.
We could add a conflict rule for versions of the Console component before 3.2.
014c9eb tocf946c9Compare| namespaceSymfony\Bundle\FrameworkBundle\DependencyInjection\Compiler; | ||
| @trigger_error(sprintf('%s is deprecated as of 3.2 and will be removed in 4.0. Use Symfony\Component\Console\DependencyInjection\AddConsoleCommandPass instead.', AddConsoleCommandPass::class),E_USER_DEPRECATED); |
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.
[...] deprecated since version 3.2 [...]
cf946c9 toe7f2763Comparexabbuh commentedOct 22, 2016
👍 (failure is unrelated) Status: Reviewed |
ro0NL commentedOct 22, 2016
Should we make some hardcoded id's / strings configurable thru required constructor args, as done in#20250? See discussion:#20250 (comment) |
…l component (xabbuh)This PR was merged into the 2.7 branch.Discussion----------[FrameworkBundle][HttpKernel] move test to the HttpKernel component| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#19443 (comment)| License | MIT| Doc PR |The moved test case does not test the `AddConsoleCommandPass` class, but ensures certain behavior in the `registerCommands()` method of the `Bundle` class from the HttpKernel component.Commits-------c9ca322 move test to the HttpKernel component
bcremer commentedJan 11, 2017
Is there still anything I can help to get this merged into 3.3? |
xabbuh commentedJan 11, 2017
@bcremer You rebase this once again to resolve conflicts, but otherwise this looks ready to me. ping @symfony/deciders |
UPGRADE-3.2.md Outdated
| * The`Resources/public/images/*` files have been removed. | ||
| * The`Resources/public/css/*.css` files have been removed (they are now inlined | ||
| in TwigBundle). | ||
| * The`Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\AddConsoleCommandPass` has been deprecated. Use`Symfony\Component\Console\DependencyInjection\AddConsoleCommandPass` 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.
Ah sorry, now that Symfony 3.2 is released we would need to move this to theUPGRADE-3.2md file and update deprecation messages accordingly.
e7f2763 to4928c24Compare4928c24 to7743989Comparebcremer commentedJan 11, 2017 • 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.
@xabbuh Rebased onto |
fabpot commentedJan 11, 2017
Thank you@bcremer. |
…dle to Console. (bcremer)This PR was merged into the 3.3-dev branch.Discussion----------[Console] Move AddConsoleCommandPass from FrameworkBundle to Console.| Q | A || --- | --- || Branch? | master || Bug fix? | no || New feature? | yes || BC breaks? | yes || Deprecations? | yes || Tests pass? | yes || Fixed tickets |#19440 || License | MIT || Doc PR | - |Commits-------7743989 Move AddConsoleCommandPass from FrameworkBundle to Console.
…ent isn't installed (dunglas)This PR was merged into the 3.3-dev branch.Discussion----------[FrameworkBundle] Prevent an error when the console component isn't installed| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#21246| License | MIT| Doc PR |n/aFinish#19443. Alternative to#21246.Commits-------ab133ca [FrameworkBundle] Prevent an error when the console component isn't installed
…onent (chalasr)This PR was merged into the 3.3-dev branch.Discussion----------[Form][FrameworkBundle] Move FormPass to the Form component| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aSo that anyone using only Form and DI can use it for registering form types/type guessers.Follows#19443, related to#21284Commits-------e68a6d9 [FrameworkBundle][Form] Move FormPass to the Form component
Uh oh!
There was an error while loading.Please reload this page.