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

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

Open
wants to merge 4 commits into
base: master
from

Conversation

@Islam93
Copy link
Contributor

Islam93 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" is not replaced for service "App\Test\Test" yet: should be defined by TestPass
if argument was not replaced by compiler pass

...
 public function process(ContainerBuilder $container)
 {
     $test = $container->getDefinition(Test::class);
     $test->setArgument('$a', 'test');
 }
...
@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 left a comment

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

This comment has been minimized.

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.

@Islam93

This comment has been minimized.

Copy link
Contributor Author

Islam93 commented Dec 26, 2019

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

@Islam93

This comment has been minimized.

Copy link
Contributor Author

Islam93 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?

@Islam93

This comment has been minimized.

Copy link
Contributor Author

Islam93 commented Dec 29, 2019

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

@Islam93 Islam93 force-pushed the Islam93:ticket-31769_abstarct-arguments-in-xml-config branch from 8ea71a0 to dd1f2cb Jan 4, 2020
@Islam93 Islam93 requested a review from nicolas-grekas Jan 4, 2020
/**
* Throws an exception for any service that have an abstract argument.
*/
class AbstractableArgumentPresenceExceptionPass extends AbstractRecursivePass

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 12, 2020

Member

I think the pass is not needed because consumers of containers must deal with AbstractableArgument anyway.
ContainerBuilder is missing the same check as PhpDumper btw

This comment has been minimized.

Copy link
@Islam93

Islam93 Jan 17, 2020

Author Contributor

I think the pass is not needed because consumers of containers must deal with AbstractableArgument anyway.

@nicolas-grekas, pass just need to be removed and the only checkpoints with exceptions are ContainerBuilder and PhpDumper?
it seemed to me that the main idea of abstract argument is to check at container compilation time and compiled container must not contain an abstract argument. or we can rely on exception in PhpDumper during compilation?

ContainerBuilder is missing the same check as PhpDumper btw

ContainerBuilder::doResolveServices() looks like the only suitable for an exception, but it will not thrown at compilation time

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 17, 2020

Member

pass just need to be removed and the only checkpoints with exceptions are ContainerBuilder and PhpDumper?

correct

the main idea of abstract argument is to check at container compilation time

where the check is done is an implementation detail - actually eg XmlDumper doesn't want throwing to happen.
The point of abstract arguments is to allow defining arguments that must be defined in a DI extension or in a pass. The rest is implementation details.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 17, 2020

Member

ContainerBuilder::doResolveServices() looks like the only suitable for an exception, but it will not thrown at compilation time

correct - and we don't care about throwing at compile time :)

This comment has been minimized.

Copy link
@Islam93

Islam93 Jan 18, 2020

Author Contributor

fixed

@Islam93 Islam93 requested a review from nicolas-grekas Jan 17, 2020
@Islam93 Islam93 force-pushed the Islam93:ticket-31769_abstarct-arguments-in-xml-config branch from dd1f2cb to 767e456 Jan 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.