Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[DI][FrameworkBundle] Show autowired methods in descriptors#21315

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

Merged
fabpot merged 1 commit intosymfony:masterfromogizanagi:feature/fwb/di/autowired_methods_descr
Feb 16, 2017
Merged

[DI][FrameworkBundle] Show autowired methods in descriptors#21315

fabpot merged 1 commit intosymfony:masterfromogizanagi:feature/fwb/di/autowired_methods_descr
Feb 16, 2017

Conversation

@ogizanagi
Copy link
Contributor

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsN/A
LicenseMIT
Doc PRN/A

so we can see the autowired methods usingdebug:container.

if (method_exists($definition,'isAutowired')) {
$output .="\n".'- Autowired:'.($definition->isAutowired() ?'yes' :'no');

if (count($autowiredMethods =$definition->getAutowiredMethods()) >0) {

Choose a reason for hiding this comment

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

No need for count

$tableRows[] =array('Autowired',$definition->isAutowired() ?'yes' :'no');

$autowiredMethods =$definition->getAutowiredMethods();
$tableRows[] =array('Autowired Methods',count($autowiredMethods) ?implode(',',$autowiredMethods) :'-');

Choose a reason for hiding this comment

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

count


if (method_exists($definition,'isAutowired')) {
$serviceXML->setAttribute('autowired',$definition->isAutowired() ?'true' :'false');
if (count($autowiredMethods =$definition->getAutowiredMethods()) >0) {

Choose a reason for hiding this comment

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

count

Shared yes
Abstract no
Autowired no
Autowired Methods -
Copy link
Member

@nicolas-grekasnicolas-grekasJan 17, 2017
edited
Loading

Choose a reason for hiding this comment

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

Given the fact that the constructor is now always autowired, I think we need a single key, autowire (no "d"), that either contains "yes", or the list of methods, constructor present only if present in the user declaration.
If possible. WDYT?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I think you're right. Thanks for the suggestion 👍

Copy link
ContributorAuthor

@ogizanagiogizanagiJan 17, 2017
edited
Loading

Choose a reason for hiding this comment

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

I've updated the text and markdown descriptors. Does it match your expectations?


$output .="\n".'- Abstract:'.($definition->isAbstract() ?'yes' :'no');

if (method_exists($definition,'isAutowired')) {
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@dunglas : Do you remember why this check was needed?
To me it looks weird, because it was introduced in 2.8, where the FrameworkBundle already requiredsymfony/dependency-injection: "~2.8", so we can't get aDefinition instance not having this method.

Maybe I'm missing something, but it looks safe to remove.

Copy link
Member

Choose a reason for hiding this comment

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

Probably that FrameworkBundle wasn't requiring DI v2.8 when I started working on autowiring. I'll double check (and open a PR if you want me to) but I think that it can be removed now.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

No problem, I can open one. I just wanted to be sure I didn't miss something. It seems the same happened for theshared property.
Thank you. :)

if (method_exists($definition,'isAutowired')) {
$output .="\n".'- Autowired:'.($definition->isAutowired() ?'yes' :'no');
$output .="\n".'- Autowire:';
$output .=$definition->isAutowired() ?implode(',',array_map(function ($method) {

Choose a reason for hiding this comment

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

__construct should be removed from the output - and when only__construct is autowired, we should display a simple "yes"
same for other descriptors imho

dunglas added a commit that referenced this pull requestJan 28, 2017
…dunglas)This PR was merged into the 2.8 branch.Discussion----------[FrameworkBundle] Remove useless checks in descriptors| Q             | A| ------------- | ---| Branch?       | 2.8| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | n/a| License       | MIT| Doc PR        | n/aAs pointed out by@ogizanagi in#21315 (comment), some code in FrameworkBundle's descriptors is useless because the bundle works only with DI 2.8.Commits-------c759345 [FrameworkBundle] Remove useless checks in descriptors
@ogizanagi
Copy link
ContributorAuthor

(rebased)

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

👍 (maybe just rebase to ensure tests are still green after recent changes?)

@ogizanagi
Copy link
ContributorAuthor

Rebased done. Everything seems to be green.
As theautowiredMethods has changed forautowiredCalls, I did the same in the xml format (autowire-call(s) tags).

return$method !=='__construct';
});
if ($autowiredCalls) {
$serviceXML->appendChild($autowiredMethodsXML =$dom->createElement('autowired-calls'));

Choose a reason for hiding this comment

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

not sure we need this wrapper tag btw

Copy link
ContributorAuthor

@ogizanagiogizanagiFeb 14, 2017
edited
Loading

Choose a reason for hiding this comment

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

Me neither. But that's consistent with the waytags,calls, ... are dumped. (Seehttps://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Descriptor/definition_arguments_2.xml for instance)

@fabpot
Copy link
Member

Thank you@ogizanagi.

@fabpotfabpot merged commit482435c intosymfony:masterFeb 16, 2017
fabpot added a commit that referenced this pull requestFeb 16, 2017
…ors (ogizanagi)This PR was merged into the 3.3-dev branch.Discussion----------[DI][FrameworkBundle] Show autowired methods in descriptors| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | N/A| License       | MIT| Doc PR        | N/Aso we can see the autowired methods using `debug:container`.Commits-------482435c [DI][FrameworkBundle] Show autowired methods in descriptors
@ogizanagiogizanagi deleted the feature/fwb/di/autowired_methods_descr branchFebruary 16, 2017 14:19
nicolas-grekas added a commit to nicolas-grekas/symfony that referenced this pull requestFeb 25, 2017
…ods in descriptors (ogizanagi)"This reverts commita27accf, reversingchanges made tob056d40.
nicolas-grekas added a commit to nicolas-grekas/symfony that referenced this pull requestFeb 28, 2017
…ods in descriptors (ogizanagi)"This reverts commita27accf, reversingchanges made tob056d40.
fabpot added a commit that referenced this pull requestMar 1, 2017
…quired` annotation (nicolas-grekas)This PR was squashed before being merged into the 3.3-dev branch (closes#21763).Discussion----------[DI] Replace wildcard-based methods autowiring by `@required` annotation| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no (affects things that are only on master)| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -While playing a bit with new features in master around DI configuration, several people around me got bitten by wildcard-based autowiring. The typical example is adding `autowire: [set*]` in `_defaults`: use that on `resource: ../src/Command/` PSR4-based loading and boom, `setApplication` and `setHelperSet` will now be wrongly called. You could tell me "of course, don't to that" - but being bitten so early on a master-only feature makes me really unconfident that this will be easy enough for people after the release.If wildcard-based autowiring is removed, then I don't see anymore the need for allowing arrays as in `autowire: [setFoo,getBar]`. Moreover, this array syntax has a core DX issue: it's a dead end as far as the learning curve is concerned. You learn it, then when becoming a more advanced dev, someone teaches you that you'd better use another syntax: explicit wiring.And in fact, we don't need it at all, because something else already exists: just declare a method call, but don't define its arguments. If `autowire: true` is set, then the AutowiringPass already fills in the holes. There is only one tweak required to make this work: don't autowire optional arguments for method calls - or that'd be a BC break. To my PoV that's even better: this makes autowiring fit a "do the minimum to make it work" strategy. A really good one to me.But there is still an issue: wildcard-based autowiring fits a need. Namely, it allows one to define a convention (eg. `'set*'`), and have all such methods that follow the convention be autowired. To me, this looks like doing it reverse (the DI config should adapt to the code, not reverse). So, to fill this need, let the declaration be in the source: just use an annotation!This PR adds support for the `@required` annotation, borrowed from the Spring framework:https://www.tutorialspoint.com/spring/spring_required_annotation.htmUsing the annotation is totally optional of course. If you do, *and if autowiring is on*, then it'll be autowired. If you don't, nothing changes: do manual wiring.Even when not using autowiring, the annotation is still a nice hint for the consumer of your classes: it tells the reader that this method needs to be called for correct instantiation - thus lowering one drawback of setter injection (discoverability).The implementation of the annotation parsing is done using a few regexp (no dep on any complex parser) - and works with inheritance, by leveraging the `@inheritdoc` tag (the default behavior being to *not* inherit anything from parent methods).All in all, looking at the diff stats, it makes everything simpler. Good sign, isn't it?Commits-------f286fcc [DI] Replace wildcard-based methods autowiring by `@required` annotation9081699 Revert "minor#21315 [DI][FrameworkBundle] Show autowired methods in descriptors (ogizanagi)"
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@dunglasdunglasdunglas left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

5 participants

@ogizanagi@fabpot@dunglas@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp