Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Theme Review Project Structure #12

Open
1 of 5 tasks
StevenDufresne opened this issue Jul 13, 2021 · 25 comments
Open
1 of 5 tasks

Proposal: Theme Review Project Structure #12

StevenDufresne opened this issue Jul 13, 2021 · 25 comments

Comments

@StevenDufresne
Copy link

StevenDufresne commented Jul 13, 2021

Purpose

This issue is a proposal on how we could structure the technical aspects of the theme review process to converge on a collection of tools, new & existing.

The theme review process and it's associated infrastructure spreads out across numerous domains and includes different projects created in different languages. Rule sets are enforced linearly across the system making it difficult to identify gaps, add/modify rules or incorporate new tools.

The temptation when trying to automate an existing process is to rush into adding more rules to existing tools, but at this point, I think it would be more helpful to separate out the high-level concerns, solidify their rules sets and then assign the right tools. Some of the challenges are new and choosing the right tool for the specific job will be critical in this project's success.

For context, please read:

Table Of Contents

Technical Flow Chart #

 Technical flow chart

Parts #

The parts following represent a general responsibility.

Upload Interface #

Users currently upload their themes via the interface on WordPress.org. The interface provides some basic error messaging when themes have "upload critical" errors.

Default Error view
Upload view Upload with errors view

System Checks #

This is a collection of checks to make sure that the theme can be added to the directory. At a high level, the rules will make sure the theme is unique to the directory and follows the correct naming conventions. It will also make sure that the user is authorized to upload a theme.

See rule list

Structure Checks #

This is a collection of checks to make sure that the theme has the necessary files in the correct format for the theme type.

See rule list

Basic Distribution Checks #

This is a collection of checks that make sure the theme can be distributed based on proper licensing and the absence of overtly abusive structure/content.

See rule list

Static Code Review #

This is a diverse collection of checks that work on static code.

See rule list

User Experience Review #

The is a collection of checks that utilize a browser session to review a theme's user experience.

See rule list

Review Results Interface #

The theme review interface is currently a ticket on themes.trac.wordpress.org. Each theme gets a ticket and all output from the tests are presented to the uploadee.

Example Results

Next Steps #

  • Agree on the parts
  • Associate the correct tools to the correct parts
  • Assign rules to the correct tools
  • Identify which rules are completed (Not, Partially, Full).
  • Write the remaining tests.

Appendix #

These were gathered from this theme requirements issue and are subject to change. Additionally, this issue is not the source of truth and any questions or comments about specific rules should be added to the aforementioned issue.

System Check List #

Expand to see list
ID Rule Details Exception Tool
system6 All Themes can't use: WordPress, Theme, Twenty* in their name Theme Check

Basic Distribution Check List #

Expand to see list
ID Type Rule Details Exception Tool
distribute1 All Themes can include one single front facing credit link, which is restricted to the Theme URI or Author URI defined in style.css
distribute2 All Themes can have an additional footer credit link pointing to WordPress.org
distribute3 All users must state explicitly that the products you’re selling/distributing (free and paid) are GPL compatible that needs to be in an easy-to-find place for visitors.
distribute4 All Themes can't display “obtrusive” upselling
distribute5 All Themes must disclose all affiliates
distribute6 All Themes must be compatible with the GNU General Public License Although any GPL-compatible license is acceptable, using the same license as WordPress — “GPLv2 or later” — is strongly recommended. All code, data, and images — anything in the theme zip file — must comply with the GPL or a GPL-Compatible license.
distribute7 All Themes must include third-party libraries, code, images, or otherwise that are GPL-compatible For a specific list of compatible licenses, please read the GPL-Compatible license list on gnu.org.
distribute8 All Themes must declare copyright for the theme itself.
distribute9 All Themes must declare license, copyright information, and source for all resources included such as fonts or images. that is provided in a list of all resources in one file. unless the assets are public domain, where copyright is not included.
distribute10 All Themes must include code and design that are your own or legally yours Cloning of designs is not acceptable
distribute11 All Themes must only display the user’s copyright on the front end not the theme author’s copyright.
distribute12 All Themes Warning: Showing preview/demo data or manipulating the preview on WordPress.org is not allowed and can result in suspension or your user account being terminated
distribute13 All Themes must spell “WordPress” correctly in all public-facing text: all one word, with both an uppercase W and P
distribute14 All Themes can't have trademark violations in their content.
Structure11 All Themes can't have images that promote hate or violence or images that show children with recognizable facial or body features.
Structure12 All Themes can't have a screenshot that looks like an advertisement The reviewer can subjectively ask you to change screenshots if they find that it is not appropriate.
system4 A user can only distribute themes that are 100% compatible with GPL.

