Ensure services that are delayed and have requirements are scheduled correctly

[schlessera]

Summary

The code that was dealing with service requirements was missing logic for the scenario where the service has requirements AND it is Delayed itself. In that case, and if the delay of the service being checked is later than the delay of the requirements, that delay would not have been respected.

This PR changes up the logic to make it clearer when registration is scheduled.

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]

Plugin builds for 1e9f460 are ready 🛎️!

• Download development build
• Download production build

[schlessera]

@pierlon The test for the polyfills fails on WP 4.9.

From what I could find out, it looks like is_gutenberg_page() is not defined in that scenario, which therefore makes the registration of the Polyfills service bail early here: https://github.com/ampproject/amp-wp/blame/develop/src/Admin/Polyfills.php#L70-L71

Not sure why this test didn't fail before. Am I misunderstanding something here?

[westonruter]

From what I could find out, it looks like is_gutenberg_page() is not defined in that scenario, which therefore makes the registration of the Polyfills service bail early here: https://github.com/ampproject/amp-wp/blame/develop/src/Admin/Polyfills.php#L70-L71

Bail early? But the check is function_exists( 'is_gutenberg_page' ) && is_gutenberg_page() so if the function doesn't exist then it should not return.

It seems rather that is_gutenberg_page() exists and the function is returning true for some reason?

In WordPress 4.9 the version of Gutenberg installed is 4.9.0:

amp-wp/bin/ci/after-wp-install.sh

Lines 14 to 17 in ebb4008

	case "$WP_VERSION" in
	"4.9")
	gb_version="4.9.0"
	;;

This is being done: https://github.com/ampproject/amp-wp/pull/6548/checks?check_run_id=3368405984#step:16:6

The is_gutenberg_page() function is as follows: https://github.com/WordPress/gutenberg/blob/v4.9.0/gutenberg.php#L111-L146

[westonruter]

There we go. The issue appears to be that the test was calling register method:

amp-wp/src/Admin/Polyfills.php

Lines 58 to 76 in ebb4008

	/**
	* Runs on instantiation.
	*/
	public function register() {
	if ( function_exists( 'get_current_screen' ) ) {
	$screen = get_current_screen();
	if ( ! empty( $screen->is_block_editor ) ) {
	return;
	}
	}
	// Applicable to Gutenberg v5.5.0 and older.
	if ( function_exists( 'is_gutenberg_page' ) && is_gutenberg_page() ) {
	return;
	}
	$this->register_shimmed_scripts( wp_scripts() );
	$this->register_shimmed_styles( wp_styles() );
	}

This is calling wp_scripts() and wp_styles() which instantiates WP_Scripts and WP_Styles if the globals aren't already instantiated. Nevertheless, immediately after calling register the test was overriding the WP_Scripts and WP_Styles globals so that they were devoid of anything that the register method did with register_shimmed_scripts and register_shimmed_styles:

amp-wp/tests/php/src/Admin/PolyfillsTest.php

Lines 69 to 73 in ebb4008

	global $wp_scripts, $wp_styles;
	$this->instance->register();
	$wp_scripts = new WP_Scripts();
	$wp_styles = new WP_Styles();

It seems that this action is being triggered unnecessarily:

amp-wp/tests/php/src/Admin/PolyfillsTest.php

Lines 75 to 76 in ebb4008

	/** This action is documented in includes/class-amp-theme-support.php */
	do_action( 'amp_register_polyfills' );

Since the register method was manually invoked above. It doesn't seem this action results in register being called again.

[pierlon]

Thanks for looking into why that test was failing @westonruter 🙌. Great detective work.

[pierlon]

Thanks for identifying the bug @schlessera.

[bartoszgadomski]

QA passed

I did run smoke tests and didn't notice any issues.