Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#55682 closed enhancement (fixed)

Allow block themes to be uploaded without index.php

Reported by: florianbrinkmann's profile FlorianBrinkmann Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.0 Priority: normal
Severity: normal Version: 6.0
Component: Themes Keywords: has-patch has-testing-info commit dev-reviewed has-unit-tests
Focuses: Cc:

Description (last modified by SergeyBiryukov)

In [52940] it was made possible to activate a block theme without an index.php, but that file is still a requirement when uploading a theme via the theme install screen (tested with WP 6 RC1). I think that should not be the case, when activating is possible without it.

To reproduce: Try to upload a ZIP containing a block theme without an index.php file-

Change History (32)

#1 @FlorianBrinkmann
2 years ago

  • Type changed from defect (bug) to enhancement

#2 @SergeyBiryukov
2 years ago

  • Component changed from General to Themes
  • Description modified (diff)
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 6.0

Hi there, welcome back to WordPress Trac! Thanks for the ticket.

Moving to the milestone for visibility, just in case this should be addressed for 6.0. Otherwise, I think this can be moved to 6.0.1 or 6.1.

This ticket was mentioned in PR #2692 on WordPress/wordpress-develop by peterwilsoncc.


2 years ago
#4

  • Keywords has-patch added; needs-patch removed

#5 @peterwilsoncc
2 years ago

I forgot to post on the ticket at the time, I've set up a pull request linked to this ticket:

  • themes are required to have index.php or templates/index.html
  • error message updated to use indicate either file is required
  • error uses an existing string in WP so it doesn't break the string freeze

This ticket was mentioned in Slack in #core by chaion07. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-test by peterwilsoncc. View the logs.


2 years ago

#8 @Boniu91
2 years ago

Test Report

Env

  • WordPress 6.0-beta3-53290-src and patch version
  • Theme: Twenty Twenty Two (edited), Twenty Twenty (edited)

Steps to test

  1. Upload block theme that:
  • doesn't have index.php nor templates/index.html
  • doesn't have index.php but has templates/index.html
  • has index.php but doesn't have templates/index.html
  • has index.php and templates/index.html
  1. Upload non block theme that:
  • doesn't have index.php nor templates/index.html
  • doesn't have index.php but has templates/index.html
  • has index.php but doesn't have templates/index.html
  • has index.php and templates/index.html

Is it possible to upload a theme (before|after the patch)?

Block theme:

  • ❌|❌
  • ❌|✅
  • ✅|✅
  • ✅|✅

Non block theme:

  • ❌|❌
  • ❌|✅
  • ✅|✅
  • ✅|✅

Question

Should it be allowed to upload non block theme that doesn't have index.php but has templates/index.html?

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


2 years ago

#10 @costdev
2 years ago

  • Keywords has-testing-info added

Test Report

Environment

  • Server: Apache (Linux)
  • WordPress: 6.1-alpha-53344-src
  • Browser: Chrome 101.0.4951.54
  • OS: Windows 10
  • Theme: Twenty Twenty-Two
  • Plugins: None activated.

Steps

  1. Create four theme folders:
    • Just style.css.
    • style.css, index.php and templates/index.html.
    • style.css and index.php.
    • style.css and templates/index.html.
  2. Zip each folder into its own .zip file.
  3. Navigate to Appearance > Themes > Add New > Upload Theme.
  4. Attempt to upload each theme.
  5. Delete the uploaded themes.
  6. Apply PR 2692.
  7. Repeat steps 3-4.

Results

  1. Before applying PR 2692:
    • Just style.css. ❌
    • style.css, index.php and templates/index.html. ✅
    • style.css and index.php. ✅
    • style.css and templates/index.html. ❌
  2. After applying PR 2692:
    • Just style.css. ❌
    • style.css, index.php and templates/index.html. ✅
    • style.css and index.php. ✅
    • style.css and templates/index.html. ✅

Notes/Queries

  1. The PR appears to work as expected.
  2. As a block theme may instead use block-templates/index.html for BC reasons ref, should the PR also account for this file?

#11 @costdev
2 years ago

  • Keywords dev-feedback added

#12 follow-up: @hellofromTonya
2 years ago

as noted in the PR, PR 2692 changed the behavior for classic themes.

  • Before this PR, a classic theme without a index.php file could not be uploaded.
  • After this PR, it can be if it has a templates/index.html file.

Is this problematic?

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


2 years ago

#14 in reply to: ↑ 12 @aristath
2 years ago

Replying to hellofromTonya:

as noted in the PR, PR 2692 changed the behavior for classic themes.

  • Before this PR, a classic theme without a index.php file could not be uploaded.
  • After this PR, it can be if it has a templates/index.html file.

Is this problematic?

No, that's OK and not a problem. If the theme has a templates/index.html file, it would override the index.php file even if it existed.
It's possible to have both PHP and HTML templates in the same theme, but HTML templates get a higher priority than PHP templates, so an index.php file doesn't make any sense when the HTML file exists 👍

This ticket was mentioned in Slack in #core by costdev. View the logs.


2 years ago

#16 @costdev
2 years ago

PR 2692 changed the behavior for classic themes.
Before this PR, a classic theme without a index.php file could not be uploaded.
After this PR, it can be if it has a templates/index.html file.

