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

Update optional_dependencies.rst #19954

Closed
wants to merge 1 commit into from
Closed

Conversation

hbgamra
Copy link
Contributor

@hbgamra hbgamra commented Jun 12, 2024

Update optional Dependency param

Update optional Dependency param
@xabbuh
Copy link
Member

xabbuh commented Jun 12, 2024

What is the reason for this change? I mean why should one pass null to a setLogger() method?

@hbgamra
Copy link
Contributor Author

hbgamra commented Jun 12, 2024

You are right, but ultimately the parameter can accept null and is likely to be null

@xabbuh
Copy link
Member

xabbuh commented Jun 13, 2024

Why is that likely? I’d argue that if you don’t want to set a logger, don’t call this method. For now, I don’t see a reason why we should change the docs, but you can of course do it differently in your code if you prefer to.

Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

Why should I call an optional method with an optional parameter? I would Justinian the setLogger() method call

@hbgamra
Copy link
Contributor Author

hbgamra commented Jun 13, 2024

You raise a valid point. Calling an optional method with an optional parameter may seem redundant and it's not good

@javiereguiluz
Copy link
Member

This example is part of this section:

Ignoring Missing Dependencies
-----------------------------

The behavior of ignoring missing dependencies is the same as the "null" behavior
except when used within a method call, in which case the method call itself
will be removed.

[...]

So, in this example, the argument of public function setLogger(LoggerInterface $logger): void doesn't have to be nullable because, if $logger is null, the method will never be called. So, the method will never receive a null argument.

If you use this method in other parts of the app, then the argument could be nullable depending on your code. But, for the code example of this doc section, is not strictly needed. That's why I'0m closing this PR as won't fix. @hbgamra if you think I missed something, please tell me and we'll reopen this. Thanks!

@hbgamra hbgamra deleted the patch-22 branch June 13, 2024 14:06
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

5 participants