Make Team permission handler service lazy

[mxr576]

Fixes #140

I do not think the reported problem would require an extra test in this module. I mean we should not verify whether the reported by @Jaesin exist or not after this fixed gets merged. The service remains lazy loaded.

The latest Travis build: https://travis-ci.org/mxr576/apigee-devportal-drupal/builds/499723141

[Jaesin]

This seems like a complicated workaround. What other goals are you trying to achieve with this PR? Whould the simple change as it was in https://github.com/apigee/apigee-edge-drupal/issues/140 solve this problem? Is there an Interface somewhere that explains these self loading lazy loading services?

[mxr576]

This is not a "complicated workaround". It is an official way to postpone initiation of "heavy" services. This method is inherited from Symfony and proxy classes are auto-generated with a PHP script provided by the Drupal core.

https://symfony.com/doc/3.4/service_container/lazy_services.html
https://www.webomelette.com/lazy-loaded-services-drupal-8

This can be a solution but not in all cases. Have you tested this PR? Could you send me a link to a commit where you have troubles because of this?

I am asking around in the Drupal community whether retrieving storage in a service's constructor is actually an undocumented bad practice because I keep seeing this in other contrib modules as well. Also, just imagine that you need storage in n+1 different methods in your service class, would you always write $this->entityTypeManager->getStorage() to retrieve it? I would not if it is not really necessary.

(Attached an updated test to #140)

[dawehner]

I had some quick conversation with @Jaesin about this pull request.
When we build lazy services into Drupal core we introduced it to get some better performance for example for the page cache. For page cache you don't want to load the cron service for example.

Much like lazy services also work for working around circular dependencies, I don't think this is the best way, given that you need to re-generate the proxies when we would want to change the interface.

There are some alternatives:

• Throw an event on invalidate and let the other service subscribe to it.
• Use setter injection. This effectively makes dependencies optional. I created an alternative PR for that: Inject product access manager via setter injection #144

[mxr576]

@dawehner Thanks for taking a look at this PR also.
I am also aware of the PROs and CONs of lazy services and I think they have more PROs than CONs. Because both modules define more than 10 services, maybe we should consider later to transform more of our services to lazy. As you mentioned, lazy services can improve performance and reduce memory usage.
I do not expect that our service interfaces are going to change too much or too frequently in the future - or if they do - as you also mentioned, these proxy classes can be re-generated with the php core/scripts/generate-proxy-class.php again. (I also see one problem with the generate-proxy-class.php script, it does not support PHP 7 return types, so those needs to be added manually. Haven't checked whether there is a Drupal core issue about this.)

I also asked around in the Drupal community whether saving an instance of an entity storage to a property in a service's constructor is bad practice. TL;DR; I did not get a clear answer.

Sascha Grossenbacher (berdir) wrote on Slack:

I would recommend to not inject entity storages into properties, especially if they are not always required and it is also possible that they get off sync and might no longer receive cache clears, that's however rare outside of tests

Okay, but what if I need one specific storage, let's say 3 different places in my service class. Should I dare to save it to a property in the constructor or let's retrieve it every time from the entity type manager?

I would retrieve it from the entity type manager, if you need it multiple times in the same method then use a local variable. that's what I do 🙂

There is no clear answer within these lines, there is no justification why you should use one over the other. Only a custom that can vary per developer (lsd. Commerce guys also use the same technique). Issus that he mentioned which could occur in testing can be eliminated by using $this->container() and \Drupal::service() properly in testing based on my experiences. For example, do not expect that changes made on the UI with Mink are going to change the state of a service retrieved by $this->container in a test. For example, the changes made on the UI did invalidate the entity storage's static cache, just not that instance of the storage (precisely the entity.memory_cache service) one that we expected here: a1674ad

Consequently, creating lazy services as a fix to this problem still seems a better option to me.

[Jaesin]

We have a workaround for #140 by not injecting the service there is no hurry for this.

I will test it and we can talk more about it after the current sprint.

[cnovak]

This has been fixed by #144