Skip to content

[DependencyInjection] Document FQCN aliases #7656

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 28, 2017
Merged

[DependencyInjection] Document FQCN aliases #7656

merged 1 commit into from
Apr 28, 2017

Conversation

GuilhemN
Copy link
Contributor

Fix #7445

Not only do we need to remove autowiring-types, we should show clearly how you can use aliases to choose what class should be autowired for a specific interface/class.

Also, Stof noted that you can/should mark these aliases as private: we don't need them to be available on the final, cached container.

theofidry
theofidry reviewed Mar 19, 2017
View reviewed changes
components/dependency_injection/autowiring.rst Outdated

Acme\TransformerInterface:
alias: rot13_transformer
public: false
Copy link
Contributor

Choose a reason for hiding this comment

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

why bother with public: false? Let's show the short Acme\TransformerInterface: '@ rot13_transformer' instead :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because we don't need this alias in the dumped container, it's better to show the best practices imo ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should bother with this kind of optimization that can lead to confusion here. And as it's not specific to autowiring, I should rather be in a dedicated section rather. The notion of public/private is a bit special in Symfony, I wouldn't mix it with autowiring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, let's remove it for now, we'll see if others think differently.

@theofidry
Copy link
Contributor

👍 just the public: false which is only confusing here to me, it's not specific to auto-wiring and it's not really the topic of that section

xabbuh
xabbuh reviewed Mar 20, 2017
View reviewed changes
components/dependency_injection/autowiring.rst Outdated
autowiring_types: Acme\TransformerInterface
class: Acme\Rot13Transformer

Acme\TransformerInterface: @rot13_transformer
Copy link
Member

Choose a reason for hiding this comment

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

The @rot13_transformer string needs to be quoted.

xabbuh
xabbuh reviewed Mar 20, 2017
View reviewed changes
components/dependency_injection/autowiring.rst Outdated
<service id="rot13_transformer" class="Acme\Rot13Transformer">
<autowiring-type>Acme\TransformerInterface</autowiring-type>
</service>
<service id="rot13_transformer" class="Acme\Rot13Transformer" />
Copy link
Member

Choose a reason for hiding this comment

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

for consistency, we should add a blank line after this too

xabbuh
xabbuh reviewed Mar 20, 2017
View reviewed changes
components/dependency_injection/autowiring.rst Outdated
Fortunately, the FQCN alias is here to specify which implementation
to use by default.

.. versionadded:: 3.2
Copy link
Member

Choose a reason for hiding this comment

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

3.3

xabbuh
xabbuh approved these changes Mar 20, 2017
View reviewed changes
Copy link
Member

@xabbuh xabbuh left a comment

Choose a reason for hiding this comment

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

👍

HeahDude
HeahDude approved these changes Mar 26, 2017
View reviewed changes
Copy link
Contributor

@HeahDude HeahDude left a comment

Choose a reason for hiding this comment

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

👍

@GuilhemN GuilhemN force-pushed the AUTOWIRING branch 3 times, most recently from 99ba301 to a544200 Compare April 12, 2017 17:28
@javiereguiluz
Copy link
Member

@GuilhemN could you please rebase this so we can merge it? Thanks!

@GuilhemN
Copy link
Contributor Author

rebased ☺

@javiereguiluz
Copy link
Member

@GuilhemN thanks for rebasing. This is now merged!

@javiereguiluz javiereguiluz merged commit a530019 into symfony:master Apr 28, 2017
javiereguiluz added a commit that referenced this pull request Apr 28, 2017
@javiereguiluz
minor #7656 [DependencyInjection] Document FQCN aliases (GuilhemN)
f9dafd6 
This PR was merged into the master branch.

Discussion
----------

[DependencyInjection] Document FQCN aliases

Fix #7445

> Not only do we need to remove autowiring-types, we should show clearly how you can use aliases to choose what class should be autowired for a specific interface/class.
>
> Also, Stof noted that you can/should mark these aliases as private: we don't need them to be available on the final, cached container.

Commits
-------

a530019 [DependencyInjection] Document FQCN aliases
@GuilhemN GuilhemN deleted the AUTOWIRING branch April 28, 2017 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theofidry theofidry theofidry left review comments

@xabbuh xabbuh xabbuh approved these changes

@HeahDude HeahDude HeahDude approved these changes

Assignees
No one assigned
Labels
DependencyInjection Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@GuilhemN @theofidry @javiereguiluz @xabbuh @HeahDude @carsonbot