Making WordPress.org

Opened 6 years ago

Closed 6 years ago

#3718 closed task (blessed) (fixed)

Theme Directory: Allow minimum WP and PHP version requirement

Reported by: flixos90's profile flixos90 Owned by: dd32's profile dd32
Milestone: Priority: normal
Component: Theme Directory Keywords: has-patch servehappy
Cc:

Description

As a follow up to https://core.trac.wordpress.org/ticket/40934 and #2952, this ticket aims to add headers for version requirements to WordPress themes.

Themes do not always support all WordPress versions (for example Twenty Seventeen requires WP 4.7), and, while that is not a common practice seen so far, there are also themes that require a higher PHP version than 5.2. Core should highlight such incompatibilities, but as a prerequisite we need to allow theme authors to indicate required versions.

Attachments (1)

3718.diff (52.1 KB) - added by flixos90 6 years ago.

Download all attachments as: .zip

Change History (35)

@flixos90
6 years ago

#1 @flixos90
6 years ago

3718.diff adds support for a Requires header (for required WP version) and a Requires PHP header (for required PHP version). While we could go with Requires WP, I thought it would make sense to align the names with the names of the plugin headers, so people can remember them easily for both plugins and themes.

As far as I can tell, my patch covers all areas necessary to have full support for those headers, but since I haven't contributed to the theme directory meta part before, I might have missed something else.

Also, be aware that this patch would require https://core.trac.wordpress.org/ticket/44592 to be merged into core first, in order to have support for the two headers in the WP_Theme class, which is also used by the theme directory plugin.

#2 @flixos90
6 years ago

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in Slack in #core-php by flixos90. View the logs.


6 years ago

#4 @dd32
6 years ago

FWIW I think this is part of the bigger discussion around Theme Meta. Unlike the Plugin Directory, Themes have no way to define extra meta-data like this, or even to have a theme description that isn't one-long-line-that's-unreadable-because-it-just-keeps-on-going-on-and-on.

The patch as-is adds it to the Theme style.css headers, which seems like the wrong place for it - but the only place we currently have.
All new themes in the repo need to have a readme.txt - but the theme directory doesn't parse anything from it - #215

To me, it feels like the implementation should match what plugins have, where it's included in the readme (which is where all the Metadata ultimately comes from).

I have a feeling that borrowing the plugin directory readme parser and only supporting headers / description wouldn't require too much effort here and would provide for a much better platform for future changes (such as supporting a minimum WP version requirement too, etc)

This ticket was mentioned in Slack in #core-php by schlessera. View the logs.


6 years ago

This ticket was mentioned in Slack in #meta by tellyworth. View the logs.


6 years ago

#7 follow-up: @afragen
6 years ago

I agree that having new themes require a readme.txt file is the path forward. Parity with the plugin directory makes the most sense as does using the same parser.

For existing themes that don’t have a readme.txt file couldn’t we create a parsed data array from the info we do have and use that if no actual reader.txt file exists?

#8 in reply to: ↑ 7 ; follow-up: @dd32
6 years ago

Replying to afragen:

For existing themes that don’t have a readme.txt file couldn’t we create a parsed data array from the info we do have and use that if no actual reader.txt file exists?

That's what would happen, yes.

I'm simply suggesting that skipping this header entirely and pulling at least that info from the readme would be a better long-term solution

#9 in reply to: ↑ 8 @afragen
6 years ago

Replying to dd32:

I'm simply suggesting that skipping this header entirely and pulling at least that info from the readme would be a better long-term solution

Agreed. I don’t think the style.css file needs to be parsed at all. I would suggest the following process for getting the information.

  1. Parse an existing readme.
  2. Use the API to get existing data, eg. https://api.wordpress.org/themes/info/1.2/?action=theme_information&request[slug]=twentyten

We might be referring to the same process by different names. I’m not sure.

Last edited 6 years ago by afragen (previous) (diff)

This ticket was mentioned in Slack in #meta by sergey. View the logs.


6 years ago

#11 @SergeyBiryukov
6 years ago

Last edited 6 years ago by SergeyBiryukov (previous) (diff)

