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

Drastically improved debug:autowiring + new autowiring info system #26142

Closed

Conversation

@weaverryan
Copy link
Member

weaverryan commented Feb 11, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets none
License MIT
Doc PR symfony/symfony-docs#9247

Hey guys!

This PR makes debug:autowiring WAY more useful :). See screenshot at the bottom.

It's now human-focused: you look for the problem you need to solve (Cache, Logger, etc).

Note: if there are multiple types for the same thing - e.g. Twig\Environment &
Twig_Environment - then by convention, we'll choose one to "recommend" and only
add info about it. The other will still appear at the bottom of the list.

Behind the scenes, this introduces a new mini-system where bundles can tell the
core the purpose of the classes/interfaces that they expose. I didn't include
this at the container level (e.g. in Definition), because I think that's overkill:
this is just a nice decoration around the system. The system is also NOT included
when kernel.debug = false, so there's zero prod overhead. The debug:autowiring
command still works in this situation, but falls back to its original "one big list".

Cheers!

screen shot 2018-02-11 at 2 57 53 pm

This introduces a new interface + tag that can be used by bundles to inform the core about the *purpose* of the autowirable classes/interfaces.

This is makes debug:autowiring human-focused: showing you a list of
the autowireable services based on the *purpose* of them.
@weaverryan weaverryan force-pushed the weaverryan:autowiring-debug-improved branch from 6e10bf2 to dffc922 Feb 11, 2018
@@ -15,6 +15,8 @@
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
use Symfony\Component\Console\Style\SymfonyStyle;
use Symfony\Component\DependencyInjection\Debug\AutowiringInfoManager;
use Symfony\Component\DependencyInjection\Debug\AutowiringTypeInfo;

This comment has been minimized.

Copy link
@bmax

bmax Feb 11, 2018

looks like this isn't used.

This comment has been minimized.

Copy link
@weaverryan

weaverryan Feb 11, 2018

Author Member

They are both used - one I think just for phpdoc :). We have a PR bot (fabbot) that validates cs, including unused use statements

This comment has been minimized.

Copy link
@bmax

bmax Feb 11, 2018

Oh how come include namespaces for just docblocks? Interesting!

This comment has been minimized.

Copy link
@weaverryan

weaverryan Feb 11, 2018

Author Member

It lets you use the short class names in phpdoc - like @param Foo - editors will use the use statement to know the full class name that you’re referencing :)

Copy link
Contributor

ro0NL left a comment

Cool :)

*/
class AutowireDebugInfoPass implements CompilerPassInterface
{
const AUTOWIRING_INFO_PROVIDER_TAG = 'debug.autowiring_info_provider';

This comment has been minimized.

Copy link
@ro0NL

ro0NL Feb 11, 2018

Contributor

public const But needed? This is the first tag const right...

*
* @return AutowiringTypeInfo|null
*/
public function getInfo(string $type)

This comment has been minimized.

Copy link
@ro0NL

ro0NL Feb 11, 2018

Contributor

: ?AutowiringTypeInfo (docblock is redundant)

This comment has been minimized.

Copy link
@weaverryan

weaverryan Feb 12, 2018

Author Member

+1 I keep thinking Symfony requires 7.0, so no nullable types :). I'll change this happily

return isset($this->typeInfos[$type]) ? $this->typeInfos[$type] : null;
}

private function populateTypesInfo()

This comment has been minimized.

Copy link
@ro0NL

ro0NL Feb 11, 2018

Contributor

we dont do :void right?

This comment has been minimized.

Copy link
@stof

stof Feb 14, 2018

Member

we could for new APIs existing only in 4.1+

final class AutowiringTypeInfo
{
private $type;

This comment has been minimized.

Copy link
@ro0NL

ro0NL Feb 11, 2018

Contributor

new lines to be removed

*/
public static function create(string $type, string $name, int $priority = 0)
{
return new static($type, $name, $priority);

This comment has been minimized.

Copy link
@ro0NL

ro0NL Feb 11, 2018

Contributor

new self will do as it's final


private $typeInfos = null;

public function __construct(array $autowiringInfoProviders)

This comment has been minimized.

Copy link
@ro0NL

ro0NL Feb 11, 2018

Contributor

iterable to leverage type tagged in DI

@@ -23,5 +23,13 @@
<argument type="service" id="debug.argument_resolver.inner" />
<argument type="service" id="debug.stopwatch" />
</service>

<service id="debug.autowiring_info_manager" class="Symfony\Component\DependencyInjection\Debug\AutowiringInfoManager">
<argument /> <!-- argument info providers -->

This comment has been minimized.

Copy link
@ro0NL

ro0NL Feb 11, 2018

Contributor

type="tagged"?

return;
}

$io->title('Key Services');

This comment has been minimized.

Copy link
@ro0NL

ro0NL Feb 11, 2018

Contributor

i'd say Main Services / Common Services / General Services or so =/

AutowiringTypeInfo::create(Registry::class, 'Workflow')
->setDescription('use to fetch workflows'),

AutowiringTypeInfo::create(Registry::class, 'Workflow')

This comment has been minimized.

Copy link
@ro0NL

ro0NL Feb 11, 2018

Contributor

duplicated :)

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 11, 2018

That's great!
I'm wondering two things:

  • is this really related to autowiring? Any services could be described like this, and the other commands could also leverage the info, isn't it?
  • reciprocally, this reminds me a lot #24562, which needs just additional but very similar extra info. Would be great to have only one such system. (Could be just extending Definition in fact, not sure it would be overkill :))
@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Feb 11, 2018

For me the new priority will do (compared to suggested autowireable semantic in #24562 (comment)), related to that i was thinking to group services by info provider (visually in CLI with a label), that way it's already prioritized by provider (tag priority) and the new priority value is not really needed.

is this really related to autowiring?

ServiceInfoProviderInterface seems better indeed

Could be just extending Definition in fact, not sure it would be overkill :)

Tend to like having all info at one place, compared to potentially scattered around all over. But doing it per service definition with definition config is reasonable (overkill IMHO).

@javiereguiluz javiereguiluz added the DX label Feb 12, 2018
@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Feb 12, 2018
@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Feb 12, 2018

This is great. So nice that it shouldn't be limited to autowiring. It can be useful for other services, in other contexts (I think to the Graphviz dumper, and to the debug:container command). I agree with @nicolas-grekas, it would be nice to be able to set the description directly in the XML or YAML file. Then the command can just put the ones with a description first.

@weaverryan

This comment has been minimized.

Copy link
Member Author

weaverryan commented Feb 14, 2018

I’m going to rethink this in the context of all services, and possibly a new debug:services command that dumps things in a new way.

Thanks for the suggestions :)

@artursvonda

This comment has been minimized.

Copy link
Contributor

artursvonda commented Aug 2, 2018

Could we leverage PHPDocs of classes for this? As a fallback. Strip out tags, for example. But class (or interface) purpose probably matches service purpose.
Or do we want to keep those things separate?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Oct 24, 2018

Closing in favor of #28970, thanks for pushing!

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
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

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