Fix readme file issue in Windows #435
Conversation
|
@ernilambar can we make sure to add a Changelog here? |
There was a problem hiding this comment.
@ernilambar Thank you for the PR!
While this PR addresses the concrete problem you're facing, I wonder whether we should take a step back to find a more holistic solution to the problem.
Paths being different on Windows can affect any checks, and I wonder whether we should cater for that in a more central location, rather than having to adjust any such checks.
WordPress provides a wp_normalize_path() function which takes care of normalizing paths so that they always use /.
Maybe we should use this function in the Abstract_File_Check::get_files() method (and potentially other methods in that class) so that all paths are always normalized? This way we can keep using the common / everywhere, no need for workarounds like this one.
Curious to hear other people's thoughts. cc @swissspidy @mukeshpanchal27
| @@ -27,12 +27,15 @@ protected function filter_files_for_readme( array $files, $plugin_relative_path | |||
| // Find the readme file. | |||
| $readme_list = preg_grep( '/readme\.(txt|md)$/i', $files ); | |||
|
|
|||
| $plugin_relative_path = rtrim( $plugin_relative_path, '/' ); | |||
There was a problem hiding this comment.
Why is this needed? In which situation would $plugin_relative_path end in a forward slash?
| // Filter the readme files located at root. | ||
| $potential_readme_files = array_filter( | ||
| $readme_list, | ||
| function ( $file ) use ( $plugin_relative_path ) { | ||
| $file = str_replace( $plugin_relative_path, '', $file ); | ||
| return ! str_contains( $file, '/' ); | ||
| $file = ltrim( $file, '/\\' ); | ||
| return ! ( str_contains( $file, '/' ) || str_contains( $file, '\\' ) ); |
There was a problem hiding this comment.
Could be simplified to:
| return ! ( str_contains( $file, '/' ) || str_contains( $file, '\\' ) ); | |
| return ! str_contains( $file, '/' ) && ! str_contains( $file, '\\' ); |
|
Closing. |
Fixes #434