Skip to content

[DI] added possibility to define services with abstract arguments #35076

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

Conversation

fu22ybear
Copy link
Contributor

@fu22ybear fu22ybear commented Dec 21, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #31769
License MIT
Doc PR n/a

feature caused by rfc #31769 from issues list
I hope, this PR will be useful

Abstract argument have to replaced by one of compiler passes or exception will be thrown.
Example:
This service definition

...
 <service id="App\Test\Test">
    <argument key="$a" type="abstract">should be defined by TestPass</argument>
 </service>
...

or this for yaml

    App\Test\Test:
        arguments:
            $a: !abstract should be defined by TestPass

causes exception like Argument "$a" of service "App\Test\Test" is abstract (should be defined by TestPass), did you forget to define it?
if argument was not replaced by compiler pass

...
 public function process(ContainerBuilder $container)
 {
     $test = $container->getDefinition(Test::class);
     $test->setArgument('$a', 'test');
 }
...

@carsonbot carsonbot added Status: Needs Review RFC RFC = Request For Comments (proposals about features that you want to be discussed) DependencyInjection Feature labels Dec 21, 2019
@nicolas-grekas nicolas-grekas added this to the next milestone Dec 21, 2019
@nicolas-grekas nicolas-grekas changed the title [RFC][DI] added possibility to define service with abstract argument … [DI] added possibility to define services with abstract arguments Dec 21, 2019
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

the YamlLoader and YamlDumper should also be able to deal with abstract arguments
and ContainerBuilder should throw an exception when it encounters one when creating a service.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 26, 2019

Before spending more time here: is it worth it?
This looks like a lot of code, for something that is of low value to me.

@fu22ybear
Copy link
Contributor Author

fu22ybear commented Dec 26, 2019

most part of changes are tests.
abstract argument definition looks more clear and obvious and I'd like to continue

@fu22ybear
Copy link
Contributor Author

fu22ybear commented Dec 29, 2019

and ContainerBuilder should throw an exception when it encounters one when creating a service.

@nicolas-grekas, what if specific pass will be registered in removing passes list, like DefinitionErrorExceptionPass, to check an abstract argument existence? and this pass will throw an exception, if an abstract argument is not replaced yet

if this behavior is suitable, do dumpers still have to able to deal with abstract arguments?

@fu22ybear
Copy link
Contributor Author

the YamlLoader and YamlDumper should also be able to deal with abstract arguments

example for yaml config

    App\Test\Test:
        arguments:
            $a: !abstract should be defined by TestPass

tags functionality looks suitable

@fu22ybear fu22ybear force-pushed the ticket-31769_abstarct-arguments-in-xml-config branch from 8ea71a0 to dd1f2cb Compare January 4, 2020 18:15
@fu22ybear fu22ybear force-pushed the ticket-31769_abstarct-arguments-in-xml-config branch from dd1f2cb to 767e456 Compare January 18, 2020 14:41
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

some final comments :)

@fu22ybear fu22ybear force-pushed the ticket-31769_abstarct-arguments-in-xml-config branch from 767e456 to 5982780 Compare January 25, 2020 08:38
@nicolas-grekas nicolas-grekas force-pushed the ticket-31769_abstarct-arguments-in-xml-config branch from 5982780 to 62fefaa Compare February 5, 2020 19:16
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Looks good to me! (I made some minor tweak FYI)

Copy link
Contributor

@maxhelias maxhelias left a comment

Choose a reason for hiding this comment

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

Interesting :)

@alexander-schranz
Copy link
Contributor

@Islam93 Nice 👍 thank you having a look at this issue.

@fabpot
Copy link
Member

fabpot commented Feb 6, 2020

Thank you @Islam93.

fabpot added a commit that referenced this pull request Feb 6, 2020
…t arguments (Islam93)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[DI] added possibility to define services with abstract arguments

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fix #31769
| License       | MIT
| Doc PR        | n/a

feature caused by rfc #31769 from issues list
I hope, this PR will be useful

   Abstract argument have to replaced by one of compiler passes or exception will be thrown.
   Example:
   This service definition
   ```xml
   ...
    <service id="App\Test\Test">
       <argument key="$a" type="abstract">should be defined by TestPass</argument>
    </service>
   ...
   ```
or this for yaml
```yaml
    App\Test\Test:
        arguments:
            $a: !abstract should be defined by TestPass
```
   causes exception like `Argument "$a" of service "App\Test\Test" is abstract (should be defined by TestPass), did you forget to define it?`
   if argument was not replaced by compiler pass
   ```php
   ...
    public function process(ContainerBuilder $container)
    {
        $test = $container->getDefinition(Test::class);
        $test->setArgument('$a', 'test');
    }
   ...
   ```

Commits
-------

62fefaa [DI] added possibility to define services with abstract arguments
@fabpot fabpot merged commit 62fefaa into symfony:master Feb 6, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DependencyInjection Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] [DI] Abstract arguments
6 participants