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

[DependencyInjection] Improved "optional argument" documentation #6511

Merged
merged 1 commit into from
May 9, 2016

Conversation

dantleech
Copy link
Contributor

@dantleech dantleech commented Apr 23, 2016

Q A
Doc fix? yes
New docs? yes
Applies to 2.3+/master

The on-ignore=null behavior of the DI container arguments is currently undocumented.

TODO:

  • Add - When used on a collection argument, the argument will be removed. list item in master.

@dantleech
Copy link
Contributor Author

Not sure if this should be expanded into a code block showing the on null configuration for XML, YAML and PHP.

@xabbuh
Copy link
Member

xabbuh commented Apr 23, 2016

Didn't you mean on-invalid instead of on-ignore? I think we would then also need to clarify the difference between IGNORE_ON_INVALID_REFERENCE and NULL_ON_INVALID_REFERENCE.

@dantleech
Copy link
Contributor Author

@xabbuh yeah that is what i meant - should this be expanded into a new section with a new set of code examples? or shall i just expand the note?

@xabbuh
Copy link
Member

xabbuh commented Apr 23, 2016

I think a new section with a dedicated example (e.g. a collection argument) that shows the difference would be a good idea.


- When used as a standard argument, the argument will be set to ``null``;
- When used within a method call, the method call itself will be removed;
- When used on a collection argument, the argument will be removed.
Copy link
Contributor Author

@dantleech dantleech Apr 23, 2016

Choose a reason for hiding this comment

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

This is only in Symfony 3.0 master

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think we should omit it here and add it after merging your PR up to master.

@dantleech dantleech changed the title Added note about on-ignore=null [DependencyInjection] Improved "optional argument" documentation Apr 23, 2016
- When used within a method call, the method call itself will be removed;
- When used on a collection argument, the argument will be removed.

Setting missing dependencies to null
Copy link
Member

Choose a reason for hiding this comment

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

Setting Missing Dependencies to null (not sure if we should use literals for null here)

@xabbuh
Copy link
Member

xabbuh commented Apr 24, 2016

I think I would move the "ignoring" strategy after the "set null" strategy. This way, you could say that it's basically the same strategy, but with the following differences (and then lists these differences).

@dantleech dantleech force-pushed the on-ignore-null branch 3 times, most recently from f5329b7 to 0cf3bab Compare April 27, 2016 06:30
@dantleech
Copy link
Contributor Author

Updated, reworded, rearanged and changed the "ignore" example to use a method call.

service does not exist then you may use the ``null`` strategy as follows:

.. configuration-block::

Copy link
Contributor

Choose a reason for hiding this comment

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

code block with yaml is missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this feature isn't supported with YAML afaict.

Copy link
Contributor

Choose a reason for hiding this comment

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

really? if yes, sorry, i didn't know that, but then this would be a good information here ;-)

what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its given just below the code examples :)

On Fri, Apr 29, 2016 at 06:09:01AM -0700, Oskar Stark wrote:

In [1]book/service_container.rst:


Sometimes, one of your services may have an optional dependency, meaning
that the dependency is not required for your service to work properly. In
the example above, the app.mailer service must exist, otherwise an exception
will be thrown. By modifying the app.newsletter_manager service definition,
-you can make this reference optional. The container will then inject it if
-it exists and do nothing if it doesn't:
+you can make this reference optional, there are two strategies for doing this.
+
+Setting Missing Dependencies to null
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+If you want to explicitly set the argument to null in the event that the
+service does not exist then you may use the null strategy as follows:
+
+.. configuration-block::
+

really? if yes, sorry, i didn't know that, but then this would be a good
information here ;-)

what do you think?


You are receiving this because you authored the thread.
Reply to this email directly or [2]view it on GitHub

Reverse link: [3]unknown

References

Visible links

  1. [DependencyInjection] Improved "optional argument" documentation #6511 (comment)
  2. https://github.com/symfony/symfony-docs/pull/6511/files/0cf3bab63f400c6d04ff7baa1664c6f2b0f66d73#r61573614
  3. https://github.com/symfony/symfony-docs/pull/6511/files/0cf3bab63f400c6d04ff7baa1664c6f2b0f66d73#r61573614

@wouterj
Copy link
Member

wouterj commented May 5, 2016

This is the first time I understand the difference, thanks for submitting this patch!

👍

Expanded the documentation for the "on invalid" behaviors for optional
arguments.
@dantleech
Copy link
Contributor Author

Updated

@xabbuh xabbuh merged commit 84b1037 into symfony:2.3 May 9, 2016
xabbuh added a commit that referenced this pull request May 9, 2016
…ntation (dantleech)

This PR was merged into the 2.3 branch.

Discussion
----------

[DependencyInjection] Improved "optional argument" documentation

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | yes
| Applies to    | 2.3+/master

The `on-ignore=null` behavior of the DI container arguments is currently undocumented.

TODO:

- Add `- When used on a collection argument, the argument will be removed.` list item in master.

Commits
-------

84b1037 Improved "optional argument" documentation"
@xabbuh
Copy link
Member

xabbuh commented May 9, 2016

Thank you @dantleech for adding this missing piece of information. I also opened #6550 so that we do not forget to update the docs for Symfony 3.1 with the changes made for invalid reference collection arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants