Implement dependency resolution for services #6484

pierlon · 2021-07-25T00:24:04Z

Summary

This PR implements a simple way for services to resolve their dependent services so that it is guaranteed that those services exist before retrieving them from the service container.

In order to accomplish this, the AmpProject\AmpWP\Infrastructure\ServiceBasedPlugin::register_services() method has been modified to check if a service implements the AmpProject\AmpWP\Infrastructure\HasRequirements interface, and if it does, it then retrieves the list of service identifiers it requires in order for the service to be registered. If any of the required service IDs do not exist in the service container, the registration of the service will be delayed (moved to the end of the to-be-registered services list) to allow for other services to be registered in the mean time.

Testing

To test that this fixes #6459:

Add the following plugin to replicate the bug:

<?php
/**
 * Plugin Name: Activate AMP Plugin
 */

add_action( 'admin_head', function () {
    activate_plugin( 'amp/amp.php' );
} );

Deactivate the AMP plugin
Activate the plugin above and it should successfully activate the AMP plugin without any errors.

Checklist

My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

github-actions · 2021-07-27T20:44:50Z

Plugin builds for 953a039 are ready 🛎️!

Download development build
Download production build

westonruter

Looks great to me, but I defer to @schlessera for his final word.

westonruter · 2021-07-28T00:08:59Z

codecov.yml

-codecov:
-  notify:
-    # Wait for Jest & PHPUnit $ Behat reports before notifying
-    after_n_builds: 3
-


Why is this no longer needed?

Codecov waits for all 3 coverage reports before posting the summary for the PR; problem is that we are now skipping some PR checks which means it's no longer guaranteed 3 coverage reports will be uploaded, so Codecov will be waiting for reports that it will potentially never receive.

westonruter · 2021-07-28T00:10:19Z

src/Admin/PairedBrowsing.php

+	/**
+	 * Get the list of service IDs required for this service to be registered.
+	 *
+	 * @return array<string> List of required services.


This means the same thing, but any reason for/against?

Suggested change

* @return array<string> List of required services.

* @return string[] List of required services.

Either works, just a little copypasta 😄.

I'll commit the suggestion since it's what's more prominent in the codebase.

westonruter · 2021-07-28T00:13:34Z

src/Infrastructure/ServiceBasedPlugin.php

+				/*
+				 * Move the service to the end of the array.
+				 *
+				 * Note: Unsetting the key advances the internal array pointer to the next array item, so
+				 * the current array item will have to be temporarily stored so that it can be re-added later.
+				 */
+				$delayed_service = [ key( $services ) => current( $services ) ];
+				unset( $services[ key( $services ) ] );
+				$services[ key( $delayed_service ) ] = current( $delayed_service );


Co-authored-by: Weston Ruter <[email protected]>

swissspidy · 2021-07-29T08:41:23Z

Any objections to copy this for the Web Stories plugin? :-)

pierlon · 2021-07-29T14:32:17Z

Any objections to copy this for the Web Stories plugin? :-)

None at all 😄.

src/AmpWpPlugin.php

…ssing requirements

schlessera · 2021-08-02T16:22:37Z

@pierlon I made changes to this to add a more robust mechanism to deal with the load order. Please review these changes and let me know your thoughts.

pierlon · 2021-08-04T21:50:03Z

@schlessera Thanks for making those changes. I've reviewed and tested the updates regarding delayed services and it works as expected 👍.

src/Infrastructure/ServiceBasedPlugin.php

Co-authored-by: Pierre Gordon <[email protected]> Co-authored-by: Alain Schlesser <[email protected]>

pierlon added 5 commits July 24, 2021 20:23

Implement dependency resolution for services

874b337

Update .phpstorm.meta.php

0ce8493

Fix delayed service not being re-inserted into the array

e3c45dc

Test registering a service with requirements

c80decd

Fix lint errors

dcc3c71

pierlon added this to the v2.1.4 milestone Jul 27, 2021

pierlon requested review from westonruter and schlessera July 27, 2021 20:38

pierlon marked this pull request as ready for review July 27, 2021 20:38

No longer wait on all coverage reports

357ae8d

westonruter approved these changes Jul 28, 2021

View reviewed changes

Use string[] param type

c06b300

Co-authored-by: Weston Ruter <[email protected]>

swissspidy reviewed Jul 29, 2021

View reviewed changes

src/AmpWpPlugin.php Outdated Show resolved Hide resolved

Remove obsolete comment

1f1feee

swissspidy mentioned this pull request Jul 30, 2021

Implement dependency resolution for services GoogleForCreators/web-stories-wp#8513

Merged

schlessera added 7 commits August 2, 2021 14:45

Add more robust mechanism for postponing service registration with mi…

400f8d6

…ssing requirements

Add delayed requirements test

7e1ee16

Fix PHPCS issues

bca199f

Fix broken tests

614f35b

Remove unused argument from callback

ab34525

Mock less of the plugin for requirements tests

14af92b

Fix test

9ef514e

schlessera requested review from westonruter and swissspidy August 2, 2021 16:21

swissspidy reviewed Aug 4, 2021

View reviewed changes

src/Infrastructure/ServiceBasedPlugin.php Outdated Show resolved Hide resolved

Remove invalid import

953a039

westonruter merged commit 543aa27 into develop Aug 4, 2021

westonruter deleted the fix/6459-service-dependency-resolution branch August 4, 2021 23:57

westonruter added a commit that referenced this pull request Aug 5, 2021

Implement dependency resolution for services (#6484)

8a018cb

Co-authored-by: Pierre Gordon <[email protected]> Co-authored-by: Alain Schlesser <[email protected]>

westonruter mentioned this pull request Aug 10, 2021

Plugin fails to activate using TGMPA #6459

Closed

pierlon mentioned this pull request Aug 11, 2021

Fix service registration delay causing next service to be skipped #6529

Merged

2 tasks

westonruter mentioned this pull request Dec 10, 2021

Add back-compat to old WP versions for new v2.2 features #6773

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement dependency resolution for services #6484

Implement dependency resolution for services #6484

pierlon commented Jul 25, 2021 •

edited

Loading

github-actions bot commented Jul 27, 2021 •

edited

Loading

westonruter left a comment

westonruter Jul 28, 2021

pierlon Jul 28, 2021

westonruter Jul 28, 2021

pierlon Jul 28, 2021

pierlon Jul 28, 2021 •

edited

Loading

westonruter Jul 28, 2021

swissspidy commented Jul 29, 2021

pierlon commented Jul 29, 2021

schlessera commented Aug 2, 2021

pierlon commented Aug 4, 2021

	* @return array<string> List of required services.
	* @return string[] List of required services.

Implement dependency resolution for services #6484

Implement dependency resolution for services #6484

Conversation

pierlon commented Jul 25, 2021 • edited Loading

Summary

Testing

Checklist

github-actions bot commented Jul 27, 2021 • edited Loading

westonruter left a comment

Choose a reason for hiding this comment

westonruter Jul 28, 2021

Choose a reason for hiding this comment

pierlon Jul 28, 2021

Choose a reason for hiding this comment

westonruter Jul 28, 2021

Choose a reason for hiding this comment

pierlon Jul 28, 2021

Choose a reason for hiding this comment

pierlon Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

westonruter Jul 28, 2021

Choose a reason for hiding this comment

swissspidy commented Jul 29, 2021

pierlon commented Jul 29, 2021

schlessera commented Aug 2, 2021

pierlon commented Aug 4, 2021

pierlon commented Jul 25, 2021 •

edited

Loading

github-actions bot commented Jul 27, 2021 •

edited

Loading

pierlon Jul 28, 2021 •

edited

Loading