Skip to content

Add --include-root parameter to also verify root directory #102

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.

Sign up for GitHub

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

Jump to bottom
Merged
danielbachhuber merged 19 commits into from
Mar 24, 2023
Merged

Add --include-root parameter to also verify root directory #102

danielbachhuber merged 19 commits into from
Mar 24, 2023

Conversation

@paulschreiber
Copy link
Contributor

@paulschreiber paulschreiber commented Mar 22, 2023 •
edited
Loading

Description

Add --include-root parameter. Allows warning for unexpected files in ABSPATH.

Related Issues

Fixes #81, Fixes #74.

@paulschreiber
Add --include-root parameter
4df6e69 
Allows warning for unexpected files in ABSPATH.
@paulschreiber paulschreiber requested a review from a team as a code owner March 22, 2023 02:22
danielbachhuber
danielbachhuber requested changes Mar 22, 2023
View reviewed changes
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request!

Overall, this is headed in the right direction. Can you add some functional tests covering the change? Here is some guidance on our pull request best practices, if it's helpful.

@danielbachhuber danielbachhuber changed the title Add --include-root parameter Add --include-root parameter to also verify root directory Mar 22, 2023
@danielbachhuber danielbachhuber mentioned this pull request Mar 22, 2023
Closed
1 task
@paulschreiber
Copy link
Contributor Author

paulschreiber commented Mar 22, 2023

@danielbachhuber I added a test to features/checksum-core.feature. Is that what you wanted? If not, please be more explicit as to the location and nature of tests required.

@danielbachhuber danielbachhuber self-requested a review March 22, 2023 13:36
danielbachhuber
danielbachhuber reviewed Mar 22, 2023
View reviewed changes
Copy link
Member

@danielbachhuber danielbachhuber left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I added a test to features/checksum-core.feature. Is that what you wanted?

Yep, that's headed in the right direction.

It seems to flag plugins too. Is that expected?

$ wp core verify-checksums --include-root
Warning: File should not exist: wp-config.php
Warning: File should not exist: vanilla.wordpress.2023-01-24.000.xml
Warning: File should not exist: update.php
Warning: File should not exist: wp-cli.yml
Warning: File should not exist: white-150-square.jpg
Warning: File should not exist: mysitetitle.wordpress.2022-12-05.004.xml
Warning: File should not exist: vanilla.wordpress.2022-09-11.000.xml
Warning: File should not exist: test.jpg
Warning: File should not exist: mysitetitle.wordpress.2022-12-05.000.xml
Warning: File should not exist: wp-content/plugins/wordpress-importer/parsers/class-wxr-parser-regex.php
Warning: File should not exist: wp-content/plugins/wordpress-importer/parsers/class-wxr-parser.php
Warning: File should not exist: wp-content/plugins/wordpress-importer/parsers/class-wxr-parser-simplexml.php

We probably should allow wp-config.php.

Also, I didn't debug this, but wp-cli.yml is flagged for some reason without the flag included:

$ wp core verify-checksums
Warning: File should not exist: wp-cli.yml
Success: WordPress installation verifies against checksums.

@paulschreiber
Copy link
Contributor Author

paulschreiber commented Mar 22, 2023

It seems to flag plugins too. Is that expected?

Nope.

We probably should allow wp-config.php.

Yes.

I fxied the above.

  • What is wp-cli.yml used for?
  • How do you run the tests manually?

@chesio
Copy link

chesio commented Mar 22, 2023

What is wp-cli.yml used for?

@paulschreiber, see: https://make.wordpress.org/cli/handbook/references/config/#config-files

@paulschreiber
Copy link
Contributor Author

paulschreiber commented Mar 22, 2023

@chesio Would this file present on production deployments of WordPress?

@paulschreiber
Copy link
Contributor Author

paulschreiber commented Mar 23, 2023

wp-cli.ymlis flagged by the existing ^wp-(?!config\.php)([^\/]*)$ regex:
image

@danielbachhuber
Copy link
Member

danielbachhuber commented Mar 23, 2023

wp-cli.ymlis flagged by the existing ^wp-(?!config\.php)([^\/]*)$ regex:

@paulschreiber Can you expand your tests to cover the expected behavior for wp-cli.yml, as well as 31f15fc?

danielbachhuber
danielbachhuber reviewed Mar 23, 2023
View reviewed changes
danielbachhuber
danielbachhuber reviewed Mar 23, 2023
View reviewed changes
@paulschreiber
Copy link
Contributor Author

paulschreiber commented Mar 23, 2023

@danielbachhuber I've made changes you requested. I am not sure what tests you wanted for 31f15fc, but added one for wp-cli.yml and added you as a collaborator to the repo.

danielbachhuber
danielbachhuber reviewed Mar 23, 2023
View reviewed changes
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

I am not sure what tests you wanted for 31f15fc

Specifically, to make sure there are some non-core files in wp-content and that it doesn't warn on them. We implicitly should have akismet installed, but it would be helpful to include an assertion that: 1) the directory exists, and 2) it's not included in warning output.

@paulschreiber
Copy link
Contributor Author

paulschreiber commented Mar 23, 2023

Specifically, to make sure there are some non-core files in wp-content and that it doesn't warn on them. We implicitly should have akismet installed, but it would be helpful to include an assertion that: 1) the directory exists, and 2) it's not included in warning output.

How do I assert the directory exists? I'm not familiar with this testing framework. Can you make that change?

Also: is there a reason some Scenario lines have an extra indent?

@danielbachhuber danielbachhuber added this to the 2.2.0 milestone Mar 24, 2023
@danielbachhuber
Copy link
Member

danielbachhuber commented Mar 24, 2023

How do I assert the directory exists? I'm not familiar with this testing framework. Can you make that change?

Yep, I improved upon the tests with d755af7

Also: is there a reason some Scenario lines have an extra indent?

I'm not sure why that happened. I removed with 63fd9e7

Thanks for your work on this, @paulschreiber ! I'll merge once the tests pass.

@danielbachhuber
Copy link
Member

danielbachhuber commented Mar 24, 2023

Hm, not quite sure why this failure is happening:

image

main passed just fine a moment ago https://github.com/wp-cli/checksum-command/actions/runs/4513959747

@danielbachhuber
Copy link
Member

danielbachhuber commented Mar 24, 2023

Hm, not quite sure why this failure is happening:

Figured it out dc03bc8

@danielbachhuber danielbachhuber self-requested a review March 24, 2023 20:50
@danielbachhuber danielbachhuber merged commit 1a44dfb into wp-cli:main Mar 24, 2023
@paulschreiber paulschreiber deleted the patch-1 branch March 25, 2023 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danielbachhuber danielbachhuber danielbachhuber approved these changes

Assignees

No one assigned

Labels

command:core-verify-checksums

Projects

None yet

Milestone

2.2.0

Development

Successfully merging this pull request may close these issues.

Core verify-checksums command doesn't catch added files at ABSPATH directory Flag presence of unexpected files

3 participants

@paulschreiber @chesio @danielbachhuber