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

Try: Add simplified readme validator #376

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

carolinan
Copy link
Collaborator

@carolinan carolinan commented Jul 29, 2021

Fixes #375

This check and the accompanying parser is copied from the plugin readme validator.

It is more limited than the plugin readme validator, and it does not work for themes that are in sub folders.
The subfolder issue may be solved by #370. Untested.

How to test:

  1. Test with a theme that is missing a readme.txt and readme.md
  2. Confirm that the error about the missing readme is displaying.
  3. Test with a theme with missing headers in the readme.
  4. Confirm that the warnings are displaying.
  5. Test with a theme with incorrect headers in the readme.
  6. Confirm that the warnings are displaying.

@carolinan
Copy link
Collaborator Author

The report:
Theme check displays a result with missing headers in the readme file

Replace short array syntax
Remove the readme.txt file from class-file-check.php
Update the error message when the file is missing.
@carolinan carolinan marked this pull request as ready for review August 3, 2021 09:27
@TeBenachi
Copy link
Contributor

Step 2. Tested and can confirm the missing error.

Step 4. Tested and confirmed.

Step 6. Tested and confirmed. Screenshot attached.

One thing I noticed. There was no warning when I added multiple contributors.

screenshot2

@carolinan
Copy link
Collaborator Author

carolinan commented Aug 11, 2021

Thank you.
Yes good point about the contributors. I left that out since the Theme Check plugin can not verify the user name (which the plugin readme validator can).

@kafleg
Copy link
Member

kafleg commented Aug 11, 2021

Test with a theme that is missing a readme.txt and readme.md
Tested and worked. Also, i liked this REQUIRED: Found a copy of Underscores. See README.md. Learn how to update the files for your own theme. If there is README.md file of default underscores found.

Confirm that the error about the missing readme is displaying.
Confirmed - REQUIRED: The readme file is missing.

Test with a theme with missing headers in the readme.
Confirmed.

README WARNING: The Tested up to field is missing from the readme.
README WARNING: The Requires at least field is missing from the readme.
README WARNING: The Requires PHP field is missing from the readme.
README WARNING: The License field is missing from the readme.
README WARNING: The License URI field is missing from the readme.
README WARNING: The Contributors field is missing from the readme.

Confirm that the warnings are displaying.
Done, please check above.

Test with a theme with incorrect headers in the readme.
Only one issue shown, README WARNING: The Requires at least field in the readme was ignored. This field should only contain a valid WordPress version such as 0.0 or -0.1.

my readme header is like below,

Contributors: templatesellxyz
Tags: red, cat, dog
Requires at least: 22.5
Requires PHP: 9.0
Tested up to: 4.3
Stable tag: 1.2.9
License: License.txt
License URI: wordpress.org

Confirm that the warnings are displaying.
Check above.

On a side note, is this INFO necessary?

README INFO: No == Frequently Asked Questions == section was found in the readme.
README INFO: No == Description == section was found in the readme.
README INFO: No == Changelog == section was found in the readme.

@carolinan
Copy link
Collaborator Author

carolinan commented Aug 11, 2021

The requires at least is compared to $latest_wordpress_version = '5.8'; so 22.5 should fail. How can we document this?
This hard coded value is problematic, but there is no constant with the latest WordPress version that can be used.

@carolinan
Copy link
Collaborator Author

The info does make it noisy. My thought was to:
-Match the plugin validator info
-Prepare for when the info is shown in the directory.

But if you prefer it can be removed and reconsidered once there are solid plans to show it in the directory.

@TeBenachi
Copy link
Contributor

How about something like this?

The Requires at least field requires the least WordPress version. This field should only contain a valid WordPress version such as 0.0 or 0.1.

I didn’t notice it yesterday but there is “ - " (minus sign) in the Requires at least warning message?

screenshot3

@carolinan
Copy link
Collaborator Author

Yeah I think it is broken. I'd like to compare with the live plugin validator.

@kafleg
Copy link
Member

kafleg commented Aug 12, 2021

Looks good.

README WARNING: The Contributors field should only contain one WordPress.org username. Remember that usernames are case-sensitive.

@kafleg
Copy link
Member

kafleg commented Aug 12, 2021

protected $latest_wordpress_version = '5.8';

We need to make it dynamic.

@TeBenachi
Copy link
Contributor

TeBenachi commented Aug 12, 2021

Contributors: Ginger Snap

