Skip to content
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

Adding a new debug:autowiring command #24583

Merged
merged 1 commit into from Oct 18, 2017
Merged

Conversation

@weaverryan
Copy link
Member

weaverryan commented Oct 17, 2017

Q A
Branch? 3.4 (if I can make my case, otherwise 4.1)
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #21222 and #24562 partially
License MIT
Doc PR TODO

Very simply, this adds a proper debug:autowiring, which is much shorter / nicer than debug:container --types and much prettier.

Before (debug:container --types):

screen shot 2017-10-16 at 8 28 05 pm

screen shot 2017-10-16 at 8 28 18 pm

After (debug:autowiring)

screen shot 2017-10-16 at 7 58 06 pm

screen shot 2017-10-16 at 7 58 16 pm

The command is purposely simple: no special powers, no magic (other than a search argument), just a clean list and nice output.

I would love to sneak this in for 3.4, but I understand either way.

@weaverryan weaverryan force-pushed the weaverryan:debug-autowiring branch from 30a7548 to 41df512 Oct 17, 2017
@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Oct 17, 2017

The class the alias points to would be interesting

@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Oct 17, 2017

We need to remove the --types option from debug:container, right?
👍 for 3.4.

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Oct 17, 2017

This would be really great to have in 3.4

Copy link
Member

javiereguiluz left a comment

This is really nice. I like it!

My only concern is forward compatibility with #24562. If we add a "debug:services" command soon ... should this one be merged with it and renamed as debug:Services? Should this be an independent command? Thanks!

$this
->setDefinition(array(
new InputArgument('search', InputArgument::OPTIONAL, 'A search filter'),
))

This comment has been minimized.

Copy link
@stof

stof Oct 17, 2017

Member

this command should have a format option, for people wanting to do some scripting (we generally don't guarantee stability on the text format for usage with |grep`, as this would forbid us to improve DX, but then it requires having other formats)

This comment has been minimized.

Copy link
@weaverryan

weaverryan Oct 17, 2017

Author Member

I did this on purpose. I really want this command. But I also realize that it's after feature freeze, and things might change with debug:container or debug:services in the future. I don't want to encourage (right now) that people use this for scripting. Also, they can use debug:container --types and pass a format.

/**
* @group functional
*/
class DebugAutowiringCommandTest extends WebTestCase

This comment has been minimized.

Copy link
@stof

stof Oct 17, 2017

Member

you should use KernelTestCase only, as you don't care about the web part (you won't use a BrowserKit client).

This comment has been minimized.

Copy link
@weaverryan

weaverryan Oct 17, 2017

Author Member

I think it's correct actually: this WebTestCase lives right in this same Functional directory and all the other tests (including those for other commands) use it - it helps setup the temp kernel/project I believe. Changing to KernelTestCase made things blow up.

This comment has been minimized.

Copy link
@stof

stof Oct 18, 2017

Member

ah, I missed that this is the special WebTestCase from the FrameworkBundle testsuite, not the one exposed to users of Symfony

@weaverryan

This comment has been minimized.

Copy link
Member Author

weaverryan commented Oct 17, 2017

We need to remove the --types option from debug:container, right?
👍 for 3.4.

I'd like to keep it, and remove it later when we've thought more about what to do with the debug:container command in general. And as Stof pointed out, the new command does not have a --format option, on purpose, so that we have a bit more flexibility to change the command in the future (because we're not guaranteeing BC of the output of the new command). The debug:container --types does have an option, so people can continue to use that :).

My only concern is forward compatibility with #24562. If we add a "debug:services" command soon ... should this one be merged with it and renamed as debug:Services? Should this be an independent command? Thanks!

I think I still like the independent debug:autowiring. But, it should be quite easy to deprecate & change this new command if that's what we want to do. It has a much lower BC-policy cost than most other pieces of code.

All comments addressed!

@nicolas-grekas nicolas-grekas modified the milestones: 4.0, 3.4 Oct 18, 2017
@fabpot
fabpot approved these changes Oct 18, 2017
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Oct 18, 2017

Thank you @weaverryan.

@fabpot fabpot merged commit 41df512 into symfony:3.4 Oct 18, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
fabpot added a commit that referenced this pull request Oct 18, 2017
This PR was merged into the 3.4 branch.

Discussion
----------

Adding a new debug:autowiring command

| Q             | A
| ------------- | ---
| Branch?       | 3.4 (if I can make my case, otherwise 4.1)
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #21222 and #24562 partially
| License       | MIT
| Doc PR        | TODO

Very simply, this adds a proper `debug:autowiring`, which is much shorter / nicer than `debug:container --types` and much prettier.

Before (`debug:container --types`):

<img width="1280" alt="screen shot 2017-10-16 at 8 28 05 pm" src="https://user-images.githubusercontent.com/121003/31641112-931c84ca-b2b0-11e7-9432-136ecf47ed0f.png">
<img width="1280" alt="screen shot 2017-10-16 at 8 28 18 pm" src="https://user-images.githubusercontent.com/121003/31641113-932ac1fc-b2b0-11e7-8a65-34199c9933c1.png">

After (`debug:autowiring`)

<img width="1131" alt="screen shot 2017-10-16 at 7 58 06 pm" src="https://user-images.githubusercontent.com/121003/31641124-a3288a6c-b2b0-11e7-8255-a8e676a26aba.png">
<img width="1101" alt="screen shot 2017-10-16 at 7 58 16 pm" src="https://user-images.githubusercontent.com/121003/31641125-a334c354-b2b0-11e7-8ee3-3bbad5678a1a.png">

The command is purposely simple: no special powers, no magic (other than a `search` argument), just a clean list and nice output.

I would love to sneak this in for 3.4, but I understand either way.

Commits
-------

41df512 Adding a new debug:autowiring command
This was referenced Oct 18, 2017
@weaverryan weaverryan deleted the weaverryan:debug-autowiring branch Oct 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.