This ticket was mentioned in Slack in #themereview by joyously. View the logs.


6 years ago

This ticket was mentioned in Slack in #themereview by joyously. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-php by sergey. View the logs.


6 years ago

This ticket was mentioned in Slack in #design by boemedia. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-php by afragen. View the logs.


6 years ago

This ticket was mentioned in Slack in #themereview by andreiglingeanu. View the logs.


6 years ago

#18 @afragen
6 years ago

If we are hoping to achieve a similar effect for blocking theme installation that doesn't meet the developer specified compatibility requirements as listed in the themes readme.txt we will need the Themes API, as described above, to return these data Requires at least and Requires PHP.

See #43986 and possibly #43987, #44350

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


6 years ago

This ticket was mentioned in Slack in #meta by afragen. View the logs.


6 years ago

#21 @afragen
6 years ago

  • Keywords servehappy added

This ticket was mentioned in Slack in #themereview by poena. View the logs.


6 years ago

#23 @dd32
6 years ago

In 8789:

Theme Directory: Start extracting headers from the readme.txt file in themes.

This is an MVP and does not use a proper readme parser as used in the Plugin Directory.
This is intended to collect the minimal header data we current have a need for.

In the future it's likely this will be expanded to allow more data from readme.txt to be utilised, but that's not the current priority.

See #3718

#24 @dd32
6 years ago

  • Owner set to dd32
  • Status changed from new to accepted

[8789] starts extracting some minimal header information, based off the themes readme.txt standard.

This data is intentionally not exposed in the API yet, this is the minimal steps needed to start extracting the data for later use - and also to act as a flag to easier identify which themes do have extractable readme.txt data (hopefully all of them).

We can start exposing the data in various places once we've got a few themes with data in the database - 3718.diff looks like it'll be mostly ready-to-go.

#25 @dd32
6 years ago

In 8794:

Theme Directory: Sanitize version numbers in the Theme readme.txt import process.

A number of themes include extra details in the fields such as 'WordPress', and others don't follow the plugin/theme readme stanard at all, this change validates the data a little more before importing.

It also removes the importing of the 'Tags' header as there's no immediate plans for it's use and we defer to the style.css Tags header instead.

See #3718.

#26 @dd32
6 years ago

Just noting that so far we've imported 49 themes (new submissions & updates) who have the Requires PHP header.. So far 8 require PHP 7.2.14, the rest are mostly split PHP 5.2 and 5.6.

Looking at the themes that specify 7.2.14, it doesn't look like any of them actually require PHP 7.2 (Tide confirms it too) so I'm not sure if it's Authors mis-using the field, preferring recent versions, or just copying from an example somewhere?

#27 @dd32
6 years ago

In 8895:

Theme Directory: Expose the PHP & WordPress version requirements within wordpress.org/themes based on the data extracted from readme.txt.

Props flixos90 for initial work, dd32.
See #3718.

#28 @dd32
6 years ago

In 8896:

Theme Directory: Bump cache busters after [8895].

See #3718.

#29 @dd32
6 years ago

In 8913:

Theme Directory: Add the requires and requires_php to the Theme objects.

Missed in [8895].
Props flixos90 for initial work, dd32.
See #3718.

#30 @dd32
6 years ago

In 8914:

Theme Directory: API: Define a constant statically to avoid edgecases.

See #3718.

#31 @dd32
6 years ago

In 8915:

Theme Directory: API: Change the cache format for api.wordpress.org/themes/update-check/.

See #3718.

#32 @dd32
6 years ago

This should now be rolled out.

As it turned out, I missed committing some of the code last week, but it's fixed now.

The API's (info, and update-checks) should now also be returning the data.
eg: https://api.wordpress.org/themes/info/1.2/?action=theme_information&request[slug]=doody

I've not backfilled the data for older themes, but I'm going to call this fixed for now, given the speed at which most themes get updated.

If going back and importing data for all themes is needed, let me know and I'll make it happen.

This ticket was mentioned in Slack in #meta by rabmalin. View the logs.


6 years ago

#34 @dd32
6 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

Data for older themes is being imported.

Note: See TracTickets for help on using tickets.