Skip to content

Add --exclude argument for verifying checksums#124

Closed
ethicaladitya wants to merge 1 commit intowp-cli:mainfrom
ethicaladitya:gh-64-add-exclude-argument
Closed

Add --exclude argument for verifying checksums#124
ethicaladitya wants to merge 1 commit intowp-cli:mainfrom
ethicaladitya:gh-64-add-exclude-argument

Conversation

@ethicaladitya
Copy link

Added argument "--excluded" to the wp core verify-checksums to rule out any false positives when we have extra files in the core.

Related:
#64
wp-cli/wp-cli#5955

@ethicaladitya ethicaladitya requested a review from a team as a code owner June 13, 2024 14:48
@mrsdizzie
Copy link
Member

mrsdizzie commented Jun 13, 2024

I think this would need something more specific than strpos because that is for matching substrings and these should be exact matches. For example, if you exclude config.php it should ignore a top level config.php but still flag wp-admin/config.php as an unknown file. There should be another behat test to make sure it doesn't do that.

It also mentions excluding directories, but I'm not sure that is necessary since the command already ignores non-core directories both at the top level and within core folders as far as I can see. So it shouldn't be flagging anything inside of an unknown directory already. If there is a specific example of it doing that, or why that would be needed, there should be a test for that as well.

Edit: now I see a similar PR with that approach was already opened around the same time as this: #123

This one should probably be closed in favor of that

@swissspidy swissspidy changed the title gh-64 added exclude argument in checksums Add --exclude argument for verifying checksums Aug 7, 2024
@swissspidy swissspidy added command:core-verify-checksums Related to 'core verify-checksums' command command:plugin-verify-checksums Related to 'plugin verify-checksums' command and removed command:plugin-verify-checksums Related to 'plugin verify-checksums' command labels Aug 7, 2024
*/
private function is_excluded( $file ) {
foreach ( $this->exclude_paths as $exclude_path ) {
if ( strpos( $file, $exclude_path ) !== false ) {
Copy link
Member

Choose a reason for hiding this comment

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

This is too broad IMHO. It means I can do --exclude=a and every file with the letter a in it will be excluded. I don't think that's desired.

We should make this a strict comparison, where the path has to match exactly. Could be a simple in_array() check.

"""
And STDERR should be empty

Scenario: Verify core checksums with excluded file
Copy link
Member

Choose a reason for hiding this comment

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

An additional test with a file in a subfolder would be beneficial, to verify that works too.

@swissspidy
Copy link
Member

Actually, it looks like #123 covers both my points already, so I am closing this PR in favor of it.

@swissspidy swissspidy closed this Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

command:core-verify-checksums Related to 'core verify-checksums' command

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants