Backport block template resolution algorithm unit tests

[ockham]

It seems like we missed backporting these as part of #1796. Doing it now since they will give us better confidence when we need to modify the algorithm.

Prompted by this discussion.

• Tests are carried over from https://github.com/WordPress/gutenberg/blob/6ba7c24e590e0deda4e3524f5343fe8293b8a42a/phpunit/class-template-loader-test.php
• Text fixtures are carried over from https://github.com/WordPress/gutenberg/tree/6ba7c24e590e0deda4e3524f5343fe8293b8a42a/phpunit/fixtures/themes

I opted to integrated test fixture files into existing block theme test fixtures in wordpress-develop to keep the number of fixtures and files down. AFAICS, they've been used for testing theme.json related features so far, so I don't think there should be any collisions.

Trac ticket: https://core.trac.wordpress.org/ticket/54478

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[ockham]

Can't remove index.php from block-theme yet because of this check:

wordpress-develop/src/wp-includes/class-wp-theme.php

Lines 341 to 348 in 3442f77

	if ( ! file_exists( $this->theme_root . '/' . $this->stylesheet . '/index.php' ) ) {
	$error_message = sprintf(
	/* translators: 1: index.php, 2: Documentation URL, 3: style.css */
	__( 'Template is missing. Standalone themes need to have a %1$s template file. <a href="%2$s">Child themes</a> need to have a Template header in the %3$s stylesheet.' ),
	'<code>index.php</code>',
	__( 'https://developer.wordpress.org/themes/advanced-topics/child-themes/' ),
	'<code>style.css</code>'
	);

Are we going to relax this to accept block-templates/index.html instead (and soon, templates/index.html)?

[ockham]

3 unit test failures to go -- 1 from the newly backported tests, 2 from what seems to be a side effect of the theme switching (?):

There were 3 failures:

1) Block_Template_Test::test_child_theme_php_template_takes_precedence_over_equally_specific_parent_theme_block_template
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'/var/www/tests/phpunit/includes/../data/themedir1/block-theme-child/page-home.php'
+'/var/www/src/wp-includes/template-canvas.php'

/var/www/tests/phpunit/tests/block-template.php:120

2) Tests_General_Template::test_has_custom_logo
Failed asserting that true is false.

/var/www/tests/phpunit/tests/general/template.php:270

3) Tests_General_Template::test_get_custom_logo
Failed asserting that a string is empty.

/var/www/tests/phpunit/tests/general/template.php:315

[youknowriad]

For the remaining failure test_child_theme_php_template_takes_precedence_over_equally_specific_parent_theme_block_template, I followed the logic of the algorithm and it's behaving as expected, basically I don't see any logic that compares the specificity of the php template with the block template that comes from the parent theme so I don't really know why this test passes in Gutenberg.

[ockham]

For the remaining failure test_child_theme_php_template_takes_precedence_over_equally_specific_parent_theme_block_template, I followed the logic of the algorithm and it's behaving as expected, basically I don't see any logic that compares the specificity of the php template with the block template that comes from the parent theme so I don't really know why this test passes in Gutenberg.

That's a bit concerning though. The test has a note that specifically points at a regression fix: https://github.com/WordPress/wordpress-develop/pull/1920/files#diff-12e215aaa7e67637d3de9eae20536f56ac9248727dcdca0b6f87c09e3f256166R101-R108

The reasoning of that PR makes sense to me; and I think I thought to preserve that functionality when refactoring the template resolution algorithm, and added the test to cover against further regressions.

I'll look a bit more into this.

[ockham]

I think I've tracked it down: You're right that the algorithm is working as expected here.

In Gutenberg OTOH, something isn't quite working with test theme directory registration, and thus, we're not detecting that test-theme-child is a child theme, and test-theme is its parent. (This can be validated by echoing get_template() somewhere in the unit test file: It should return the parent theme -- i.e. test-theme -- rather than the child -- but in GB, it returns the child theme instead.)

This will require a number of different fixes:

• In the unit test in GB, we need to make sure the child theme is detected as such, and its parent is found. This will cause test_child_theme_php_template_takes_precedence_over_equally_specific_parent_theme_block_template to fail in GB.
• We need to fix the template resolution algorithm in both GB and WP Core to make test_child_theme_php_template_takes_precedence_over_equally_specific_parent_theme_block_template pass in both.

[youknowriad]

@ockham I think to avoid delaying the switch to "templates" and "parts", we should comment this test for now while we work on it separately. In parallel, I can start the "folders" patch in Core that way it will unblock the Gutenberg PR.

[youknowriad]

@ockham I've created the folders PR in #1932

[carolinan]

Can't remove index.php from block-theme yet because of this check:

wordpress-develop/src/wp-includes/class-wp-theme.php

Lines 341 to 348 in 3442f77

	if ( ! file_exists( $this->theme_root . '/' . $this->stylesheet . '/index.php' ) ) {
	$error_message = sprintf(
	/* translators: 1: index.php, 2: Documentation URL, 3: style.css */
	__( 'Template is missing. Standalone themes need to have a %1$s template file. <a href="%2$s">Child themes</a> need to have a Template header in the %3$s stylesheet.' ),
	'<code>index.php</code>',
	__( 'https://developer.wordpress.org/themes/advanced-topics/child-themes/' ),
	'<code>style.css</code>'
	);

Are we going to relax this to accept block-templates/index.html instead (and soon, templates/index.html)?

This was punted to 6.0:
https://core.trac.wordpress.org/ticket/54272

[ockham]

Are we going to relax this to accept block-templates/index.html instead (and soon, templates/index.html)?

This was punted to 6.0: https://core.trac.wordpress.org/ticket/54272

Thanks for clarifying @carolinan!

[ockham]

@ockham I think to avoid delaying the switch to "templates" and "parts", we should comment this test for now while we work on it separately. In parallel, I can start the "folders" patch in Core that way it will unblock the Gutenberg PR.

Okay, I'll do that now 👍

[ockham]

@ockham I think to avoid delaying the switch to "templates" and "parts", we should comment this test for now while we work on it separately. In parallel, I can start the "folders" patch in Core that way it will unblock the Gutenberg PR.

Okay, I'll do that now 👍

Done: 6d63d8f. Should pass now & be ready for review.

[ockham]

Multisite tests are failing. I'll try a rebase for starters.

[ockham]

Multisite tests are still failing. Specifically, tests affected are in the sidebars and widgets controller test suites (rest-sidebars-controller.php and rest-widgets-controller.php), and they're mostly getting the wrong REST API responses (e.g. some data where an empty response was expected, or extraneous fields).

I've double-checked that this is not affecting trunk, or other PRs. It's also not affecting any of the single-site tests. The only thing in this PR that I think could've caused this is the theme switching (to our block test theme(s), and back to whatever theme was active before); but apparently there's some cross-site leakage of data. Or so.

cc/ @adamziel @TimothyBJacobs @desrosj in case the above seems familiar to any of you, since AFAICS, y'all have worked on the sidebar and widget controllers.

[ockham]

Trying to revert the theme switch, and restoring the delete_option( 'site_logo' ).

[ockham]

No, multisite tests are still failing 😕 (It's the same ones as before, plus a bunch more in themeMods.php.

[ockham]

The next thing I'll try will be to copy the theme switching strategy from tests/phpunit/tests/theme/wpThemeJsonResolver.php, since that's also using block-theme and block-theme-child, and multisite tests obviously seem to be passing there:

wordpress-develop/tests/phpunit/tests/theme/wpThemeJsonResolver.php

Lines 15 to 37 in e7c8bf8

	public function set_up() {
	parent::set_up();
	$this->theme_root = realpath( DIR_TESTDATA . '/themedir1' );
	$this->orig_theme_dir = $GLOBALS['wp_theme_directories'];
	// /themes is necessary as theme.php functions assume /themes is the root if there is only one root.
	$GLOBALS['wp_theme_directories'] = array( WP_CONTENT_DIR . '/themes', $this->theme_root );
	add_filter( 'theme_root', array( $this, 'filter_set_theme_root' ) );
	add_filter( 'stylesheet_root', array( $this, 'filter_set_theme_root' ) );
	add_filter( 'template_root', array( $this, 'filter_set_theme_root' ) );
	// Clear caches.
	wp_clean_themes_cache();
	unset( $GLOBALS['wp_themes'] );
	}
	public function tear_down() {
	$GLOBALS['wp_theme_directories'] = $this->orig_theme_dir;
	wp_clean_themes_cache();
	unset( $GLOBALS['wp_themes'] );
	parent::tear_down();
	}

[ockham]

Having another look at the test matrix, not all multisite tests are failing. The ones that are passing are:

• PHPUnit Tests / 5.6 multisite on ubuntu-latest
• PHPUnit Tests / 8.1 multisite on ubuntu-latest
• PHPUnit Tests / 5.6 multisite slow tests on ubuntu-latest

[ockham]

Uh, but the 8.1 jobs have continue-on-error. (Closer inspections shows that unit tests are failing there.)

The 5.6 ones do seem to pass, tho.

[ockham]

The next thing I'll try will be to copy the theme switching strategy from tests/phpunit/tests/theme/wpThemeJsonResolver.php

This got a bit messy upon first attempt (more failures in other tests).

New/alternative hypothesis: Since we've been doing the theme switching in the static wpSetUpBeforeClass (and wpTearDownAfterClass) -- maybe that's happening too early (or late, respectively) -- i.e. before multisite setup is complete? I'll try moving them into the non-static set_up and tear_down.

[ockham]

Hmm, looking at the test failures, it seems like the data might be coming from tests/phpunit/tests/widgets.php. I'll look into that file...

[ockham]

This could be relevant: #1331

[ockham]

Seems like the sidebar/widgets data we're seeing if written by wp_install_defaults. In a multi-site context, the latter is called by wp_initialize_site.

[ockham]

@jorgefilipecosta gave me the decisive tip: 5087aeb 😄

[youknowriad]

Great, going to commit this.

[youknowriad]

committed in https://core.trac.wordpress.org/changeset/52246

[ockham]

I've filed an issue for the failing unit test that we had to disable in Core Trac: https://core.trac.wordpress.org/ticket/54515