Structure Check List #

Expand to see list
ID Type Rule Details Exception Tool
Structure2 All Themes must include the main stylesheet style.css Theme Check and Theme Review action
Structure3 All Themes must use headers in style.css that follow the guidelines and requirements for the main stylesheet in the Theme Developer Handbook. Theme Check
Structure4 All Themes can include 'Theme URI' in style.css that must be about the theme hosted on WordPress.org. wordpress.org is reserved for the default themes (Twenty *). The exception is checked with Theme Check
Structure5 All Themes can include 'Author URI' in style.css that links to a page or website about the author, author theme shop, or author project/development website.
Structure7 All Themes must include a readme.txt file that matches the format located here. Possibly the readme validator
Structure9 All Themes can't have minification of scripts or files unless the original files are also in the theme folder.
Structure13 All Themes can't have a screenshot bigger than 1200 x 900px Theme Check
Structure14 All Themes must a ratio of width to height of 4:3 Theme Check
Structure15 Block Themes must include a Index.php Theme Check, Theme review action
Structure16 Block Themes must include a style.css Theme Check, Theme review action
Structure17 Block Themes must include a readme.txt Theme Check
Structure18 Block Themes must include a theme.json Theme Check, Theme review action
Structure19 Block Themes must include a index.html that is inside a folder called block-templates. Theme Check, Theme review action

Static Code and User Experience Review rules #

There rules are categorized to into Safe, Global and Quality in order to make them more digestible. For this moment in time, they are internal.

Safe

These rules indicate whether the theme is safe to install.

Expand to see list
ID Type Rule Details Exception Tool
safe1 All Themes must disable any tracking and collection of user data by default and must be opt-in.
safe2 All Themes must include documentation on how any user data is collected, and used, and needs to be included in the theme readme.txt file, preferably with a clearly stated privacy policy.
safe3 All Themes can't have PHP or JavaScript errors, warnings or notices. Theme review action
safe4 All Themes must validate and/or sanitize untrusted data before entering it into the database
safe5 All Themes must escape all untrusted data before output (See: Data Validation)
safe6 All Themes must Provide a unique prefix for everything the theme defines in the public namespace including options, functions, global variables, constants, post meta, wp_enqueue_script/style handle names, add_image_size names, wp_script_add_data keys, slugs/ids for new categories created with register_block_pattern_category etc. unless its a menu location or sidebar id. wp_enqueue_script & wp_enqueue_style handles should not be prefixed if they are 3rd-party assets, like a framework’s CSS files or any 3rd-party script. Possibly PHPCS sniff
safe7 All Themes can't include zip files in the theme folder Theme check
safe8 All Themes can't include plugin functionality If you are not sure if a feature is plugin territory, contact the team and ask first. [email protected].
safe9 All Themes can recommend plugins that are hosted on WordPress.org
safe10 All Themes must only install plugins by installed by user action
safe11 All Themes can't include plugins in the theme folder
Structure1 All Themes can't include remote resources without user consent. unless the resource is from Google Fonts.
Structure10 All Themes must use WordPress’ default libraries. WordPress includes a number of libraries such as jQuery. For security and stability reasons themes may not include those libraries in their own code. Instead themes must use the versions of those libraries packaged with WordPress. For a list of all JavaScript libraries included in WordPress, please review Default Scripts Included and Registered by WordPress.

Global

These rules indicate whether the theme is ready for a global user base.

