add multisite find & replace functionality #481

daithi-coombes · 2013-05-29T20:02:05Z

forked from #278

scribu · 2013-05-29T20:40:21Z

php/commands/search-replace.php

Why not use $wpdb->tables( 'global' )?

daithi-coombes · 2013-05-30T00:15:10Z

@scribu I have tested this by changing a username and blog name, and checked that the text/varchar fields are used from each table. Think this PR maybe ready for a merge or the next state

scribu · 2013-05-30T01:05:11Z

The next step would be to make it mergea-ble into master again (I merged #473 recently).

Update master:

git remote add upstream git://github.com/wp-cli/wp-cli.git
git checkout master
git pull -u upstream

Re-apply changes to the branch (also has the benefit of cleaning up the history):

git checkout multisite-find-replace
git reset master
git commit -a
git push -f

scribu · 2013-05-30T01:15:14Z

Or, the more familiar route, merge master into the branch (and resolve conflicts).

scribu · 2013-05-30T11:20:27Z

You squashed all the commits into one, but the parent commit is still the old one. The latest HEAD in master is 5432c54

See https://github.com/wp-cli/wp-cli/network

daithi-coombes · 2013-05-30T15:59:44Z

@scribu sorry, think I might have merged with master twice after you. Got numerous errors when trying to merge the pr with remote/master, hope its all working (sorry if i've given you extra headache there)

scribu · 2013-05-30T16:16:42Z

Just do this:

git reset remote/master
git ci -am "search-replace function modified to perform a search and replace through multisite specific tables"
git push -f

daithi-coombes · 2013-05-30T16:45:05Z

typo:

git: 'ci' is not a git command.

daithi-coombes · 2013-05-30T16:47:00Z

also from git reset remote/master

git reset remote/master
fatal: ambiguous argument 'remote/master': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

my config:

[core]
        repositoryformatversion = 0
        filemode = true
        bare = false
        logallrefupdates = true
[remote "origin"]
        url = https://github.com/daithi-coombes/wp-cli.git
        fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
        remote = origin
        merge = refs/heads/master
[branch "multisite-find-replace"]
        remote = origin
        merge = refs/heads/multisite-find-replace
[remote "upstream"]
        url = git://github.com/wp-cli/wp-cli.git
        fetch = +refs/heads/*:refs/remotes/upstream/*

scribu · 2013-05-30T16:57:11Z

Right, ci is a git alias of mine.

git fetch upstream
git reset upstream/master
git commit -am "search-replace function modified to perform a search and replace through multisite specific tables"
git push -f

daithi-coombes · 2013-05-30T19:20:48Z

looking at the network chart not sure if that went through right?
https://github.com/wp-cli/wp-cli/network

scribu · 2013-05-30T20:56:00Z

It's good now.

scribu · 2013-05-30T21:01:21Z

php/commands/search-replace.php

Why is the fix here to ignore the first column?

I see it - just above fields is set to array( $primary_key, $col ), so the first column is null. But I worry about the update later on. It seems to me if null is passed for the $where argument, no updates will be made. I think the current strategy requires a primary key.

Seems like it. Two possible solutions:

a) skip tables which don't have a primary key column
b) for tables without a primary key column, assume they don't have serialized values and do a single UPDATE query

In the case of wp_signups, there is a field ('meta') that contains serialized data. Option a) seems safer to me. Maybe instead of zero column counts in the output for that table we could have an output row indicating the table was skipped.

Sounds like a plan. 👍

@daithi-coombes This hasn't been resolved.

scribu · 2013-05-30T21:37:41Z

cc: @cyberhobo

scribu · 2013-06-03T21:10:36Z

Just to clear up any misunderstandings:

@daithi-coombes Do you think you'll have time to continue working on this? Let me know if anything is unclear.

daithi-coombes · 2013-06-04T07:40:46Z

sure I have time. Will start on it imediately

scribu · 2013-06-04T20:22:14Z

Alright. Just shout when you think the code is ready for another review or if you get stuck.

scribu · 2013-06-04T23:15:00Z

features/steps/basic_steps.php

You can just add this as a regular step to the "Given" part of the scenario:

Given a WP multisite install And I run `wp blog --slug="foo" --title="foo" --email="foo@example.com"` And ... When I run `wp search-replace foo bar --multisite`

It's possible because Behat doesn't care if a step is defined with $step->Given() or $step->Then().

You should remove this unused step definition.

scribu · 2013-06-07T15:12:12Z

@scribu is this ready for merge? I'm not sure if my bdd is sufficient.

It will be ready for merge when all the inline comments are gone: https://github.com/wp-cli/wp-cli/pull/481/files

(they disappear once the line of code they reference is removed or changed)

daithi-coombes · 2013-06-08T09:03:39Z

I have added wp_2_options:value to the feature check, and now tables with primary key are skipped and this is reported in the table printout.

One issue is that its failing the CI test (the 3rd ci test) but I can't recreate the failure on my machine

scribu · 2013-06-08T09:47:39Z

One issue is that its failing the CI test (the 3rd ci test) but I can't recreate the failure on my machine

Are you sure that you're running the tests against WP 3.4.2?

You just need to run:

rm -rf /tmp/wp-cli-test-core-download-cache/
wp core download --version=3.4.2 --path=/tmp/wp-cli-test-core-download-cache/

and then run the search-replace tests.

daithi-coombes · 2013-06-08T11:27:38Z

features/search-replace.feature

just noticed... if a user's password hash contains the keyword being replaced then there will be login issues. Is there currently a blacklist of tables:cols?

Nice catch! No blacklist, currently.

daithi-coombes · 2013-06-08T12:11:59Z

ready for review,

if go-ahead, then i'm gonna:

git fetch upstream
git reset upstream/master
git commit -am "merge msg"
git push -f

?

scribu · 2013-06-08T12:21:27Z

php/commands/search-replace.php

This comment is redundant (you could get the same information by just looking at the code); explain the "why", not the "how".

scribu · 2013-06-08T12:35:36Z

It looks good.

The only minor complaint I have left is inconsistent use of whitespace. For example:

isset( $assoc_args['multisite']);

should be:

isset( $assoc_args['multisite'] );

I'll let you guess what the next one should be:

} else{

See http://make.wordpress.org/core/handbook/coding-standards/php/#space-usage

daithi-coombes · 2013-06-08T12:42:23Z

done ;) Cheers for the tips, especially commenting the "why" and not "how", learn something new everyday

scribu · 2013-06-08T12:49:03Z

Alright; please combine all your commits into one again.

I'll merge soon.

scribu · 2013-06-08T14:09:49Z

Merged via #501.

Thanks for getting this off the ground, @daithi-coombes!

scribu reviewed May 29, 2013
View reviewed changes

scribu reviewed May 30, 2013
View reviewed changes

scribu reviewed Jun 4, 2013
View reviewed changes

daithi-coombes reviewed Jun 8, 2013
View reviewed changes

scribu reviewed Jun 8, 2013
View reviewed changes

search-replace command updated for multisite

d162e27

scribu mentioned this pull request Jun 8, 2013

Land multisite support for wp search-replace #501

Merged

scribu closed this Jun 8, 2013

add multisite find & replace functionality #481

add multisite find & replace functionality #481

Uh oh!

Conversation

daithi-coombes commented May 29, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daithi-coombes commented May 30, 2013

Uh oh!

scribu commented May 30, 2013

Uh oh!

scribu commented May 30, 2013

Uh oh!

scribu commented May 30, 2013

Uh oh!

daithi-coombes commented May 30, 2013

Uh oh!

scribu commented May 30, 2013

Uh oh!

daithi-coombes commented May 30, 2013

Uh oh!

daithi-coombes commented May 30, 2013

Uh oh!

scribu commented May 30, 2013

Uh oh!

daithi-coombes commented May 30, 2013

Uh oh!

scribu commented May 30, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scribu commented May 30, 2013

Uh oh!

scribu commented Jun 3, 2013

Uh oh!

daithi-coombes commented Jun 4, 2013

Uh oh!

scribu commented Jun 4, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scribu commented Jun 7, 2013

Uh oh!

daithi-coombes commented Jun 8, 2013

Uh oh!

scribu commented Jun 8, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daithi-coombes commented Jun 8, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scribu commented Jun 8, 2013

Uh oh!

daithi-coombes commented Jun 8, 2013

Uh oh!

scribu commented Jun 8, 2013

Uh oh!

scribu commented Jun 8, 2013

Uh oh!