Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[DependencyInjection] Invokable Factory Services#30255

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@zanbaldwin
Copy link
Member

@zanbaldwinzanbaldwin commentedFeb 15, 2019•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets
LicenseMIT
Doc PRsymfony/symfony-docs#11014

Failing test is in the Twig bridge, and outside of the the scope of this PR.

Allow referencing invokable factory services, just as route definitions reference invokable controllers.
This functionality was also added for service configurators for consistency.

Example

<?phpnamespaceApp\Factory;class ServiceFactory{publicfunction__invoke(bool$debug)    {returnnewService($debug);    }}
services:App\Service:# Prepend with "@" to differentiate between service and function.factory:'@App\Factory\ServiceFactory'arguments:[ '%kernel.debug%' ]
<?xml version="1.0" encoding="UTF-8" ?><containerxmlns="http://symfony.com/schema/dic/services"xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"xsi:schemaLocation="http://symfony.com/schema/dic/services               http://symfony.com/schema/dic/services/services-1.0.xsd">    <services><!-- ...-->        <serviceid="App\Service"class="App\Service">            <factoryservice="App\Factory\ServiceFactory" />        </service>    </services></container>
<?phpuseApp\Service;useApp\Factory\ServiceFactory;useSymfony\Component\DependencyInjection\Reference;$container->register(Service::class, Service::class)    ->setFactory(newReference(ServiceFactory::class));

theofidry, vudaltsov, and olvlvl reacted with thumbs up emoji
@zanbaldwinzanbaldwin changed the titleInvokable Factory Services[DependencyInjection] Invokable Factory ServicesFeb 16, 2019
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedFeb 16, 2019•
edited
Loading

Looks like there are no reasons not to do it to me.
Some notes to help:

  • should target master
  • the implementation should be quite different: there should be no need for anything special inAbstractRecursivePass. Instead, we should makeContainerBuilder andPhpDumper handle whenDefinition::getFactory() returns a reference. That should be enough.

@nicolas-grekasnicolas-grekas added this to thenext milestoneFeb 16, 2019
@zanbaldwinzanbaldwinforce-pushed thefeature/callable-factory-services branch from57e42e9 to98693beCompareFebruary 18, 2019 14:11
@zanbaldwinzanbaldwin changed the base branch from3.4 tomasterFebruary 18, 2019 14:15
@zanbaldwin
Copy link
MemberAuthor

  • Now targettingmaster
  • AbstractRecursivePass was expecting an array, now handles aReference object.
  • Now sure how to move that logic out intoContainerBuilder andPhpDumper unfortunately 🙁

@nicolas-grekas
Copy link
Member

AbstractRecursivePass was expecting an array, now handles aReference object.

actually, maybe this should be handled 100% at the loader level? If service reference then method defaults to__invoke?

@zanbaldwinzanbaldwinforce-pushed thefeature/callable-factory-services branch from9e2edb7 to3b7bc89CompareFebruary 18, 2019 17:48
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Nice. Could you please add some test cases? Then it will look to good me.

$factory =explode('::',$factory,2);
}

if ($factoryinstanceof Reference) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could beelseif. If\is_string($factory) resolves to true,instanceof will never be true. Although it does not really matter, it seems to be more logical to me, WDYT?

XuruDragon reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it should be an elseif logically to avoid an extra if condition.
Also I would suggest to extract the duplicate parsing logic in setConfigurator and setFactory into a private method.

vudaltsov reacted with thumbs up emoji
@zanbaldwin
Copy link
MemberAuthor

Undrafting PR because I'm gettingClass 'PHPUnit_Framework_TestCase' not found errors locally.

