WordPress / theme-review-action

Other
30 stars 9 forks source link

Feedback: PHP error check #34

Open carolinan opened 3 years ago

carolinan commented 3 years ago

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.

kafleg commented 3 years ago

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

carolinan commented 3 years ago

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

StevenDufresne commented 3 years ago

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: