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

Allow ServiceDefinition::setArguments() to use previously set arguments #171

Closed
czukowski opened this issue Aug 3, 2018 · 6 comments

Comments

@czukowski
Copy link
Contributor

commented Aug 3, 2018

Currently, it is possible to use setArguments() method of ServiceDefinition to set partial arguments and have the rest autowired:

$builder->addDefinition(...)
    ->setFactory(...)
    ->setArguments([
        2 => $config['value']
    ])

However, if we want to modify other arguments later, we can't use the same method because it'll throw away anything set previously, ie:

$builder->getDefinition(...)
    ->setArguments([
        3 => $config['another value']  // 2nd argument is now lost.
    ])

I think this behavior may be confusing since method principally does allow to set an incomplete list of arguments.

It is possible to do something like this, but it looks awkward to actually use such construct as the rest of the API is nice and concise:

$builder->getDefinition(...)
    ->getFactory()
    ->arguments[3] = $config['another value'];  // 2nd argument is now preserved.

On the other hand, being able to 'unset' previously set arguments may still be useful, and for that use case setArguments still does the job. Maybe we could have a different method to merge arguments passed into the current list, such as addArguments or mergeArguments? (these two names don't sound perfect though)

@czukowski

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2018

Come to think of it, if the use case is to modify an argument (or arguments), a good method would be ServiceDefinition::setArgument($key, $value) to set or replace the specified positional or named argument in service factory.

If the idea sounds okay, I can submit a pull request, please let me know.

@dg

This comment has been minimized.

Copy link
Member

commented Aug 5, 2018

I think this behavior may be confusing since method principally does allow to set an incomplete list of arguments.

It is intention. Other parameters may be added via autowiring.

@czukowski

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

@dg what do you think about my other comment? I agree that changing the behavior of setArguments is not desired for some cases and would be a breaking change anyway. So let me sum up my reasoning:

While autowiring is one of the greatest features Nette has, it cannot be the answer for everything. There are at least two use cases when autowiring can't be used:

  1. You may want to change a parameter that cannot be autowired such as array or scalar value
  2. In some cases you may wish to avoid the overhead of defining a service and generate the arguments using Statement objects.

Acknowledging those use cases, a need for a convenient method to set individual arguments becomes apparent. I think adding a method to ServiceDefinition like setArgument($key, $value) would serve that need well.

@dg

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

The key thing is, if you have a real usecase where it would be really handy.

@czukowski

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

The 'real' use case is to be able to modify services created by other extensions. I prefer creating a separate extension when the number of required modifications reaches a certain amount (while still being logically related), rather than dumping it all to a config file. Same if I need to extend a 3rd party extension in a considerable way.

@dg

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

Ok, send PR :-)

@dg dg closed this in 2ea7e11 Feb 19, 2019

dg added a commit that referenced this issue Feb 19, 2019

ServiceDefinition::setArgument() added [Closes #171][Closes #172]
Add method to set individual service factory arguments

dg added a commit that referenced this issue Feb 20, 2019

ServiceDefinition::setArgument() added [Closes #171][Closes #172]
Add method to set individual service factory arguments

dg added a commit that referenced this issue Feb 20, 2019

ServiceDefinition::setArgument() added [Closes #171][Closes #172]
Add method to set individual service factory arguments

dg added a commit that referenced this issue Feb 20, 2019

ServiceDefinition::setArgument() added [Closes #171][Closes #172]
Add method to set individual service factory arguments

dg added a commit that referenced this issue Feb 28, 2019

ServiceDefinition::setArgument() added [Closes #171][Closes #172]
Add method to set individual service factory arguments

dg added a commit that referenced this issue Mar 5, 2019

ServiceDefinition::setArgument() added [Closes #171][Closes #172]
Add method to set individual service factory arguments

dg added a commit that referenced this issue Mar 5, 2019

ServiceDefinition::setArgument() added [Closes #171][Closes #172]
Add method to set individual service factory arguments

dg added a commit that referenced this issue Mar 5, 2019

ServiceDefinition::setArgument() added [Closes #171][Closes #172]
Add method to set individual service factory arguments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.