Avoid erroring when getQueryArgs processes a malformed URL

[kozer]

What?

Related to #45558

When getQueryArgs parses a malformed URL it throws an error.

Why?

The current functionality breaks calypso
Please check this issue for more info

How?

Ignore the malformed parameters and continue parsing the url

Testing Instructions

Before

1. Try to parse the following url: http://calypso.localhost:3000/start/domains?ref=calypso-selector&source=my-home&siteSlug=%E0%A4%A using getQueryArgs('http://calypso.localhost:3000/start/domains?ref=calypso-selector&source=my-home&siteSlug=%E0%A4%A')
2. Validate that function throws an error

After

1. Try to parse the following url: http://calypso.localhost:3000/start/domains?ref=calypso-selector&source=my-home&siteSlug=%E0%A4%A using getQueryArgs('http://calypso.localhost:3000/start/domains?ref=calypso-selector&source=my-home&siteSlug=%E0%A4%A')
2. Validate that the function doesn't throw an error, and the parsing continues

Screenshots or screencast

[codesandbox]

  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @kozer! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[kozer]

@danielbachhuber Can you please take a look into this? It's part of this issue.

Thanks!

[danielbachhuber]

PHP's parse_str() returns the malformed query var:

wp shell
wp> parse_str( 'foo=bar&baz=%E0%A4%A', $args );
=> NULL
$ wp> $args
=> array(2) {
  ["foo"]=>
  string(3) "bar"
  ["baz"]=>
  string(4) "�%A"
}

getQueryArgs() was originally designed to emulate parse_str() (#20693). Would it be possible to return the malformed value instead, for consistent behavior?

[kozer]

Thanks, @danielbachhuber for pointing this out. I wasn't familiar with that.
I did the changes as requested.

Please note, that although this is a step to the right direction, will minimally affect the issue we have in calypso.

Calypso uses two ways to parse URLs:

1. qs library
2. Using the following import:

import { getQueryArgs } from 'calypso/lib/query-args';

This internally uses the get-query-args I'm about to fix here, but to fix the issue related to this pr, we need to stop using qs and start using our function as stated above.

I proposed to add that inside the calypso README file for other developers to be aware.

We can create a ticket to remove qs library of course, but this is a huge take and I think it's better if we do it incrementally.

Maybe we should communicate that also to the calypso channel as well?

Any thoughts?

Thank you!

[danielbachhuber]

This internally uses the get-query-args I'm about to fix here, but to fix the issue related to this pr, we need to stop using qs and start using our function as stated above.

I proposed to add that inside the calypso README file for other developers to be aware.

We can create a ticket to remove qs library of course, but this is a huge take and I think it's better if we do it incrementally.

@kozer I'm not sure README is quite the right approach. Can you create a new issue in https://github.com/Automattic/wp-calypso and we can discuss with the Calypso team?

[ntsekouras]

getQueryArgs() was originally designed to emulate parse_str() (#20693). Would it be possible to return the malformed value instead, for consistent behavior?

I kind of agree with this. Is there is a reason not to return the value? You could just replace decodeURIComponent with safeDecodeURIComponent then.

[tyxla]

Raising another hand of agreement. I think using safeDecodeURIComponent was the intention here since it does exactly what we're aiming for.

[kozer]

@ntsekouras , @tyxla I did the changes you proposed. Thanks so much!
I already have created an issue here. Do you think it make sense to open a new one?

[danielbachhuber]

I already have created an issue here. Do you think it make sense to open a new one?

@kozer No, that's sufficient.

[tyxla]

Thanks for taking care of that, @kozer, looks great 🙌

[danielbachhuber]

Looks good to me, @kozer ! Flagging for a second review from @tyxla or @ntsekouras

[tyxla]

As @ntsekouras also brought up, we should be able to use safeDecodeURIComponent() here since it does exactly what we're aiming for.

Suggested change

	.map( ( kv ) => {
	try {
	return decodeURIComponent( kv );
	} catch ( uriComponentError ) {}
	return kv;
	} );
	.map( safeDecodeURIComponent );

Note that we'll need to import it in this module.

[tyxla]

Works well and the code looks good. 👍

I've left a minor optional suggestion.

Thanks, @kozer! 🙌

[tyxla]

Thanks for taking care of that, @kozer, looks great 🙌

[danielbachhuber]

@tyxla In Managing Packages, I see:

Publishing WordPress packages to npm is automated by synchronizing it with the bi-weekly Gutenberg plugin RC1 release.

When will this happen next for @wordpress/url? Was it supposed to happen a couple days ago?

[tyxla]

They're usually every 2 weeks, so you can expect them within today and the next couple of days.