I agree with Ari that this isn't problematic, as it doesn't change the behaviour of a classic theme.

Classic themes and block themes can be defined in several ways. However, here are definitions that reflect the current behaviour of WP_Theme::is_block_theme() source:

Block theme:
A theme that has an index.html file in the templates or block-templates directory.

Classic theme:
A theme that does not have an index.html file in the templates or block-templates directory.

Since the theme in question has a templates/index.html file, it is not a classic theme, so classic theme behaviour has not been changed. As Ari points out, in the event that it has both index.php and templates/index.html files, the templates/index.html file takes precedence.

Last edited 2 years ago by costdev (previous) (diff)

#17 @peterwilsoncc
2 years ago

As a block theme may instead use block-templates/index.html for BC reasons ref, should the PR also account for this file?

Yes, it should.

I think this is a wider bug, neither of the following check for the legacy folder configuration:

  • validate_current_theme()
  • WP_Theme::__construct

@aristath or @poena, could I get a logic check on this, would this be problematic in any way?

#18 follow-up: @poena
2 years ago

It depends on if the intention is to keep the backwards compatibility for the folder names forever or if we expect theme authors to update the themes since they were experimental?

#19 @poena
2 years ago

19 of the 70+ themes in the theme directory still use the block-templates folder, so that's not great.
But they are also mostly Automattic themes. Pinging @jffng @jeffikus

#20 in reply to: ↑ 18 @peterwilsoncc
2 years ago

Replying to poena:

It depends on if the intention is to keep the backwards compatibility for the folder names forever or if we expect theme authors to update the themes since they were experimental?

The goal is to support backward compatibility when possible once a feature makes it in to core. Given there is no technical limitation I will update my pull request.

peterwilsoncc commented on PR #2692:


2 years ago
#21

In https://github.com/WordPress/wordpress-develop/pull/2692/commits/c408ff64861900cc92a31c79ec9f34e557248905 I've added support back support for the deprecated paths in each of the locations that check for block themes.

I'll add in some tests to ensure deprecated block themes are not broken.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-editor by peterwilsoncc. View the logs.


2 years ago

gziolo commented on PR #2692:


2 years ago
#24

I left some comments about this patch on WordPress Slack (link requires registration at https://make.wordpress.org/chat):

https://wordpress.slack.com/archives/C037DD0CLHY/p1652748680139119

and re-sharing here for visibility. I can confirm that in other places we use both folder names for detecting whether we have a block theme:

https://github.com/WordPress/wordpress-develop/blob/3dcdd4b799de1442fa76f3b71b84485362fce83a/src/wp-includes/class-wp-theme.php#L1484-L1496

so it feels like we should be doing the same checks in all other places. @mcsf was so kind to also look at this problem and we both arrived at the same conclusions.

peterwilsoncc commented on PR #2692:


2 years ago
#25

Thanks for the help, folks, I think this has caught all the areas that were missing the check for the legacy folder structure.

Riad mentioned in Slack the deprecated paths are handled by the function above too. Based on some quick testing that seems to be happening in the FSE interface already.

I'm sure I could add more tests but if I can get some approvals, I think this is good to go in as is.

#26 @gziolo
2 years ago

  • Keywords commit added

I'm approving this patch based on my previous comment and all the code checks I performed. I also went through all test reports which confirm that everything works as intended. I also saw that @SergeyBiryukov approved PR on GitHub. Thank you Peter for bringing full backward compatibility!

This ticket was mentioned in Slack in #core by costdev. View the logs.


2 years ago

#28 @audrasjb
2 years ago

  • Keywords dev-reviewed added; dev-feedback removed

After a code logic review and a quick test using a custom theme, I think we're good to go.
Adding dev-reviewed workflow keyword.

#29 @peterwilsoncc
2 years ago

This was committed to trunk in [53416] but I used the incorrect ticket reference.

In [53416] :

Themes: Accept valid block themes.

Updates theme validation to accept block themes. This replaces the requirement for an index.php with a requirement for either an index.php, /templates/index.html or the deprecated /block-templates/index.html.

Validation is updated for theme uploads, within WP_Theme::__construct and validate_current_theme().

A block theme using the deprecated file structure is now included in the unit tests.

Props peterwilsoncc, sergeybiryukov, hellofromtonya, costdev, azaozz, gziolo, FlorianBrinkmann, Boniu91, aristath, poena, audrasjb.

#30 @peterwilsoncc
2 years ago

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

In 53417:

Themes: Accept valid block themes.

Updates theme validation to accept block themes. This replaces the requirement for an index.php with a requirement for either an index.php, /templates/index.html or the deprecated /block-templates/index.html.

Validation is updated for theme uploads, within WP_Theme::__construct and validate_current_theme().

A block theme using the deprecated file structure is now included in the unit tests.

Props peterwilsoncc, sergeybiryukov, hellofromtonya, costdev, azaozz, gziolo, FlorianBrinkmann, Boniu91, aristath, poena, audrasjb.
Merges [53416] to the 6.0 branch.
Fixes #55682.

This ticket was mentioned in PR #2733 on WordPress/wordpress-develop by peterwilsoncc.


2 years ago
#32

  • Keywords has-unit-tests added
Note: See TracTickets for help on using tickets.