fix: Add a new filter for custom redirect validations

[diksha-nts]

Description: We apply here a filter for custom redirect validation. Filter Name: apply_filters( 'validate_vip_legacy_redirect_loop', $output, $redirect_from, $redirect_to );

[diksha-nts]

@rbcorrales Thanks for the solution you provided me and I tried implementing the same which works for me.

But the validate function only returns bool value which in turn will be true or false but I need my custom error messages instead of a common message which will be displayed from this condition.

To make it custom, is it okay if I add my filter inside validate_urls function which will return WP_Error too. Like this:

public static function validate_urls( $from_url, $redirect_to ) {
    // Ongoing conditions.

   return apply_filters( 'wpcom_legacy_redirector_validate', array( $from_url, $redirect_to ) );
}

[rbcorrales]

@diksha-nts Thanks for sharing these details. I see, so you would like to return an error object so it's shown in the response. I think adding a filter for the validate_urls function could work in this case, as the call comes from insert_legacy_redirect, which is being used consistently from a CLI context too.

Just a couple of suggestions/caveats:

• Could you add proper filter documentation to the filter (as in my previous example)?
• Can you name it using the full name of the function, like wpcom_legacy_redirector_validate_urls?
• If the function hooked into this filter is meant to prevent the insert process, it should make sure to return a proper WP_Error object, as any other non-object value returned will continue with the insert function execution.

[diksha-nts]

@diksha-nts Thanks for sharing these details. I see, so you would like to return an error object so it's shown in the response. I think adding a filter for the validate_urls function could work in this case, as the call comes from insert_legacy_redirect, which is being used consistently from a CLI context too.

Just a couple of suggestions/caveats:

• Could you add proper filter documentation to the filter (as in my previous example)?
• Can you name it using the full name of the function, like wpcom_legacy_redirector_validate_urls?
• If the function hooked into this filter is meant to prevent the insert process, it should make sure to return a proper WP_Error object, as any other non-object value returned will continue with the insert function execution.

@rbcorrales I have addressed the things you mentioned above. Can you please have a look?

[rbcorrales]

@diksha-nts I see you're applying the same filter twice because there's an early return for an array for some legacy behavior. Calling the filters once makes the code cleaner, easier to maintain, and more predictable in its behavior. To avoid this, I see two options:

1. Remove the non-error return from the if ( is_numeric( $redirect_to ) || false !== strpos( $redirect_to, 'http' ) ) { block, add an else to it and nest the rest of the validations so there's only one return apply_filter() at the bottom of the function:

public static function validate_urls( $from_url, $redirect_to ) {
    ...
    if ( is_numeric( $redirect_to ) || false !== strpos( $redirect_to, 'http' ) ) {
        if ( is_numeric( $redirect_to ) && true !== self::vip_legacy_redirect_parent_id( $redirect_to ) ) {
            $message = __( 'Redirect is pointing to a Post ID that does not exist.', 'wpcom-legacy-redirector' );
            return new WP_Error( 'empty-postid', $message );
        }
        if ( ! wp_validate_redirect( $redirect_to ) ) {
            $message = __( 'If you are doing an external redirect, make sure you safelist the domain using the "allowed_redirect_hosts" filter.', 'wpcom-legacy-redirector' );
            return new WP_Error( 'external-url-not-allowed', $message );
        }
        // Remove the early array return.
    } else {
        // Nest all the other checks inside this else block.
        if ( false === self::validate( $from_url, $redirect_to ) ) {
            $message = __( '"Redirect From" and "Redirect To" values are required and should not match.', 'wpcom-legacy-redirector' );
            return new WP_Error( 'invalid-values', $message );
        }
        if ( 404 !== absint( self::check_if_404( home_url() . $from_url ) ) ) {
        ...
    }

    // Apply filter only once at the end.
    return apply_filters( 'wpcom_legacy_redirector_validate_urls', array( $from_url, $redirect_to ) );
}

2. The other option could be to centralize all the return statements instead of keeping multiple returns for errors. The advantage of this approach is that it also allows for filtering error responses, not only when the validation passes. However, this would require additional testing to ensure none of the other validation flows get affected, as you'll be basically refactoring the function.

public static function validate_urls( $from_url, $redirect_to ) {
    // Initialize a variable to store the result.
    $result = array( $from_url, $redirect_to );

    // Instead of having multiple returns, store the result in a variable and return it at the end.
    if ( false !== Lookup::get_redirect_uri( $from_url ) ) {
        $result = new WP_Error( 'duplicate-redirect-uri', 'A redirect for this URI already exists' );
    } elseif ( is_numeric( $redirect_to ) || false !== strpos( $redirect_to, 'http' ) ) {
        if ( is_numeric( $redirect_to ) && true !== self::vip_legacy_redirect_parent_id( $redirect_to ) ) {
            $message = __( 'Redirect is pointing to a Post ID that does not exist.', 'wpcom-legacy-redirector' );
            $result = new WP_Error( 'empty-postid', $message );
        } elseif ( ! wp_validate_redirect( $redirect_to ) ) {
            $message = __( 'If you are doing an external redirect, make sure you safelist the domain using the "allowed_redirect_hosts" filter.', 'wpcom-legacy-redirector' );
            $result = new WP_Error( 'external-url-not-allowed', $message );
        }
    } elseif ( false === self::validate( $from_url, $redirect_to ) ) {
        $message = __( '"Redirect From" and "Redirect To" values are required and should not match.', 'wpcom-legacy-redirector' );
        $result = new WP_Error( 'invalid-values', $message );
    } elseif ( 404 !== absint( self::check_if_404( home_url() . $from_url ) ) ) {
    ...
    // Continue cascading the checks.

    // Apply the filter only once at the end
    return apply_filters( 'wpcom_legacy_redirector_validate_urls', $result );
}