Expand to see list
ID Type Rule Details Exception Tool
global1 All Themes must have skip links that include a mechanism that enables users to navigate directly to content or navigation on entering any given page unless it is a block theme, where skip links are added automatically to the main element.
global2 All Themes must have skip links that may be positioned off-screen initially but must be available to screen reader users and must be visible on focus for sighted keyboard navigators Theme review action
global3 All Themes must have skip links that are the first focusable element perceived by a user via a screen reader or keyboard navigation Theme review action
global4 All Themes must have skip links that are visible when keyboard focus moves to the link Theme review action
global5 All Themes must have skip links that move focus to the main content area of the page when activated unless there is nothing to skip past, such as a menu or larger header section or secondary widget area before the main content. Theme review action
global6 All Themes must have keyboard navigation that provide visual keyboard focus highlighting in navigation menus and for form fields, submit buttons and text links. Theme review action
global7 All Themes must have keyboard navigation that makes all controls and links reachable by keyboard. Theme review action
global8 All Themes must have keyboard navigation that makes all controls usable with the mouse usable with the keyboard, regardless of device and screen size. Including but not limited to responsive versions for small screens, mobile and other touch screen devices. Theme review action
global9 All Themes must have underlined links within content and comments that are distinguishes by an underlined a no other style unless they are in navigation-like contexts (e.g. menus, lists of upcoming posts in widgets, grouped post meta data)
global10 All Themes must meet additional requirements if the theme has the tag ‘accessibility-ready’
global11 All Themes must use gettext for all text strings for translation
global12 All Themes must include the theme slug as the text-domain in style.css that is the name of the theme in lower case, with spaces replaced by -. It is also the folder name for the theme. Theme Check
global13 All Themes If the theme uses a framework then no more than 2 unique slugs may be used (like tgmpa, redux-framework, kirki or some other allowed framework) Theme Check

Quality

These rules indicate whether the theme is of minimum quality.

Expand to see list
ID Category Type Rule Details Exception Tool
quality1 Classic Themes must have a valid DOCTYPE declaration and include language_attributes(). Theme Check
quality2 Classic Themes must call custom template files using get_template_part() or locate_template().
quality3 Classic Themes must display the correct content according to the front page setting Theme review action
quality4 Classic Themes can use the Customizer for implementing theme options
quality5 Classic Themes should use edit_theme_options capability for determining user permission to edit options, rather than relying on a role (e.g. administrator), or a different capability (e.g. edit_themes, manage_options).
quality6 Classic Themes must call their respective template file functions header.php (via get_header()), footer.php (via get_footer()), sidebar.php (via get_sidebar()), searchform.php (via get_search_form()) unless they are not included
quality7 Classic Themes must include relevant function if specific templates wp_head(), body_class() post_class(), wp_link_pages(), the_comments_navigation(), the_comments_pagination(), the_posts_pagination(), the_posts_navigation(), wp_footer() unless they are not included
quality8 Block Themes must have completed block templates. Theme review action
quality9 All Themes must use the admin_notices API for all notifications generated by the theme.
quality10 All Themes must make notices dismissible.
quality11 All Themes must follow core UI design for everything wrapped in the admin notice
quality12 All Themes can't place WordPress features behind a paywall
quality13 All Themes can't remove, hide, or otherwise block the admin bar from appearing Theme check (warning)
quality14 All Themes can't redirect on theme activation or modify activation
quality15 All Themes can't include custom post types Theme Check
quality16 All Themes can't include custom blocks Theme Check
quality17 All Themes can't include shortcodes Theme Check
quality18 All Themes can't include functionality that is not related to design and presentation.
quality19 All Themes can't include custom roles Theme Check
quality20 All Themes can't include custom user contact methods Theme Check
quality21 All Themes can't include custom mime types Theme Check
@carolinan
Copy link

carolinan commented Jul 13, 2021

I believe we have agreed on the parts and can consider that step complete.

We must not forget about documentation, and choosing a format for the documentation.

For step 2, Associate the correct tools to the correct parts

Here are some tools or references:

Also:
https://wordpress.org/plugins/child-theme-check/

@carolinan
Copy link

I have made the following changes:

Updated the text in Structure 4

Removed Structure 20, (child themes)
Removed safe 7-9, demo content, and moved the following items up the list.
Removed duplicate text for files required for block themes.

Added quality 19-21 (custom roles, mime types, user contact methods)

@carolinan
Copy link

system3 -A user can only submit a complete theme
This is covered by checking that the required files are included,

  • Names cannot be “reserved” for future use or to protect brands.
    Authors used to upload a blank theme and then close the Trac ticket themselves to make sure that no one else could use the theme name. I have not seen this happen for a long time, partly because most authors no longer have the Trac privilege to close tickets. This is more about author behavior and I don't think it can be checked automatically.

@carolinan
Copy link

carolinan commented Jul 20, 2021

System2 -A user can submit unlimited updates for their existing themes that are in the theme directory.
This is already true, it is how Trac and uploads work right now. It is not a requirement but more of a way to answer a frequently asked question.

@carolinan
Copy link

I updated the text in distribute 11 to clarify that this requirement is for copyright displayed on the front.
distribute11 | All Themes | must | only display the user’s copyright on the front end | not the theme author’s copyright.

