Feature/delete update bulk redirects #15
Conversation
Adds two flags to `wp wpcom-legacy-redirector import-from-csv`, namely `—update` and `—delete`. Both accept the same csv format as `—add` (which is now both the default and another allowed flag), but update or delete existing redirects.
… ignore the trailing slash Sometimes we want to match a rule irrespective of whether it comes with a trailing slash. Primarily useful for bulk deletes and updates — all other matching behavior remains the same, and the default is still to strictly match.
| wp_insert_post( $args ); | ||
| switch ( $mode ) { | ||
| case 'update': | ||
| $args[ 'ID' ] = self::get_redirect_post_id( $from_url, false ); |
There was a problem hiding this comment.
I think we can simplify most of this code by just using wp_insert_post and setting $args['ID'].
There was a problem hiding this comment.
Oh yah! I forgot that insert falls back to update if there's an ID arg.
There was a problem hiding this comment.
the self::redirect_post_id() may return 0 which would be ignoring the update request. So perhaps we should check the return value of that call before proceeding with the update procedure, since it would end up in adding new redirect, instead of updating existing one, which may not be the action which has been intended by the user.
| */ | ||
| static function insert_legacy_redirect( $from_url, $redirect_to ) { | ||
| static function insert_legacy_redirect( $from_url, $redirect_to, $mode='add' ) { |
There was a problem hiding this comment.
I think the param might be better as a bool like $update_if_exists (I'm generally not a fan of booleans as parameters but I think it's okay here.)
There was a problem hiding this comment.
Agree. I think I was just being clever and passing the mode in directly.
| @@ -86,7 +87,7 @@ static function insert_legacy_redirect( $from_url, $redirect_to ) { | |||
|
|
|||
| $from_url_hash = self::get_url_hash( $from_url ); | |||
|
|
|||
| if ( false !== self::get_redirect_uri( $from_url ) ) { | |||
| if ( false !== self::get_redirect_uri( $from_url ) && ( 'add' === $mode ) ) { | |||
There was a problem hiding this comment.
Instead of checking for a duplicate via get_redirect_uri, we can use get_redirect_post_id instead and then don't have to do the lookup further down.
$redirect_id = self::get_redirect_post_id( $from_url, false );
if ( $redirect_id && ! $update_if_exists ) {
return new WP_Error( 'duplicate-redirect-uri', 'A redirect for this URI already exists' );
}
There was a problem hiding this comment.
Maybe -- the lookup below is not strict (ie -- doesn't care about trailing slashes) whereas this check has so far been strict about trailing slashes. Separate enhancement?
| return false; | ||
| } | ||
|
|
||
| $post_id = self::get_redirect_post_id( $from_url, false); |
| // if we have a trailing slash, try without. If we don't try with | ||
| if ( substr( $url, -1 ) === '/' ) { | ||
| $url = rtrim( $url, '/' ); | ||
| }else{ |
| $mode = 'update'; | ||
| }elseif ( isset( $assoc_args[ 'delete' ] ) ) { | ||
| $mode = 'delete'; | ||
| }else{ |
|
|
||
| if ( 0 !== $post_id ) { | ||
| wp_delete_post( $post_id, true ); | ||
| }else{ |
|
|
||
| $url_hash = self::get_url_hash( $url ); | ||
|
|
||
| $redirect_post_id = $wpdb->get_var( $wpdb->prepare( "SELECT ID FROM $wpdb->posts WHERE post_type = %s AND post_name = %s LIMIT 1", self::POST_TYPE, $url_hash ) ); |
There was a problem hiding this comment.
Can we just call the function again with strict = true?
| } | ||
|
|
||
| wp_cache_delete( self::get_url_hash( $from_url ), self::CACHE_GROUP ); | ||
| return true; |
There was a problem hiding this comment.
The function does return true anytime it attempted to delete the post and flush cache. But it does not mean the post has been deleted. wp_delete_post returns false on failure. Perhaps we should check whether it succeed or not instead of blindly trusting that it has succeeded?
| break; | ||
| case 'add': | ||
| default: | ||
| wp_insert_post( $args ); |
There was a problem hiding this comment.
similarly to how the code is checking whether the wp_update_post succeeded, we perhaps should check the return value of the wp_insert_post action before returning true.
I know that it hasn't been doing that prior this update, so feel free to leave that for a separate enhancement.
| switch ( $mode ) { | ||
| case 'update': | ||
| $args[ 'ID' ] = self::get_redirect_post_id( $from_url, false ); | ||
| if ( 0 === wp_update_post( $args ) ) { |
There was a problem hiding this comment.
wp_update_post may also return an object of WP_Error class. We should have a check for that case as well.
| wp_insert_post( $args ); | ||
| switch ( $mode ) { | ||
| case 'update': | ||
| $args[ 'ID' ] = self::get_redirect_post_id( $from_url, false ); |
There was a problem hiding this comment.
the self::redirect_post_id() may return 0 which would be ignoring the update request. So perhaps we should check the return value of that call before proceeding with the update procedure, since it would end up in adding new redirect, instead of updating existing one, which may not be the action which has been intended by the user.
Sometimes problems with a large number of existing redirects are detected after they are imported.
This adds the capability to delete or update existing redirects via CLI using an identical csv format to that required for imports. Passing the
import-from-csvCLI command the--updateflag attempts to update existing redirects. Passing the--deleteflag attempts to delete them. When updating or deleting, pattern matching is now non-strict with respect to trailing slash.