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

Incorrect display of readme.txt instead of readme.md in Plugin Readme check #239

Merged
merged 2 commits into from
Aug 3, 2023

Conversation

mukeshpanchal27
Copy link
Member

In issue #195, it was allowed to use a readme.md file for plugin. However, when someone uses the readme.md file and runs the plugin checks, the plugin incorrectly displays readme.txt as the file name instead of the actual readme.md file.

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@mukeshpanchal27 mukeshpanchal27 marked this pull request as ready for review August 1, 2023 10:50
@joemcgill
Copy link
Member

I'm curious about the initial intent for this check. I think it's still a requirement for a plugin to have a readme.txt file, even if it also includes a readme.md, since the header in the txt version is the one used for parsing out plugin info like version, tested up to, etc.

See: https://developer.wordpress.org/plugins/wordpress-org/how-your-readme-txt-works/#readme-header-information

@felixarntz
Copy link
Member

@joemcgill I thought so too, but someone else (I don't recall, maybe it was @swissspidy or @westonruter?) pointed out that wordpress.org, despite not well documenting it, actually supports using readme.md as well as the actual .org readme file. So if you don't include readme.txt but include readme.md, wordpress.org will try to parse its contents apparently. Of course that only works if you format readme.md correctly for that purpose, not if it's a general readme e.g. for just GitHub.

@westonruter
Copy link
Member

I thought so too, but someone else (I don't recall, maybe it was @swissspidy or @westonruter?) pointed out that wordpress.org

I think it was actually @johnbillion: https://johnblackbourn.com/readme-md-plugin-on-wordpress-org/

For the AMP plugin we initially tried just going with README.md alone in ampproject/amp-wp#5807, but in ampproject/amp-wp#5815 we re-introduced the readme.txt but created it at build time out of the README.md due to various WordPress-specific issues with the format.

@joemcgill
Copy link
Member

That's really interesting, I didn't realize this was supported. Still, I'm curious if the lack of a README.txt file would get flagged in the plugin review process, and if so, is something we should still include as a check in our plugin?

@felixarntz
Copy link
Member

I don't think we need to specifically check for readme.txt, as either works. This is also confirmed by the other plugin checker code, which supports either file extension in the same way: https://github.com/WordPress/plugin-check/blob/trunk/plugin.php#L76

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mukeshpanchal27, a few minor bits of feedback below to make the file paths consistent with other checks (relative to the plugin's directory).

includes/Checker/Checks/Plugin_Readme_Check.php Outdated Show resolved Hide resolved
includes/Checker/Checks/Plugin_Readme_Check.php Outdated Show resolved Hide resolved
includes/Checker/Checks/Plugin_Readme_Check.php Outdated Show resolved Hide resolved
includes/Checker/Checks/Plugin_Readme_Check.php Outdated Show resolved Hide resolved
includes/Checker/Checks/Plugin_Readme_Check.php Outdated Show resolved Hide resolved
includes/Checker/Checks/Plugin_Readme_Check.php Outdated Show resolved Hide resolved
includes/Checker/Checks/Plugin_Readme_Check.php Outdated Show resolved Hide resolved
@mukeshpanchal27
Copy link
Member Author

Thanks all the the review and details. The PR is ready for review.

@mukeshpanchal27
Copy link
Member Author

The Plugin Checker currently checks all files, including in subfolders. However, we should modify the check to only consider files in the root directory for readme.txt and readme.md checks.

WordPress allows and parse readme.txt and readme.md files in root directory of the plugin. Therefore, we will adjust the Plugin Checker accordingly to focus only on root directory files.

For instance, when we checked the WooCommerce plugin, the Plugin Checker returned an array of files as follows:

/var/www/html/wp-content/plugins/woocommerce/Array
(
    [1511] => /var/www/html/wp-content/plugins/woocommerce/packages/action-scheduler/readme.txt
    [1668] => /var/www/html/wp-content/plugins/woocommerce/packages/woocommerce-blocks/assets/js/base/README.MD
    [3531] => /var/www/html/wp-content/plugins/woocommerce/packages/woocommerce-blocks/readme.txt
    [3962] => /var/www/html/wp-content/plugins/woocommerce/readme.txt
)

The specific error highlighted was:

FILE: packages/action-scheduler/readme.txt

Line Column Type Code Message
0 0 WARNING stable_tag_mismatch The Stable Tag in your readme file does not match the version in your main plugin file.

We will update the Plugin Checker to only report errors for readme.txt and readme.md files that are directly in the root directory of the plugin and not in subfolders.

@felixarntz @joemcgill If the above makes sense, then I will open the follow-up issue and start working on it. Thanks.

@felixarntz
Copy link
Member

Thanks @mukeshpanchal27, that's a great catch! +1 to opening a follow-up issue and PR to fix it so that the plugin checker only considers readme files in the root directory.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's open a follow-up PR for the other problem you discovered above. Thank you @mukeshpanchal27!

@mukeshpanchal27 mukeshpanchal27 merged commit 1da4245 into trunk Aug 3, 2023
@mukeshpanchal27 mukeshpanchal27 deleted the fix/readme-error-filename branch August 3, 2023 04:06
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

Successfully merging this pull request may close these issues.

4 participants