theme-review-action icon indicating copy to clipboard operation
theme-review-action copied to clipboard
verified publisher icon WordPress

Feedback: PHP error check

Open carolinan opened this issue 4 years ago • 3 comments

Example report: https://themes.trac.wordpress.org/ticket/99204#comment:1

/?p=1241 contains PHP errors: Notice: Undefined variable: bool in wp-content/themes/test-theme/functions.php on line 145
See: https://github.com/WordPress/theme-review-action/blob/trunk/docs/ui-errors.md#page-should-not-have-php-errors

I find the /?p=1241 confusing. I understand that this is a page or post id but when troubleshooting, it would be more helpful to know if this view is a single post or single page. The post id will not be the same if I was to use it to troubleshoot my theme.

wp-content/themes/test-theme/functions.php This is also confusing. Can wp-content/themes/test-theme/ be replaced with the theme name? (And I think whoever views the report will take it more serious if the correct theme name is used.)

Can it point and link to the Trac browser even? https://themes.trac.wordpress.org/browser/bluenest/1.0.1/functions.php#L145

The docs page says: This test expects that the plugin does not output any PHP errors. Is it meant to say the theme does not output any PHP errors? This can be updated to also mention notices and warnings.

carolinan avatar May 28 '21 10:05 carolinan

Exactly, we need more detailed information regarding the issue. If the issue is an error, we need to restrict the upload.

kafleg avatar May 28 '21 11:05 kafleg

If we know that there are no false positives then both PHP notices, warnings and errors should block upload.

carolinan avatar May 29 '21 03:05 carolinan

Perfect. Thanks for the ticket.

I find the /?p=1241 confusing. I understand that this is a page or post id but when troubleshooting, it would be more helpful to know if this view is a single post or single page. The post id will not be the same if I was to use it to troubleshoot my theme.

Yeah, that is true. Let's try to update the language.

This test expects that the plugin does not output any PHP errors. Is it meant to say the theme does not output any PHP errors?

Yes.

This can be updated to also mention notices and warnings.

Agree.

Tasks:

  • [x] Update page/post information to be more theme-specific and less testing data specific.
  • [x] Update docs to say theme instead of plugin.
  • [ ] Update docs to mention notices and warnings.

StevenDufresne avatar Jun 01 '21 01:06 StevenDufresne