When I added 2 users without “,” comma between them, the check returned no error.
I know most of the people use “,” comma to separate strings but should we also add a check in case?

Contributors: Ginger, Snap worked perfectly.

@carolinan
Copy link
Collaborator Author

I have not used the sniffer for so long, that I forgot that it checks for this:
https://github.com/WPTT/theme-sniffer/blob/development/src/sniffs/readme/class-contributors.php

@carolinan
Copy link
Collaborator Author

protected $latest_wordpress_version = '5.8';

We need to make it dynamic.

But how?

@TeBenachi
Copy link
Contributor

I have not used the sniffer for so long, that I forgot that it checks for this:
https://github.com/WPTT/theme-sniffer/blob/development/src/sniffs/readme/class-contributors.php

Thanks for the link. I got the contributor validation working based on the sniffer. It’s silly but I don’t know how to create a branch off a branch. Any suggestions on how to proceed.

checks/class-readme-check.php Outdated Show resolved Hide resolved
Co-authored-by: Dion Hulse <dion@wordpress.org>
@kafleg
Copy link
Member

kafleg commented Aug 13, 2021

@dd32 What will happen if my local WordPress version is less than the current version?

@kafleg
Copy link
Member

kafleg commented Aug 13, 2021

No notice is shown in this case, Tested up to: 5.9

But if we go above 6.0 then it shows, README WARNING: The Tested up to field in the readme was ignored. This field should only contain a valid WordPress version such as 5.8 or 5.9.

If the WordPress version will be higher than 5.9, then do we need to reset the version again?

Nothing shows for, Requires PHP: 22.5, here 22.5 is just a testing value.

Rest of the things looks fine to me.

@kafleg
Copy link
Member

kafleg commented Aug 14, 2021

I found one thing,
If there is space in readme header items, then it shows the errors.

image

And errors list,
image

Once I removed that space, only the error of contributors is displayed.
image

@carolinan
Copy link
Collaborator Author

carolinan commented Aug 16, 2021

If the WordPress version will be higher than 5.9, then do we need to reset the version again?

We would have had to but Dion's update solves that.

@kafleg
Copy link
Member

kafleg commented Aug 16, 2021

If the WordPress version will be higher than 5.9, then do we need to reset the version again?

We would have had to but Dion's update solves that.

Okay, thanks for the update.

Any idea how to fix the readme issues with spacing?

@carolinan
Copy link
Collaborator Author

I have not used the sniffer for so long, that I forgot that it checks for this:
https://github.com/WPTT/theme-sniffer/blob/development/src/sniffs/readme/class-contributors.php

Thanks for the link. I got the contributor validation working based on the sniffer. It’s silly but I don’t know how to create a branch off a branch. Any suggestions on how to proceed.

I am not sure either 😊 Maybe we need to do it in two steps and merge this one first?

@carolinan
Copy link
Collaborator Author

carolinan commented Aug 16, 2021

Any idea how to fix the readme issues with spacing?

I'll see if I can make it remove blank lines from the header, but the warning is also correct, the blank line must not be there.

Check if the name in the readme matches the theme name, if not, error.
Update the $latest_wordpres_version variable.
Remove empty lines to prevent errors with the check.
@carolinan
Copy link
Collaborator Author

8e696d2 Checks if the name in the readme matches the theme name, if not, error.
Updates the $latest_wordpres_version variable.
Removes empty lines to prevent errors with the check.

To avoid reading the wrong readme file if there is more than one readme, this PR needs to be tested together with
#370.

@kafleg
Copy link
Member

kafleg commented Aug 19, 2021

Theme readme file,
image

But there is no error/notice shown.
image

I checked again with removing other,
image

Now, I can see an error notice for Tested up to, Requires at least and Requires PHP.
image

@carolinan
Copy link
Collaborator Author

Does the license and license URI have a single space after the colon?

@kafleg
Copy link
Member

kafleg commented Aug 19, 2021

Requires at least: 5.5
Tested up to: 5.8
Requires PHP: 7.4
Stable tag: 1.0.0
License:
License URI:

Tested with both adding space, removing space, double space. The result is same.

@carolinan
Copy link
Collaborator Author

I think I found the bug where it does not show that the license is missing, but I need your full readme file to confirm.

It is not only checking the file header, so if the words "License: blah blah" are elsewhere in the readme file, like for an asset, it will use that.

Try to solve timeout issues.
Check for uppercase file names as well as lowercase with stripos.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The readme.txt file check is not specific enough
4 participants