Skip to content

/ di

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

Change providers strategy #143

Open
wants to merge 7 commits into
base: master
from
Open

Change providers strategy #143

wants to merge 7 commits into from

Conversation

@yiiliveext
Copy link
Contributor

yiiliveext commented Sep 1, 2020

Q A
Is bugfix? ❌
New feature? ❌
Breaks BC? ✔️
@yiiliveext
Change providers strategy
Loading status checks…
14a4d9c
@yiiliveext yiiliveext requested a review from yiisoft/reviewers Sep 1, 2020
yiiliveext added 5 commits Sep 1, 2020
@yiiliveext
Cleanup
Loading status checks…
edbf0de
@yiiliveext
Make methods set and setMultiple private
Loading status checks…
80a4e2f
@yiiliveext
Fix root container type
Loading status checks…
ee1cc2e
@yiiliveext
REmove Support
Loading status checks…
f5d09d3
@yiiliveext
Chage provider resolving to factory
Loading status checks…
d6d5f77
@samdark
samdark requested changes Sep 3, 2020
View changes
src/AbstractContainerConfigurator.php
@@ -8,16 +8,6 @@

abstract class AbstractContainerConfigurator

This comment has been minimized.

Sign in to view
@samdark

samdark Sep 3, 2020 Member

Seems it's near to empty now or at least name doesn't reflect the purpose anyway.
src/Container.php Outdated
@@ -267,7 +277,8 @@ private function addProvider($providerDefinition): void
*/
private function buildProvider($providerDefinition): ServiceProviderInterface
{
$provider = Normalizer::normalize($providerDefinition)->resolve($this);

$provider = $this->factory->create($providerDefinition);

This comment has been minimized.

Sign in to view
@samdark

samdark Sep 3, 2020 Member

Why do we need to create an object right away? Can't we set the config as is?

This comment has been minimized.

Sign in to view
@yiiliveext

yiiliveext Sep 4, 2020 Author Contributor

Why do we need to create an object right away? Can't we set the config as is?

We need to set params in the provider constructor.

This comment has been minimized.

Sign in to view
@yiiliveext

yiiliveext Sep 4, 2020 Author Contributor

https://github.com/yiisoft/yii-event/blob/master/src/EventDispatcherProvider.php#L24

This comment has been minimized.

Sign in to view
@samdark

samdark Sep 4, 2020 Member

Discussed in private. Decided that @yiiliveext will adjust it so providers array is returning instances instead of definitions.
@samdark samdark mentioned this pull request Sep 4, 2020
Open
@rustamwin rustamwin requested a review from yiisoft/yii3 Sep 5, 2020
@yiiliveext
Dissallow providers by definition
Loading status checks…
fec8ac7
@yiiliveext yiiliveext force-pushed the yiiliveext:providers branch from b19a9a2 to fec8ac7 Sep 7, 2020
@samdark samdark mentioned this pull request Sep 8, 2020
Open
@samdark samdark linked an issue that may be closed by this pull request Sep 8, 2020
Data providers and the order of their registration #142
Open
@BoShurik
BoShurik reviewed Sep 8, 2020
View changes
src/Container.php
throw new InvalidConfigException(
'Service provider should be an instance of ' . ServiceProviderInterface::class
);
}

if ($provider instanceof DeferredServiceProviderInterface) {

This comment has been minimized.

Sign in to view
@BoShurik

BoShurik Sep 8, 2020 •
edited

Does DeferredServiceProviderInterface make sense? Looks like all providers are lazy now

This comment has been minimized.

Sign in to view
@samdark

samdark Sep 8, 2020 Member

@yiiliveext looks like another leftover.

This comment has been minimized.

Sign in to view
@yiiliveext

yiiliveext Sep 9, 2020 Author Contributor

DeferredProvider is for a heavyweight service initialization. For example, we should make a heavyweight API call and pass the received data to the service constructor. If we use this service only on a few routes, then why do we make the API call on each route? So, we use deferred providers to avoid this.

This comment has been minimized.

Sign in to view
@BoShurik

BoShurik Sep 9, 2020

@yiiliveext

return [
    HeavyService::class => static function (ContainerInterface $container) {
        $data = $container->get(Api::class)->heavyCall();
        
        return new HeavyService($data);
    },
];

Isn't this service will be initialized only when requested?

This comment has been minimized.

Sign in to view
@yiiliveext

yiiliveext Sep 9, 2020 Author Contributor

Isn't this service will be initialized only when requested?

Yes, this is another way, it is appropriate in a simple case, but we may have something like that

class Provider implements ServiceProviderInterface
{
     public function getDefinitions(): array
     {
           $rawData = file_get_contents('http:\\mysyte.com\something');
           $dataForFirstService = $this->calculateDataForFirstService($rawData); 
           $dataForSecondService = $this->calculateDataForSecondService($rawData); 
           $dataForThirdService = $this->calculateDataForThirdService($rawData); 

           return [
               FirstService::class => [
                   '__construct()' => [$dataForFirstService],
               ],
               SecondService::class => [
                   '__construct()' => [$dataForSecondService],
               ],
               ThirdService::class => [
                   '__construct()' => [$dataForThirdService],
               ], 
           ]; 
     }

     public function calculateDataForFirstService($data)
     {
          //calculation
     }

     public function calculateDataForSecondService($data)
     {
          //calculation
     }

     public function calculateDataForThirdService($data)
     {
          //calculation
     }     
}

This comment has been minimized.

Sign in to view
@BoShurik

BoShurik Sep 9, 2020

@yiiliveext make sense, but can be defined with factories:

return [
    Api::class => [],
    ApiInterface::class => static function (ContainerInterface $container) {
        // To cache requests
        return new CachedApiDecorator($container->get(Api::class));
    },
    ServiceFactory::class => [
        // Also request may be cached in factory
        '__construct' => [Reference::to(ApiInterface::class)],
    ],
    FirstService::class => static function (ContainerInterface $container) {
        return $container->get(ServiceFactory::class)->createFirst();
    },
    SecondService::class => static function (ContainerInterface $container) {
        return $container->get(ServiceFactory::class)->createSecond();
    },
    ThirdService::class => static function (ContainerInterface $container) {
        return $container->get(ServiceFactory::class)->createThird();
    },
];

Anyway, in advanced cases you probably need additional data from container to access API (tokens, request signer etc)

This comment has been minimized.

Sign in to view
@yiiliveext

yiiliveext Sep 9, 2020 Author Contributor

Anyway, in advanced cases you probably need additional data from container to access API (tokens, request signer etc)

Yes, I think it would be fine to add setContainer() method to DeferredProviderInterface.
@vjik
Copy link
Member

vjik commented Sep 9, 2020 •
edited

Attention

If use deffered providers, providers that do something other than the container configuration will stop working.
For example in widgets: https://github.com/yiisoft/widget/blob/master/src/WidgetFactoryProvider.php

Suggestion

Add support \Closure for providers.
This is can use for not yii-* packages with simple code in providers.

For example, config:

return [
    'yiisoft/csrf' => static function() {
    CsrfToken::initialize();
    return [];
  },
];
@viktorprogger
viktorprogger reviewed Sep 9, 2020
View changes
src/Container.php
*
* @throws InvalidConfigException
* @throws NotInstantiableException
* @see ServiceProviderInterface
* @see DeferredServiceProviderInterface
*/
private function addProvider($providerDefinition): void
private function addProvider($provider): void

This comment has been minimized.

Sign in to view
@viktorprogger

viktorprogger Sep 9, 2020

Suggested change
private function addProvider($provider): void
private function addProvider(ServiceProviderInterface $provider): void
@BoShurik BoShurik mentioned this pull request Sep 16, 2020
Open
@samdark
Copy link
Member

samdark commented Sep 21, 2020

  1. Need to try that on yii-demo and packages. @yiiliveext will do it.
  2. Need to decide if that's a good way to go overall.
@samdark samdark added the type:docs label Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samdark samdark

@BoShurik BoShurik

@viktorprogger viktorprogger

@yiisoft/reviewers Reviewers

Assignees
No one assigned
Labels
status:code review type:docs
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Data providers and the order of their registration
6 participants
@yiiliveext @vjik @samdark @BoShurik @viktorprogger @thenotsoft
You can’t perform that action at this time.