Skip to content
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.

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

Load config-spec.php in a "filterable" way #5664

Merged
merged 7 commits into from Aug 25, 2022
Merged

Load config-spec.php in a "filterable" way #5664

merged 7 commits into from Aug 25, 2022

Conversation

danielbachhuber
Copy link
Member

@danielbachhuber danielbachhuber commented Aug 18, 2022 •
edited

  • Moves config-spec.php loading to a standalone method.
  • Calls WP_CLI_CONFIG_SPEC_FILTER_CALLBACK against $config_spec if callback is defined.
  • Introduces WP_CLI_EARLY_REQUIRE environment variable for early loading of a PHP file.

Fixes #5649

@danielbachhuber danielbachhuber added this to the 2.7.0 milestone Aug 18, 2022
@danielbachhuber danielbachhuber requested a review from a team as a code owner Aug 18, 2022
@danielbachhuber danielbachhuber self-assigned this Aug 18, 2022
schlessera
schlessera requested changes Aug 22, 2022
View changes
Copy link
Member

@schlessera schlessera left a comment

Here's my suggestion. Adding a definable INCLUDE seems overly permissive. A simple filter callback will be enough, and it cannot be completely abused.

php/WP_CLI/Configurator.php Outdated Show resolved Hide resolved
php/WP_CLI/Configurator.php Outdated Show resolved Hide resolved
danielbachhuber and others added 3 commits Aug 22, 2022
@danielbachhuber @schlessera
Switch to a defined callback.
f822468 
Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>
@danielbachhuber
Fix function name
c1b030e
@danielbachhuber @schlessera
Don't call this method statically
f1a8410 
Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>
@danielbachhuber danielbachhuber requested a review from schlessera Aug 22, 2022
schlessera
schlessera requested changes Aug 25, 2022
View changes
Copy link
Member

@schlessera schlessera left a comment

This latest approach looks fine to me. I'm not super happy about a general require, but I understand its value and I cannot think of an immediate attack vector right now that wouldn't be doable in some other already existing entrypoint anyways.

Now, how about adding tests for this?

@danielbachhuber danielbachhuber requested a review from schlessera Aug 25, 2022
@danielbachhuber
Copy link
Member Author

danielbachhuber commented Aug 25, 2022

Now, how about adding tests for this?

@schlessera Added with 4073a34

@danielbachhuber
Copy link
Member Author

danielbachhuber commented Aug 25, 2022

🎉

Thanks for your help with this, @schlessera ! Appreciate it 😊

@danielbachhuber danielbachhuber merged commit a533612 into master Aug 25, 2022
27 checks passed
@danielbachhuber danielbachhuber deleted the load-config-spec-filterable branch Aug 25, 2022
@schlessera
Copy link
Member

schlessera commented Oct 12, 2022

@danielbachhuber I now noticed that the test seems to be in the wrong file. It is in the config.feature file (which deals with the wp-cli.yml et al.), but it should probably rather be in the flags.feature file.

@schlessera schlessera mentioned this pull request Oct 12, 2022
Closed
@danielbachhuber
Copy link
Member Author

danielbachhuber commented Oct 12, 2022

@schlessera Sorry about that! PR to fix with #5687

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schlessera schlessera

Assignees

@danielbachhuber danielbachhuber

Labels
scope:framework
Projects
None yet
Milestone
2.7.0
Development

Successfully merging this pull request may close these issues.

Allow registration of custom global arguments
2 participants
@danielbachhuber @schlessera