Ensure PHP's urldecode is not NULL

[BrookeDot]

The plugin uses PHP's urldecode function in a few places. If an empty string is passed to this function, a deprecation warning is returned:

ErrorException: Deprecated: urldecode(): Passing null to parameter #1 ($string) of type string is deprecated

The two places where warnings appear are:
https://github.com/Automattic/WPCOM-Legacy-Redirector/blob/develop/includes/class-utils.php#L29-L36
https://github.com/Automattic/WPCOM-Legacy-Redirector/blob/develop/includes/class-utils.php#L49

Should we add is_empty() checks to $matches and $url respectively? The WordPress wp_parse_url should always return "false" or an array, but clearly that is not always happening.

[GaryJones]

Can you detail the replication steps please?

I'd like to understand why the exceptional cases are failing, rather than bury the error.

[Nikschavan]

The error happens when Utils::mb_parse_url is called from here with $component = PHP_URL_QUERY -

WPCOM-Legacy-Redirector/includes/class-lookup.php

Line 109 in 69a7634

$url_query_params = Utils::mb_parse_url( $url, PHP_URL_QUERY );

Example - https://3v4l.org/p6tvt

On a sidenote, it looks like these two variables are unused in the function -

WPCOM-Legacy-Redirector/includes/class-lookup.php

Lines 105 to 106 in 69a7634

	$preserved_param_values = array();
	$preserved_params = array();

[Nikschavan]

The WordPress wp_parse_url should always return "false" or an array, but clearly that is not always happening.

wp_parse_url() can return null as per it's documentation - https://github.com/WordPress/wordpress-develop/blob/1e21ecedf19f1b97360949a9e509a3c04ac1f34e/src/wp-includes/http.php#L708-L709

[GaryJones]

Thanks @Nikschavan .

Do you think adding a check of null === $parts and returning if true, after https://github.com/Automattic/WPCOM-Legacy-Redirector/blob/develop/includes/class-utils.php#L38 makes sense, so that Utils::mb_parse_url() returns null without it having gone through urldecode()?

[Nikschavan]

IMO, yes if wp_parse_url() is null, mb_parse_url() can return it as-is and that would fix this warning for us.

[GaryJones]

Would you like to open a PR for it?

I don't know when we'll do a release, so you may need to do the same fix locally, knowing that when there is a release the fix will be included.

[Nikschavan]

I have created a pull request #137

We are using the plugin's development branch as we need UI in a project. Once the PR is merged, we can update it to the latest commit pin. Thank you!

[GaryJones]

#137 has now been merged.