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

Place information about all existing checks with errors and requirements in one document #10

Open
carolinan opened this issue Jun 10, 2021 · 20 comments

Comments

@carolinan
Copy link

carolinan commented Jun 10, 2021

The purpose of this issue is to place information about all existing requirements in one document.

  1. Collect the information
  2. Requirements that should be reduced to recommendations will be removed from the list and placed in Not required but recommended issues #11
  3. Publish a proposal with the new, remaining requirements on the make blog.

Theme Review action

https://github.com/WordPress/theme-review-action

Check Level Is it on the requirements page?
Structure check -check for required files depending on theme type. Required/error yes
Skip links warning (temporarily) yes
Keyboard navigation -Partial warning (temporarily) yes
Links within content and comments must be underlined warning (temporarily) yes
No PHP or JavaScript errors, warnings or notices Required/Error yes
Themes can include one single front facing credit link, which is restricted to the Theme URI or Author URI defined in style.css required/error yes
Block themes: Include required files: Index.php, style.css, readme.txt, theme.json, and index.html inside a folder called block-templates. Required/Error yes
Block templates should be complete Required/Error yes

Theme Check

https://github.com/WordPress/theme-check

Check Run on block theme Level Is it on the requirements page?
Bad things yes required not the details, the requirements page only says that the theme must be secure.
Basic (doctype, body class, theme support) no required yes, but in separate places.
cdn -can be replaced with an allowed list? yes required yes
constants Without statistics, it is impossible to know if theme authors still use these and we need to check for them 🤷‍♀️ no required yes, under 4, but not listed by name.
customizer (checks for sanitize callbacks) no required yes
Directories (checks for .git etc) yes required yes
Escaping yes warning, required yes
Filenames checks for development config files, php.ini etc. yes required yes, add as example under Files
FSE required files yes required yes
Included zip files ⚠️ This does not need to be its own check, move to filenames? yes required yes, under plugins
Line endings this is required because of svn yes required yes
Non GPL sites yes required yes, in the license requirements
Plugin territory yes required yes
Screenshot yes recommended, required yes
Style CSS Header yes required yes
Title tag theme support no required yes
Theme and author URI -theme uri can not be WordPress.org yes required yes
Worms yes required not the details, the requirements page only says that the theme must be secure.
Underscores _s no required yes

@carolinan carolinan changed the title Place information about all existing checks in one document Place information about all existing checks and requirements in one document Jun 10, 2021
@carolinan carolinan changed the title Place information about all existing checks and requirements in one document Place information about all existing checks with errors and requirements in one document Jun 11, 2021
@carolinan
Copy link
Author

carolinan commented Jun 15, 2021

Of the theme check checks, I suggest that the following are moved to warnings or recommended:

  • Generated themes
  • Screen reader text CSS class- The theme may look ugly, but its the authors responsibility.
  • Favicon (this will allow themes to include a favicon)
  • Deprecated, both checks
  • Constants
  • Post formats (Because I personally don't care if you style the aside like an aside...)
  • Customizer -Only because the current checks for sanitize callbacks are not good enough and block theme uploads if variables are used.
  • The file names list is controversial. I suggest removing anything from it that isn't a security risk.

@kafleg
Copy link

kafleg commented Jun 16, 2021

For post formats tag, it should be requirement and need to block upload. Because theme users use filters while searching themes on theme page based on tags. But if the theme having post-formats tags is not properly styled and support it, that makes no sense.

Here is a ticket for more reference.

@carolinan
Copy link
Author

carolinan commented Jun 16, 2021

In my opinion, that does not align with https://make.wordpress.org/themes/2021/03/17/next-steps-on-themes-and-reviews/

"One of the action items from our call with Matt, was to come up with a list of non-negotiable guard rails for what could prevent a theme from being added to the repo. After some good discussions with him, I have a short list of guard rails but with the clarification that they shouldn’t prevent authors from submitting themes."

@carolinan
Copy link
Author

Changelog: reduced the search form check to warning and improved the explanation.

@carolinan
Copy link
Author

Style tags checks for allowed and deprecated tags in style.css -I propose that these should all be info's.

@carolinan
Copy link
Author

Theme and author URI -theme uri can not be WordPress.org -This one is kept because it falls under the new Trademark requirement?

@kafleg
Copy link

kafleg commented Jun 16, 2021

In my opinion, that does not align with https://make.wordpress.org/themes/2021/03/17/next-steps-on-themes-and-reviews/

"One of the action items from our call with Matt, was to come up with a list of non-negotiable guard rails for what could prevent a theme from being added to the repo. After some good discussions with him, I have a short list of guard rails but with the clarification that they shouldn’t prevent authors from submitting themes."

Okay, I got the point.

@aristath
Copy link

The plugin-territory functions currently includes 4 functions: https://github.com/WordPress/theme-check/blob/17dd4eb9393a6e6eca45401f01f629fe752e81e5/checks/class-plugin-territory-check.php#L33-L38

  • register_post_type: This should definitely be kept because if we remove it then users will lose content when switching themes.
  • register_taxonomy: Same as register_post_type. Taxonomies can be used to add content too, and categorize/organize content. Removing a taxonomy when the theme changes would be detrimental to users and their content.
  • wp_add_dashboard_widget: I believe we can remove this. It is not content-related and does not create lock-in.
  • register_block_type: This needs to be kept. Blocks are used to build content, and having a block registered in a theme would lock users to that theme and prevent them from changing 'cause otherwise they'll bet broken blocks in their content.

@aristath
Copy link

I think we can also remove the appearance-menu rule...

@carolinan
Copy link
Author

Admin menu rules reduced and removed from the requirements page draft 🎉

@kafleg
Copy link

kafleg commented Jun 16, 2021

I think we can remove favicon requirement too. It's been so long and I haven't seen any theme that is summitted having favicon functionality from theme.

@kafleg
Copy link

kafleg commented Jun 17, 2021

Suggest to remove - Menu locations and sidebar IDs are exceptions from prefixing section. If that is exception, then it will only create confusion.

@kafleg
Copy link

kafleg commented Jun 17, 2021

I suggest to change the text here, Place WordPress core features behind a paywall.

@aristath
Copy link

I suggest to change the text here, Place WordPress core features behind a paywall.

Why change this? They can place their own features behind a paywall if they want, but not things that are from core or plugins. Changing the text to include "core" makes it too specific and they can say "oh, so I can add a paywall in my theme and disable variable-products in WooCommerce 'cause styling for them is only included in the premium version of my theme".
Leaving it without "core" makes more sense IMO

@kafleg
Copy link

kafleg commented Jun 17, 2021

I suggest to change the text here, Place WordPress core features behind a paywall.

Why change this? They can place their own features behind a paywall if they want, but not things that are from core or plugins. Changing the text to include "core" makes it too specific and they can say "oh, so I can add a paywall in my theme and disable variable-products in WooCommerce 'cause styling for them is only included in the premium version of my theme".
Leaving it without "core" makes more sense IMO

Thanks. I just thought in a straight way.

@carolinan
Copy link
Author

For the file list,

		$blocklist = array(
			'thumbs\.db'          => __( 'Windows thumbnail store', 'theme-check' ),
			'desktop\.ini'        => __( 'windows system file', 'theme-check' ),
			'project\.properties' => __( 'NetBeans Project File', 'theme-check' ),
			'project\.xml'        => __( 'NetBeans Project File', 'theme-check' ),
			'\.kpf'               => __( 'Komodo Project File', 'theme-check' ),
			'^\.+[a-zA-Z0-9]'     => __( 'Hidden Files or Folders', 'theme-check' ),
			'php\.ini'            => __( 'PHP server settings file', 'theme-check' ),
			'dwsync\.xml'         => __( 'Dreamweaver project file', 'theme-check' ),
			'error_log'           => __( 'PHP error log', 'theme-check' ),
			'web\.config'         => __( 'Server settings file', 'theme-check' ),
			'\.sql'               => __( 'SQL dump file', 'theme-check' ),
			'__MACOSX'            => __( 'OSX system file', 'theme-check' ),
			'\.lubith'            => __( 'Lubith theme generator file', 'theme-check' ),
			'\.wie'               => __( 'Widget import file', 'theme-check' ),
			'\.dat'               => __( 'Customizer import file', 'theme-check' ),
			'phpcs\.xml\.dist'    => __( 'PHPCS file', 'theme-check' ),
			'phpcs\.xml'          => __( 'PHPCS file', 'theme-check' ),
			'\.xml'               => __( 'XML file', 'theme-check' ),
			'\.sh'                => __( 'Shell script file', 'theme-check' ),
			'postcss\.config\.js' => __( 'PostCSS config file', 'theme-check' ),
			'\.editorconfig.'     => __( 'Editor config file', 'theme-check' ),
			'\.stylelintrc\.json' => __( 'Stylelint config file', 'theme-check' ),
			'\.eslintrc'          => __( 'ES lint config file', 'theme-check' ),
			'favicon\.ico'        => __( 'Favicon', 'theme-check' ),
		);

Are we only removing these for now?

'phpcs\.xml\.dist'    => __( 'PHPCS file', 'theme-check' ),
'phpcs\.xml'          => __( 'PHPCS file', 'theme-check' ),
'postcss\.config\.js' => __( 'PostCSS config file', 'theme-check' ),
'\.editorconfig.'     => __( 'Editor config file', 'theme-check' ),
'\.stylelintrc\.json' => __( 'Stylelint config file', 'theme-check' ),
'\.eslintrc'          => __( 'ES lint config file', 'theme-check' ),

I don't know anything about

			'project\.properties' => __( 'NetBeans Project File', 'theme-check' ),
			'project\.xml'        => __( 'NetBeans Project File', 'theme-check' ),
			'\.kpf'               => __( 'Komodo Project File', 'theme-check' ),

@carolinan
Copy link
Author

I closed WordPress/theme-check#126 and admin pointers are not listed on the requirements page.

@carolinan
Copy link
Author

PR's submitted today for:

File check (still required)
title tag and wp_title -changed to recommended
wp_filesystem -one check changed to info, others remain as warnings.

Once merged, the list above will be updated.

@carolinan
Copy link
Author

There is also a PR for updating the favicon check to prevent it from stopping uploads.

@StevenDufresne
Copy link

I opened #13 not sure if it made sense to add it here. Thoughts?

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

4 participants