Implement dependency resolution for services #8513
Conversation
swissspidy
added
Type: Enhancement
|
Size Change: 0 B Total Size: 4.05 MB ℹ️ View Unchanged
|
|
I am not sure about adding this, until we need it. Is there any example service we could implement this on? How about |
|
Silly question, how does this different from conditional service? |
This is for services that depend on another service, not services that depend on some outside condition (e.g. For example if service A uses While we might not have obvious candidates for this in our code base yet (at least at first glance), it seems like a reasonable enhancement to facilitate further restructuring of the code base to use more services. One opportunity there might be moving some of our functions in Just some ideas though, haven't looked at it closely. |
|
Sooo... this PR seems to uncover some flaws in our code. For example, But with this PR it looks like public static function get_requirements(): array {
return [
'settings',
];
}The timeout goes away if @pierlon do you perhaps have any thoughts or pointers? |
|
@swissspidy It seems you've uncovered a bug. With the example you stated above the I'll see how best this can be resolved and test the changes over at ampproject/amp-wp#6484. |
The main purpose of this is to make the service registration independent of loading order. Relying on a specific order of things is always a latent bug, basically, so explicitly stating the requirements and adapting the loading order based on that is the only way to reliably deal with changing code and potentially even extending the service hierarchy. |
|
Regarding the
I'll adapt the PR in the AMP plugin to take delaying actions into account, and then we can adapt this one to ensure the problem is resolved. |
@schlessera |
|
@swissspidy Yes, but |
|
Oh, right, that doesn't make much sense 🤔 It's registered at init w/ prio 5 only to then call |
|
Yep. The method |
|
@swissspidy Is the main issue you've identified the first time around fixed now with the changes I made? |
|
I believe so, yes! Thanks a lot! Next I'll go through our services to properly identify & add dependencies where needed. |
Nope, the So far When an instance is injected via the constructor, This is in line with how it works in the AMP plugin: https://github.com/ampproject/amp-wp/blob/543aa27087c6fe1847746eda84facea44a5a90bf/src/Admin/ValidationCounts.php#L42-L61 |
tests/phpunit/integration/tests/REST_API/Stories_Media_Controller.php
Outdated
Show resolved
Hide resolved
tests/phpunit/integration/tests/REST_API/Stories_Autosaves_Controller.php
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
|
There are still a couple of instances of |
As taken from ampproject/amp-wp#6484
Are there any dependencies we'd need to add already?