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

Feature: Call user function with --callback flag #104

Closed
wants to merge 7,431 commits into from

Conversation

brandonkal
Copy link

This PR adds the ability to run a user function on search results if a match is found.
In my case, I needed this to change image src urls based on attachment id and size where a hash is a required part of the url.

danielbachhuber and others added 30 commits March 10, 2017 08:39
Abstract `wp export` to wp-cli/export-command
```
Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 0 installs, 14 updates, 0 removals
  - Updating symfony/yaml (v2.8.17 => v2.8.18): Loading from cache
  - Updating symfony/filesystem (v2.8.17 => v2.8.18): Loading from cache
  - Updating symfony/config (v2.8.17 => v2.8.18): Loading from cache
  - Updating symfony/debug (v2.8.17 => v2.8.18): Loading from cache
  - Updating symfony/dependency-injection (v2.8.17 => v2.8.18): Loading from cache
  - Updating symfony/event-dispatcher (v2.8.17 => v2.8.18): Loading from cache
  - Updating symfony/translation (v2.8.17 => v2.8.18): Loading from cache
  - Updating symfony/process (v2.8.17 => v2.8.18): Loading from cache
  - Updating symfony/finder (v2.8.17 => v2.8.18): Loading from cache
  - Updating symfony/console (v2.8.17 => v2.8.18): Loading from cache
  - Updating seld/jsonlint (1.5.0 => 1.6.0): Loading from cache
  - Updating justinrainbow/json-schema (4.1.0 => 5.1.0): Loading from cache
  - Updating composer/ca-bundle (1.0.6 => 1.0.7): Loading from cache
  - Updating composer/composer (1.3.2 => 1.4.1): Loading from cache
Writing lock file
Generating autoload files
```
Update Composer dependencies (3/10/2017)
Transform sub/sub directories into a proper dbprefix
Abstract `wp package *` to a separate wp-cli/package-command package
Doing so ensures tests run against local copy of command, instead of
Phar bundled version.
Use Composer to install WP-CLI in tests
Include `"prefer-stable": true,` to always prefer stability
Require any WP-CLI, but install dev-master for tests
To allow for versions like 1.2.0 as well, the branch alias for `dev-master` needs to be changed from `1.0.x-dev` to `1.x-dev`.
Fix Travis CI caching for composer
@brandonkal
Copy link
Author

I am hoping someone is willing and able to test this out and release it in or let me know what needs to change. I am glad the CI tests are passing now. I am not able to test this locally because it appears to have conflicts with the search-replace that is installed by default.

So I am seeing this even though I am passing 9 args in the new version:

Uncaught ArgumentCountError: Too few arguments to function WP_CLI\SearchReplacer::__construct(),
8 passed in phar:///usr/local/bin/wp/vendor/wp-cli/search-replace-command/src/Search_Replace_Command.php on line 489
and exactly 9 expected in
/home/brandonkal/.wp-cli/packages/vendor/wp-cli/search-replace-command/src/WP_CLI/SearchReplacer.php:31

I installed my fork as explained in the README:

wp package install [email protected]:brandonkal/search-replace-command.git

Thanks

@swissspidy
Copy link
Member

Try updating to WP-CLI nightly

@brandonkal
Copy link
Author

Installing the nightly version fixed the issue. Everything looks good so far but I will report back after some more thorough testing.

@brandonkal
Copy link
Author

Tested thoroughly and everything looks good to go!

.editorconfig Outdated Show resolved Hide resolved
src/Search_Replace_Command.php Outdated Show resolved Hide resolved
src/WP_CLI/SearchReplacer.php Outdated Show resolved Hide resolved
src/WP_CLI/SearchReplacer.php Outdated Show resolved Hide resolved
src/WP_CLI/SearchReplacer.php Outdated Show resolved Hide resolved
src/WP_CLI/SearchReplacer.php Outdated Show resolved Hide resolved
src/WP_CLI/SearchReplacer.php Outdated Show resolved Hide resolved
src/WP_CLI/SearchReplacer.php Outdated Show resolved Hide resolved
@schlessera
Copy link
Member

@brandonkal The PR looks good at first glance. We still need to add functional tests through Behat. Are you able to add these?

@schlessera
Copy link
Member

@brandonkal Are you interested in providing the required Behat tests to move this over the finish line?

@schlessera
Copy link
Member

@brandonkal Gentle ping to see if you are up for adding the needed tests...?

@brandonkal
Copy link
Author

Thank you for the reminder @schlessera. I forgot about this. I am not very familiar with Behat and my WordPress projects are currently on hold so I expect it will be a few months before I can look into this again.
If anyone is willing to add the tests in the meantime, I've allowed edits on this PR.

$check = true; // Set to avoid bogus error on strpos

if ( $this->callback ) {
if ( $check = strpos( $data, $this->from ) !== false ) {
Copy link

Choose a reason for hiding this comment

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

Given that we are if ( $this->regex ) { , what's the purpose of this strpos() check? Shoudn't we just proceed with call_user_func ?


if ( $this->callback ) {
if ( $check = strpos( $data, $this->from ) !== false ) {
$result = \call_user_func( $this->callback, $data, $this->to, $search_regex );
Copy link

Choose a reason for hiding this comment

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

Would passing $this->form (or $this) makes more sense thant $search_regex

  1. Because callback may need raw $from
  2. Because callback does replace which may rely on regex_limit / flags individually

@danielbachhuber
Copy link
Member

@wp-cli/committers There might be a much easier approach here: call apply_filters() if it exists. What do you think?

@drzraf
Copy link

drzraf commented Jul 11, 2022

I implemented callback support within #128
It relies upon callback's name being passed as an option, rely upon call_user_func and should easily made a filter.

From a quick look, a filter sounds better because:

  • Multiple callbacks can be run
  • It allows plugin's own code to rely upon this very filter to implement "built-in / handy / predefined callbacks"
  • It avoids passing callback's name down the function stack
  • It does not require the user to write more boilerplate code to implement such a callback than to define a top-level function.

@danielbachhuber
Copy link
Member

Proceeding with wp-cli/wp-cli#5594 for this repository. I've captured this PR to https://gist.github.com/danielbachhuber/f4bb0f6caaebc2c8a6390adc19cd1f50 in case this PR is auto-closed or broken in some way.

@danielbachhuber danielbachhuber requested a review from a team as a code owner November 18, 2022 17:08
drzraf added a commit to drzraf/search-replace-command that referenced this pull request Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.