WIP: Allow some variants of filenames in required_files_checks #129
Conversation
Merge to master
Swap to defining filenames and valid extensions in an array and itterating through during testing. Also add a workaround to catch all uppercase filenames.
|
I'll also let @timelsass look at this, since he wrote it 😄 delegating... Also can you just change the branch to |
Fixed the type prefiex to prefix
…quired_files_compatibility Issue/130/screenshot check required files compatibility
…into feature/readme-validate
Fixed the typo prefiex to prefixes Refrence #133
Update download link in README
add json and yaml definitions
Fix/provide extensions
Added a template for code of conduct text.
Added the bug report issue template, for easier support handling.
Create issue template - bug report
Feature/validate readme
closes #146, fixes #155
Included the addtion from the GB repo.
|
@pattonwebz There were some merges to the development branch. Can you update your code and fix the conflicts? Thanks! |
Create CODE_OF_CONDUCT.md
|
Yup will refresh it today :) |
dingo-d
added
the
Status: Future Release
|
I think with reworking the screenshot and readme checks the required files check won't be necessary anymore, and we can handle the file missing errors inside of those classes - you may want to hold off on this for a little bit. The start of the readme validation stuff is in the development branch though if you wanted to look at that one. I'm still not entirely sure if the sniff should allow .md for the reasons described on #128 (comment). As a note, the parser/validator on wp.org is expecting .txt when validating via URL: https://meta.trac.wordpress.org/browser/sites/trunk/wordpress.org/public_html/wp-content/plugins/plugin-directory/readme/class-validator.php#L33. Themes never had a requirement as to format when it comes to a readme.txt, so I don't see a reason to allow multiple extensions. I would even argue that having a stricter format would be a better choice than adapting to old methods which are filled with their fair share of underlying issues. The Gutenberg block RFC is a good example of a better way to handle this type of information: https://github.com/WordPress/gutenberg/blob/97e878c57266071b4c4de460dabbf3471055bd21/docs/rfc/block-registration.md That information is far more accessible, easier to parse, and pretty much every programming language has a lib to consume it easily if not already built in. However if the desire is to achieve closer parity between plugins and themes for some reason, then having the format as just |
|
@tim if you think these files could be checked for inside of alternative sniffs should the check this PR modifies just be removed entirely? My concern here is that I want to stop users from having so many false positives on the required files check. I'd like to make that happen sooner rather than later. Readme can be either |
Swap to defining filenames and valid extensions in an array and itterating through during testing. Also add a workaround to catch all uppercase filenames.
…z/theme-sniffer into issue/128/required_files_check
The Required page says readme.txt file. There is nothing about readme.md. It should be consistent with plugins, and if core will be reading it (as we intend), only one name should be allowed. |
|
In the past we have allowed people to use .md if they wanted. In the interests of getting something out there for a fix I am allowing it. If it is not allowable in future that's ok - but for now we can allow it and prevent users seeing an |
|
I think that's a dangerous way to go. It should match the requirements page and also match what core is checking for PHP version, and match what plugins allows. |
I'm not saying I disagree with that. I just don't want to have to wait until that is worked through before this check stops making people think there is a major blocker when it's only minor and easily resolvable :) |
|
I updated this PR to fix conflicts. It does have certain issues with the other updated readme checks that will need worked though if this is the method that we go with here if nothing else more appropriate can be worked out quickly. Flagging as a WIP for the moment. |
pattonwebz
changed the title
|
i merged in @pattonwebz commits and started a quick refactor in PR #160. The PR does add a few extras in as well - the aspect ratio check from theme-check on screenshot, the extra extensions checks, and case-insensitive file exists checks. It should be easier to manage and tell what's going on in the refactor - or at least I hope it is 😄 It's still not 100%, but I think it's bringing things together better. It does remove the required files check mentioned above, and brings the screenshot sniffs into it's own validator class like the readme sniffs were. These changes should resolve the issues @pattonwebz mentioned above about the updated readme checks collisions. I started to remove some of the logic for errors/warnings/messages in these, and have a more generic implementation for other sniffs to use in the future which is a Results class. Both the readme and screenshot sniffs are using it now. The Results class is designed for a single file that has a Validator class with a call to get_results() (these implement Has_Results), which returns an array of messages, and then it will format those the expected way for merging into the final report response that gets output. There's now a Validate_File class, which both readme/screenshot sniffs use - this is where the logic for required files was abstracted out to. Following @pattonwebz suggestion of a case-insensitive search in directory seemed like a good way to go for the issues with file_exists, so there's a helper/trait that can be used where needed to do that. The Validate_File class makes use of this to resolve README.txt vs readme.txt (a test case for this is twentyseventeen since it uses all caps for it's readme file). For all the extensions stuff, no ones really gave a clear answer, so I just have screenshot checking png, jpg, case-insensitive, and readme checking txt, md, case-insensitive. The filename and extensions are just class properties, so easy to spot and change if this needs to be changed. For the case insensitive stuff - as explained above is the File_Helpers trait, so the logic there can be changed to check only lower + only upper instead of case-insensitive if needed. I've not tested all of this out, so i'd say it's still WIP, and any help in testing is appreciated if anyone has some spare time |
|
@timelsass I will try test out your work on Monday for #160 . I appreciate you bearing with me here on my shifting targets for allowable types and extensions :). I'm aware that it could well be a slippery slope to go down making such decisions on the spot (for a very short term workaround on the issue) but the amount of uses that seek help with the old required file checks is out of proportion with the severity of what the check was reporting. Keeping this PR open for the moment but I expect #160 will be a better solution. |
|
The readme.txt should be validated and required, as it is going to be important to have a valid readme if were to make a theme main page look like plugins one. Btw, any update on this PR? |
Partial work towards solution for #128. This allows a .md or .txt for readme and a .jpg and .png for screenshot. Also allows uppercase
README.*variants.The
file_exists()function is case sensitive on some filesystems and not others so it's not reliable to check variants with different cases. We probably need to explore another solution for this but the only other way I can think of right now would be to list entire directories to do case insensitive string matches. Seems overly messy.