Skip to content

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

Merged
danielbachhuber merged 7 commits intomasterfrom
load-config-spec-filterable
Aug 25, 2022
Merged

Load config-spec.php in a "filterable" way#5664
danielbachhuber merged 7 commits intomasterfrom
load-config-spec-filterable

Conversation

@danielbachhuber
Copy link
Member

@danielbachhuber danielbachhuber commented Aug 18, 2022 •
edited
Loading

  • 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 August 18, 2022 20:19
@danielbachhuber danielbachhuber self-assigned this Aug 18, 2022
schlessera
schlessera requested changes Aug 22, 2022
View reviewed changes
Copy link
Member

@schlessera schlessera left a comment

Choose a reason for hiding this comment

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

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

danielbachhuber and others added 3 commits August 22, 2022 12:12
@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>
schlessera
schlessera requested changes Aug 25, 2022
View reviewed changes
Copy link
Member

@schlessera schlessera left a comment

Choose a reason for hiding this comment

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

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
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
@danielbachhuber danielbachhuber deleted the load-config-spec-filterable branch August 25, 2022 20:58
@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

@danielbachhuber danielbachhuber mentioned this pull request Nov 10, 2022
Closed
This was referenced Nov 26, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@schlessera schlessera schlessera approved these changes

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