@zanbaldwinzanbaldwin marked this pull request as ready for reviewFebruary 19, 2019 11:29
@zanbaldwinzanbaldwinforce-pushed thefeature/callable-factory-services branch 3 times, most recently froma470019 toc1ccda1CompareFebruary 19, 2019 12:03
zanbaldwin added a commit to zanbaldwin/symfony-docs that referenced this pull requestFeb 19, 2019
Document new invokable factories (and configurators) implemented insymfony/symfony#30255.
zanbaldwin added a commit to zanbaldwin/symfony-docs that referenced this pull requestFeb 19, 2019
Document new invokable factories (and configurators) implemented insymfony/symfony#30255.
@zanbaldwinzanbaldwinforce-pushed thefeature/callable-factory-services branch fromc1ccda1 toc65fb8eCompareFebruary 19, 2019 16:03
@zanbaldwinzanbaldwinforce-pushed thefeature/callable-factory-services branch fromc65fb8e to16b7fcfCompareFebruary 22, 2019 16:31
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

(once the remaining comments are addressed)

@zanbaldwinzanbaldwinforce-pushed thefeature/callable-factory-services branch 2 times, most recently from2e81b0f to801bb8aCompareFebruary 25, 2019 10:59
@zanbaldwin
Copy link
MemberAuthor

Done, and rebased latestmaster.

$factory =explode('::',$factory,2);
}

if ($factoryinstanceof Reference) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it should be an elseif logically to avoid an extra if condition.
Also I would suggest to extract the duplicate parsing logic in setConfigurator and setFactory into a private method.

vudaltsov reacted with thumbs up emoji
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

(I just fixed the comments about the "if")

@fabpotfabpotforce-pushed thefeature/callable-factory-services branch fromb47aa9b to23cb83fCompareApril 3, 2019 12:19
@fabpot
Copy link
Member

Thank you@zanbaldwin.

@fabpotfabpot merged commit23cb83f intosymfony:masterApr 3, 2019
fabpot added a commit that referenced this pull requestApr 3, 2019
…aldwin)This PR was squashed before being merged into the 4.3-dev branch (closes#30255).Discussion----------[DependencyInjection] Invokable Factory Services| Q             | A| ------------- | ---| Branch?       | `master`| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |symfony/symfony-docs#11014> Failing test is in the Twig bridge, and outside of the the scope of this PR.Allow referencing invokable factory services, just as route definitions reference invokable controllers.This functionality was also added for service configurators for consistency.## Example```php<?phpnamespace App\Factory;class ServiceFactory{    public function __invoke(bool $debug)    {        return new Service($debug);    }}``````yamlservices:    App\Service:        # Prepend with "@" to differentiate between service and function.        factory: '@app\Factory\ServiceFactory'        arguments: [ '%kernel.debug%' ]``````xml<?xml version="1.0" encoding="UTF-8" ?><container xmlns="http://symfony.com/schema/dic/services"           xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"           xsi:schemaLocation="http://symfony.com/schema/dic/serviceshttp://symfony.com/schema/dic/services/services-1.0.xsd">    <services>        <!-- ... -->        <service                >            <factory service="App\Factory\ServiceFactory" />        </service>    </services></container>``````php<?phpuse App\Service;use App\Factory\ServiceFactory;use Symfony\Component\DependencyInjection\Reference;$container->register(Service::class, Service::class)    ->setFactory(new Reference(ServiceFactory::class));```Commits-------23cb83f [DependencyInjection] Invokable Factory Services
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestApr 5, 2019
…dwin)This PR was merged into the master branch.Discussion----------[DependencyInjection] Invokable Factory ServicesDocument new invokable factories (and configurators) implemented insymfony/symfony#30255.There are now four ways to reference factories, perhaps making them more clear might be something else to do.- `function`- `Class::method`- `Service:method`- `@InvokableService`Commits-------2d7709f [DependencyInjection] Invokable Factory Services
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot mentioned this pull requestMay 9, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+3 more reviewers

@vudaltsovvudaltsovvudaltsov left review comments

@XuruDragonXuruDragonXuruDragon left review comments

@TobionTobionTobion requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

8 participants

@zanbaldwin@nicolas-grekas@fabpot@stof@Tobion@vudaltsov@XuruDragon@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp