WordPress.org

Make WordPress Core

Opened 2 weeks ago

Closed 9 days ago

Last modified 9 days ago

#54478 closed enhancement (fixed)

Backport block template resolution algorithm unit tests

Reported by: Bernhard Reiter Owned by: youknowriad
Milestone: Awaiting Review Priority: normal
Severity: normal Version: trunk
Component: Editor Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

It seems like we missed backporting these as part of https://github.com/WordPress/wordpress-develop/pull/1796.

Change History (28)

This ticket was mentioned in PR #1920 on WordPress/wordpress-develop by ockham.


2 weeks ago

  • Keywords has-patch has-unit-tests added

It seems like we missed backporting these as part of https://github.com/WordPress/wordpress-develop/pull/1796. Doing it now since they will give us better confidence when we need to modify the algorithm.

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

#2 @prbot
2 weeks ago

ockham commented on PR #1920:

Can't remove `index.php` from block-theme yet because of this check: https://github.com/WordPress/wordpress-develop/blob/3442f776f3f56eb76eec2c85f2d42941d9db9a93/src/wp-includes/class-wp-theme.php#L341-L348

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

#3 @prbot
2 weeks ago

ockham commented on PR #1920:

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

#4 @prbot
12 days ago

youknowriad commented on PR #1920:

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.

#5 @prbot
12 days ago

ockham commented on PR #1920:

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.

#6 @prbot
11 days ago

ockham commented on PR #1920:

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.

#7 @prbot
11 days ago

youknowriad commented on PR #1920:

@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.

#9 @prbot
11 days ago

carolinan commented on PR #1920:

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

https://github.com/WordPress/wordpress-develop/blob/3442f776f3f56eb76eec2c85f2d42941d9db9a93/src/wp-includes/class-wp-theme.php#L341-L348

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

#10 @prbot
11 days ago

ockham commented on PR #1920:

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!

#11 @prbot
11 days ago

ockham commented on PR #1920:

@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 👍

#12 @prbot
11 days ago

ockham commented on PR #1920:

@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.

#13 @prbot
11 days ago

ockham commented on PR #1920:

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

#14 @prbot
11 days ago

ockham commented on PR #1920:

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.

#15 @prbot
10 days ago

ockham commented on PR #1920:

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

#16 @prbot
10 days ago

ockham commented on PR #1920:

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

#17 @prbot
10 days ago

ockham commented on PR #1920:

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:

https://github.com/WordPress/wordpress-develop/blob/e7c8bf8fa70680a6723cd182bed6595732f3600c/tests/phpunit/tests/theme/wpThemeJsonResolver.php#L15-L37

#18 @prbot
10 days ago

ockham commented on PR #1920:

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

#19 @prbot
10 days ago

ockham commented on PR #1920:

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.

#20 @prbot
10 days ago

ockham commented on PR #1920:

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.

#21 @prbot
10 days ago

ockham commented on PR #1920:

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...

#22 @prbot
10 days ago

ockham commented on PR #1920:

This could be relevant: #1331

#23 @prbot
10 days ago

ockham commented on PR #1920:

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`.

#24 @prbot
9 days ago

ockham commented on PR #1920:

@jorgefilipecosta gave me the decisive tip: 5087aeb 😄

#25 @prbot
9 days ago

youknowriad commented on PR #1920:

Great, going to commit this.

#26 @youknowriad
9 days ago

  • Owner set to youknowriad
  • Resolution set to fixed
  • Status changed from new to closed

In 52246:

Themes: Add block template resolution algorithm unit tests.

Backports the unit tests for block templates
and hybrid themes template resolution from the Gutenberg plugin.

Props bernhard-reiter, jorgefilipecosta.
Fixes #54478.

#28 @prbot
9 days ago

ockham commented on PR #1920:

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

Note: See TracTickets for help on using tickets.