Skip to content

Feature/delete update bulk redirects #15

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.

Sign up for GitHub

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

Open
mattoperry wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Feature/delete update bulk redirects #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7cd8093
bulk delete or update redirects from a csv via cli
Feb 24, 2017
f5289f2
for getting existing rules, allow an option non-strict mode that will…
Mar 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions includes/wp-cli.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ function import_from_meta( $args, $assoc_args ) {
* redirect_from_path,(redirect_to_post_id|redirect_to_path|redirect_to_url)
*
* @subcommand import-from-csv
* @synopsis --csv=<path-to-csv>
* @synopsis --csv=<path-to-csv> [--update] [--delete]
*/
function import_from_csv( $args, $assoc_args ) {
define( 'WP_IMPORTING', true );
Expand All @@ -92,17 +92,28 @@ function import_from_csv( $args, $assoc_args ) {
WP_CLI::error( "Invalid 'csv' file" );
}

if ( isset( $assoc_args[ 'update' ] ) ) {
$mode = 'update';
}elseif ( isset( $assoc_args[ 'delete' ] ) ) {
$mode = 'delete';
}else{
Copy link
Member

@mjangda mjangda Mar 22, 2017

Choose a reason for hiding this comment

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

Minor: spacing.

$mode = 'add';
}

global $wpdb;
$row = 0;
if ( ( $handle = fopen( $assoc_args['csv'], "r" ) ) !== FALSE ) {
while ( ( $data = fgetcsv( $handle, 2000, "," ) ) !== FALSE ) {
$row++;
$redirect_from = $data[ 0 ];
$redirect_to = $data[ 1 ];
WP_CLI::line( "Adding (CSV) redirect for {$redirect_from} to {$redirect_to}" );
WP_CLI::line( "$mode (CSV) redirect for {$redirect_from} to {$redirect_to}" );
WP_CLI::line( "-- at $row" );
WPCOM_Legacy_Redirector::insert_legacy_redirect( $redirect_from, $redirect_to );

if ( $mode === 'update' || $mode === 'add' ) {
WPCOM_Legacy_Redirector::insert_legacy_redirect( $redirect_from, $redirect_to, $mode );
}else{
WPCOM_Legacy_Redirector::delete_legacy_redirect( $redirect_from );
}
if ( 0 == $row % 100 ) {
if ( function_exists( 'stop_the_insanity' ) )
stop_the_insanity();
Expand Down
76 changes: 66 additions & 10 deletions wpcom-legacy-redirector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,10 @@ static function maybe_do_redirect() {
*
* @param string $from_url URL or path that should be redirected; should have leading slash if path.
* @param int|string $redirect_to The post ID or URL to redirect to.
* @return bool|WP_Error Error if invalid redirect URL specified or if the URI already has a rule; false if not is_admin, true otherwise.
* @param bool $mode add|update if "update", we will update existing redirects for from_url, otherwise we raise an error on an existing redirect
* @return bool|WP_Error Error if invalid redirect URL specified or if the URI already has a rule and we aren't updating or if the update failed; false if something is very wrong (not admin, not CLI), true otherwise.
*/
static function insert_legacy_redirect( $from_url, $redirect_to ) {
static function insert_legacy_redirect( $from_url, $redirect_to, $mode='add' ) {
Copy link
Member

@mjangda mjangda Mar 22, 2017

Choose a reason for hiding this comment

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

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.)

Copy link
Author

@mattoperry mattoperry Mar 22, 2017

Choose a reason for hiding this comment

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

Agree. I think I was just being clever and passing the mode in directly.


if ( ! ( defined( 'WP_CLI' ) && WP_CLI ) && ! is_admin() && ! apply_filters( 'wpcom_legacy_redirector_allow_insert', false ) ) {
// never run on the front end
Expand All @@ -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 ) ) {
Copy link
Member

@mjangda mjangda Mar 22, 2017

Choose a reason for hiding this comment

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

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' );
}

Copy link
Author

@mattoperry mattoperry Mar 22, 2017

Choose a reason for hiding this comment

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

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 new WP_Error( 'duplicate-redirect-uri', 'A redirect for this URI already exists' );
}

Expand All @@ -104,13 +105,58 @@ static function insert_legacy_redirect( $from_url, $redirect_to ) {
return new WP_Error( 'invalid-redirect-url', 'Invalid redirect_to param; should be a post_id or a URL' );
}

wp_insert_post( $args );
switch ( $mode ) {
case 'update':
$args[ 'ID' ] = self::get_redirect_post_id( $from_url, false );
Copy link
Member

@mjangda mjangda Mar 22, 2017

Choose a reason for hiding this comment

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

I think we can simplify most of this code by just using wp_insert_post and setting $args['ID'].

Copy link
Author

@mattoperry mattoperry Mar 22, 2017

Choose a reason for hiding this comment

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

Oh yah! I forgot that insert falls back to update if there's an ID arg.

Copy link

@david-binda david-binda Mar 22, 2017

Choose a reason for hiding this comment

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

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.

if ( 0 === wp_update_post( $args ) ) {
Copy link

@david-binda david-binda Mar 22, 2017

Choose a reason for hiding this comment

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

wp_update_post may also return an object of WP_Error class. We should have a check for that case as well.

return new WP_Error( 'failed-redirect-update', 'Redirect update failed for unknown reason' );
}
break;
case 'add':
default:
wp_insert_post( $args );
Copy link

@david-binda david-binda Mar 22, 2017

Choose a reason for hiding this comment

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

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.

break;
}

wp_cache_delete( $from_url_hash, self::CACHE_GROUP );

return true;
}

/**
* @param string $from_url URL or path for the redirect we'd like to delete.
* @return bool|WP_Error WP_Error if something is amiss with the delete; true otherwise.
*/

static function delete_legacy_redirect( $from_url ) {
if ( ! ( defined( 'WP_CLI' ) && WP_CLI ) && ! is_admin() ) {
// never run on the front end
return false;
}

$from_url = self::normalise_url( $from_url );

if ( is_wp_error( $from_url ) ) {
return false;
}

$post_id = self::get_redirect_post_id( $from_url, false);
Copy link
Member

@mjangda mjangda Mar 22, 2017

Choose a reason for hiding this comment

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

Minor: spacing.


//sanity check that we are deleting only the correct post_type
if ( get_post_type( $post_id ) !== self::POST_TYPE ) {
return new WP_Error( 'failed-redirect-delete', 'Redirect delete attempted to delete a post of incorrect type.' );
}

if ( 0 !== $post_id ) {
wp_delete_post( $post_id, true );
}else{
Copy link
Member

@mjangda mjangda Mar 22, 2017

Choose a reason for hiding this comment

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

Minor: spacing.

return false;
}

wp_cache_delete( self::get_url_hash( $from_url ), self::CACHE_GROUP );
return true;
Copy link

@david-binda david-binda Mar 22, 2017

Choose a reason for hiding this comment

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

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?

}

static function get_redirect_uri( $url ) {

$url = self::normalise_url( $url );
Expand Down Expand Up @@ -144,23 +190,33 @@ static function get_redirect_uri( $url ) {
return false;
}

static function get_redirect_post_id( $url ) {
static function get_redirect_post_id( $url, $strict = true ) {
global $wpdb;

$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 ) );

if ( ! $redirect_post_id )
if ( !$redirect_post_id && !$strict ) {
// if we have a trailing slash, try without. If we don't try with
if ( substr( $url, -1 ) === '/' ) {
$url = rtrim( $url, '/' );
}else{
Copy link
Member

@mjangda mjangda Mar 22, 2017

Choose a reason for hiding this comment

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

Minor: spacing.

$url = $url . '/';
}

$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 ) );
Copy link
Member

@mjangda mjangda Mar 22, 2017

Choose a reason for hiding this comment

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

Can we just call the function again with strict = true?

Copy link
Author

@mattoperry mattoperry Mar 22, 2017

Choose a reason for hiding this comment

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

Yup that's cleaner.

}

if ( !$redirect_post_id ) {
$redirect_post_id = 0;
}

return $redirect_post_id;
}

private static function get_url_hash( $url ) {
return md5( $url );
}

/**
* Takes a request URL and "normalises" it, stripping common elements
*
Expand Down