@carolinan
Copy link

For distribute9, the themes team goes further than what public domain requires, because, we need to be able to confirm that the image is in the public domain.
License information and source still needs to be included, but the copyright information can not be included because public domain works are not protected by copyright.

I updated the text to:
distribute9 | All Themes | must | declare license, copyright information, and source for all resources included such as fonts or images. | that is provided in a list of all resources in one file. | unless the assets are public domain, where copyright is not included.

@carolinan
Copy link

I updated structure2 to say:
Structure2 | All Themes | must | include the main stylesheet style.css
Previous:
Structure2 | All Themes | Main stylesheet

@carolinan
Copy link

I updated quality 8 from should to must.
quality8 | Block Themes | must | have completed block templates.

@carolinan
Copy link

distribute 5 changed to:
distribute5 | All Themes | must | disclose all affiliates

@StevenDufresne
Copy link
Author

StevenDufresne commented Jul 21, 2021

system3 -A user can only submit a complete theme
This is covered by checking that the required files are included,

  • Names cannot be “reserved” for future use or to protect brands.
    Authors used to upload a blank theme and then close the Trac ticket themselves to make sure that no one else could use the theme name. I have not seen this happen for a long time, partly because most authors no longer have the Trac privilege to close tickets. This is more about author behavior and I don't think it can be checked automatically.

Let's remove any items that aren't a test (or tests) itself. The structure checks will cover it.

[Edit]
Same with system4, the distribution checks should cover it.

@StevenDufresne
Copy link
Author

@carolinan Apart from my last comment, is this the final list of checks? Have they all been validated and agreed upon?

@StevenDufresne
Copy link
Author

StevenDufresne commented Jul 21, 2021

I've moved Structure11, Structure12 to the distribution list. I left the ids as is.

[Edit] Moved Structure1, Structure10 to "safe".

We'll need to move to numberless & category-less ids in the next iteration to allow us flexibility as we storm this out.

@StevenDufresne
Copy link
Author

StevenDufresne commented Jul 21, 2021

Here are my thoughts on: Associate the correct tools to the correct parts.

System Checks

Tool: class-wporg-themes-upload.php

This should continue to run its checks, which I documented in #13. Its checks will have some overlap with other tools but it does so with respect to its own purposes which are specific to creating the entry on WordPress.org and filling in the trac ticket. We should not rely on any of these outcomes for the theme review.

Instead of returning each error 1 at a time, we should combine all the errors and output them so users can work on multiple problems at once.

If the theme passes these checks, the following checks should be triggered:

  • Structure Checks
  • Basic Distribution Checks

Any errors found by the tests above should trigger the same error messages and prevent the theme from being uploaded.

Structure Checks

Tool: Static Code Analysis( Theme-Check/Theme Sniffer/Other )

I'm not sure which tools make the most sense here, but I think the key is that we want to run a subset, priority-1 tests, that look specifically for missing files (and core content). We can circle back and validate more of the details of files with more specialized tools and tests in the Static Code and User Experience Review.

Basic Distribution Checks

Tool: Static Code Analysis( Theme-Check/Theme Sniffer/Other )

Here too, we should focus on priority-1 issues that look specifically for missing licensing directly related to the theme. We can cycle back and validate other licensing and imagery in the Static Code and User Experience Review.

Static Code and User Experience Review rules

Tool:

  • Static Code Analysis ( Theme-Check/Theme Sniffer/Other )
  • User Experience (Theme review action via jest)

Provided we made it through the priority-1 tests above, the rest of the suite can be divided between the User Experience and the Static Code toolset and triggered asynchronously.

Notes

I've include Theme-Check/Theme Sniffer/Other because I'm not certain which would be the best static code tool and I'm probably not the best resource to lead that discussion.

My gut tells me to keep things simple and use the theme-check for the priority-1 structure and distribution tests and then get into a bigger debate on what to use for the more complicated Static Code review.

What are others thinking? Any other ideas?

@carolinan
Copy link

This match what I had in mind, thank you for putting words to it.

@carolinan
Copy link

@carolinan Apart from my last comment, is this the final list of checks? Have they all been validated and agreed upon?

There is a request for feedback out this week, where we are requesting comments until July 26.

I am still hoping that we can find more ways to reduce the requirements.
For example, the screenshot size and ratio requirements could be a fix for the theme directory itself:
https://meta.trac.wordpress.org/ticket/5834

