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

Feedback: PHP error check #34

Open
carolinan opened this issue May 28, 2021 · 3 comments
Open

Feedback: PHP error check #34

carolinan opened this issue May 28, 2021 · 3 comments
Labels
documentation Improvements or additions to documentation

Comments

@carolinan
Copy link
Contributor

carolinan commented May 28, 2021

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 carolinan changed the title Feedback: PHP errors Feedback: PHP error check May 28, 2021
@kafleg
Copy link
Member

kafleg commented May 28, 2021

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

@carolinan
Copy link
Contributor Author

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

@StevenDufresne
Copy link
Contributor

StevenDufresne commented Jun 1, 2021

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:

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

@StevenDufresne StevenDufresne added the documentation Improvements or additions to documentation label Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants