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 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.
@@ -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;
Copy link

Choose a reason for hiding this comment

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

looks like this isn't used.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

Oh how come include namespaces for just docblocks? Interesting!

Copy link
Member Author

Choose a reason for hiding this comment

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

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 ro0NL left a comment

Choose a reason for hiding this comment

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

Cool :)

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

: ?AutowiringTypeInfo (docblock is redundant)

Copy link
Member Author

Choose a reason for hiding this comment

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

+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()
Copy link
Contributor

Choose a reason for hiding this comment

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

we dont do :void right?

Copy link
Member

Choose a reason for hiding this comment

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

we could for new APIs existing only in 4.1+

final class AutowiringTypeInfo
{
private $type;

Copy link
Contributor

Choose a reason for hiding this comment

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

new lines to be removed

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

Choose a reason for hiding this comment

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

new self will do as it's final


private $typeInfos = null;

public function __construct(array $autowiringInfoProviders)
Copy link
Contributor

Choose a reason for hiding this comment

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

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 -->
Copy link
Contributor

Choose a reason for hiding this comment

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

type="tagged"?

return;
}

$io->title('Key Services');
Copy link
Contributor

@ro0NL ro0NL Feb 11, 2018

Choose a reason for hiding this comment

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

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')
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicated :)

@nicolas-grekas
Copy link
Member

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 The debug:container command is confusing #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
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 DX = Developer eXperience (anything that improves the experience of using Symfony) label Feb 12, 2018
@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Feb 12, 2018
@dunglas
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
Copy link
Member Author

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 :)

@javiereguiluz javiereguiluz added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Mar 12, 2018
@weaverryan weaverryan removed the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Mar 29, 2018
@nicolas-grekas nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018
@artursvonda
Copy link
Contributor

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
Copy link
Member

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
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature Status: Needs Work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants