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

[rbcorrales]

@diksha-nts Thanks for submitting this PR! Could you elaborate a bit more on what you want to achieve with this filter? Is it for including additional validations in your custom code?

If that's the case, for the sake of consistency, we suggest directly filtering the WPCOM_Legacy_Redirector::validate return value. Right now, there only seem to be two places where it's used (one of them being the current function):
https://github.com/search?q=repo%3AAutomattic%2FWPCOM-Legacy-Redirector+%3A%3Avalidate%28&type=code

Also, the filter's name should start with wpcom_legacy_redirector_ and would also need proper documentation.

Considering the above, the validate function could be filtered like this:

public static function validate( $from_url, $redirect_to ) {
    $is_valid = ( ! empty( $from_url ) && ! empty( $redirect_to ) && self::transform( $from_url ) !== self::transform( $redirect_to ) );
    
    /**
     * Filter the result of the redirect validation.
     *
     * @param bool   $is_valid    Whether the URLs are valid for redirection.
     * @param string $from_url    URL to redirect (source).
     * @param string $redirect_to URL to redirect to (destination).
     */
    return apply_filters( 'wpcom_legacy_redirector_validate', $is_valid, $from_url, $redirect_to );
}

Will the above approach work for your use case?

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

[GaryJones]

This documentation is incorrect, since a single array is made available to the filter, not two individual parameters.

Please see https://developer.wordpress.org/coding-standards/inline-documentation-standards/php/#4-hooks-actions-and-filters for how arrays should be documented.

[diksha-nts]

@GaryJones Thanks for reviewing the PR, i have addressed the comment. Please have a look now.

[rbcorrales]

@diksha-nts The @param tag should document the parameter type and provide a description, including what each array element represents. For example:

/**
 * Filter the result of the redirect validation.
 *
 * @param array $params {
 *     Array containing the URLs to validate.
 *
 *     @type string $from_url     URL to redirect (source).
 *     @type string $redirect_to  URL to redirect to (destination).
 * }
 */

[diksha-nts]

@rbcorrales , I have updated the documentation accordingly. Please have a look now.