Conversation
Fix a bug where excluded directories were not detected because only the filename was checked, not the full path. Add __MACOSX to the list of excluded directories. Update error message.
There was a problem hiding this comment.
Pull request overview
Updates the Theme Check “Directories” check to correctly detect disallowed directories by inspecting full file paths (not just basenames), and adds __MACOSX to the disallowed directory list to help catch macOS ZIP artifacts.
Changes:
- Detect disallowed directories by checking path segments across full file paths.
- Add
__MACOSXto the excluded/disallowed directories list. - Update the required-message text shown when an excluded directory is found.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| '<span class="tc-lead tc-required">%s</span>: %s', | ||
| __( 'REQUIRED', 'theme-check' ), | ||
| __( 'Please remove any extraneous directories like .git or .svn from the ZIP file before uploading it.', 'theme-check' ) | ||
| __( 'Please remove any extraneous directories like <strong>.git</strong>, <strong>.svn</strong> or <strong>__MACOSX</strong> from the ZIP file before uploading it.', 'theme-check' ) |
There was a problem hiding this comment.
The error message hard-codes examples (".git", ".svn", "__MACOSX"), but the actual disallowed list also includes ".hg" and ".bzr". This can become misleading/out-of-sync if the list changes. Consider generating the displayed directory list from $excluded_directories (or wording the message generically without enumerating specific names).
| $path_segments = explode( '/', str_replace( '\\', '/', $path ) ); | ||
|
|
||
| if ( in_array( $filename, $excluded_directories, true ) ) { | ||
| if ( array_intersect( $path_segments, $excluded_directories ) ) { |
There was a problem hiding this comment.
array_intersect() returns an array; relying on its truthiness is a bit implicit. For readability, consider checking it explicitly (e.g., assign to a variable and/or use ! empty(...)) so it’s clear the condition is “any disallowed segment matched”.
| if ( array_intersect( $path_segments, $excluded_directories ) ) { | |
| $matched_directories = array_intersect( $path_segments, $excluded_directories ); | |
| if ( ! empty( $matched_directories ) ) { |
Closes #403
Replaces #414
Fixes a bug where disallowed directories were not detected because only the filename was checked, not the full path.
Adds __MACOSX to the list of directories.
Updates the error message.
Does not detect empty folders, it seems unlikely that empty folders would be included.
It is kind of backwards to check for files and then check what folder they are in... But we already have the list of files.
This is only one way to solve this, but it is the smallest change. I don't feel strongly for or against this versus a directory scan.
Testing Instructions
You need at least two themes installed.
Try some of the other folder names as well.
AI Usage: I argued back and forward with Copilot.