@StevenDufresne
Copy link
Author

I am still hoping that we can find more ways to reduce the requirements.
For example, the screenshot size and ratio requirements could be a fix for the theme directory itself:
https://meta.trac.wordpress.org/ticket/5834

What about global14... can use any text, must be the same.

I don't know the history but... to me, it applies to a small subset of themes and I would think that other factors like the quality of designs and quality of the copy have a bigger impact on a theme's quality.

@carolinan
Copy link

I am still hoping that we can find more ways to reduce the requirements.
For example, the screenshot size and ratio requirements could be a fix for the theme directory itself:
https://meta.trac.wordpress.org/ticket/5834

What about global14... can use any text, must be the same.

I don't know the history but... to me, it applies to a small subset of themes and I would think that other factors like the quality of designs and quality of the copy have a bigger impact on a theme's quality.

Like WordPress, all themes must be translatable. This is a pretty common problem, and it prevents translation.
If a theme is in both Japanese and English, and I only know English, then I can't translate it.

@carolinan
Copy link

carolinan commented Jul 28, 2021

Structure tests, Priority 1

? Valid theme name (does not contain WordPress, Theme, Twenty Twenty-series) (system6)

Include style.css, index.php, readme.txt, screenshot (Structure2, Structure7)
Block themes also include theme.json, block-templates/index.html (structure18, structure19)

Basic Distribution Checks Priority 1

Confirm that the style.css and readme.txt headers contains a valid license and license URL. (distribute6)
Confirm that the theme includes copyright information for the theme itself. Unless the theme is in the public domain. (distribute8)

@carolinan
Copy link

Removed:

|Structure6|All Themes|must|have tags in style.css that match what the theme actually does in respect to functionality and design|and doesn't use more than 3 subject tags (See: Theme Tag List).||

@carolinan
Copy link

Removed
global14 | All Themes | can | use any language for text | that only uses one language |  

https://make.wordpress.org/themes/2021/08/06/summary-of-the-request-for-feedback-on-requirement-changes/

@carolinan
Copy link

carolinan commented Aug 12, 2021

system7 Themes can't have trademark violations in their name.
Clarification about trademark usage: It only covers WordPress.
https://make.wordpress.org/themes/2021/03/17/next-steps-on-themes-and-reviews/#comment-44206

Therefor, this requirement is the same as system6: Themes can't use: WordPress, Theme, Twenty* in their name,
and I will remove it from the list.

@carolinan
Copy link

distribute8 | All Themes | must | declare copyright for the theme itself.

This is a static file content check (that does not previously exist in the theme standard for PHPCS)
and an issue has been opened for Theme Check: WordPress/theme-check#382

@carolinan
Copy link

carolinan commented Aug 12, 2021

Distribute13 | All Themes | must | spell “WordPress” correctly in all public-facing text: all one word, with both an uppercase W and P
See existing issue: WordPress/theme-check#131

There is a check for this in WPCS: https://github.com/WordPress/WordPress-Coding-Standards/blob/develop/WordPress/Sniffs/WP/CapitalPDangitSniff.php

@carolinan
Copy link

quality19 | All Themes | can't | include custom roles
See WordPress/theme-check#383

@carolinan
Copy link

carolinan commented Aug 18, 2021

Of the remining requirements that do not already have automated checks, I believe that these are the ones we can add checks for:

Doable:

distribute8 | All Themes | must | declare copyright for the theme itself.| | Theme check -issue opened |

quality14 | All Themes | can't | redirect on theme activation or modify activation| Theme check -issue opened |

quality5 | Classic Themes | should | use edit_theme_options capability for determining user permission to edit options, rather than relying on a role (e.g. administrator), or a different capability (e.g. edit_themes, manage_options).
Note: I recommend a warning for this instead of an error in case it is difficult to create a check without false positives.

Doubtful:

Structure9 | All Themes | can't | have minification of scripts or files |   | unless the original files are also in the theme folder.
What we can do:

  • Confirm that a file is minified
  • Warn if there is a file called name.min.js but not name.js. There would be many false positives for this and authors
    bundle more than one script in the same file.

What we can't do:

  • Unminify the file and do a line by line comparison to see if the minified and normal file match.
  • This is likely not possible because it depends on how the file was minified.

quality2 | Classic Themes | must | call custom template files using get_template_part() or locate_template().
-Only doable if we can identify what we mean with "custom template files".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants