Query parameter without value is dropped

[PunchyRascal]

If there is a parameter without value, e.g. https://example.com/?foo, then the $uri->query_form() method (but others too) do not return the foo parameter at all.

[oalders]

@PunchyRascal thanks for this. Looks like we should deal with this case. http://stackoverflow.com/questions/4557387/is-a-url-query-parameter-valid-if-it-has-no-value

[PunchyRascal]

I was wondering about the keywords mechanism there, that seems to take over if there is no = sign in the query string, what is its purpose? I understand, that the whole query params thing is very loosely defined outside the HTML form.

[dakkar]

Slightly worse issue:

my $uri = URI->new('http://host/foo?keyword');
$uri->query_form(foo=>'bar');
say $uri;

prints http://host/foo?foo=bar, removing the keyword.

And:

my $uri = URI->new('http://host/foo?foo=bar');
$uri->query_keywords(['keyword']);
say $uri;

prints http://host/foo?keyword, removing the query.

(this breaks a few things downstream, like Paws, see pplu/aws-sdk-perl#105 (comment) )

[oalders]

If someone can put together a patch for this, we can look at getting this taken care of.

[dakkar]

Three proposals! They all change the interface, though ☹ I can't find a way to handle equal-less form params that round-trips and keeps the current interface.

[Grinnz]

FWIW, this is how Mojo::URL handles it:

$ perl -MMojo::URL -E'my $url = Mojo::URL->new(shift); $url->query({foo => "bar"}); say $url->to_string' 'http://host/foo?keyword'
http://host/foo?keyword=&foo=bar

When mixing the formats I think this is the only sensible approach, consider the "keyword" to be a form key without a value (which is valid as 'keyword' or 'keyword=' within the & pairs)

[oalders]

I think following the Mojo pattern makes sense.

[oalders]

URI version would be current behaviour:

perl -Ilib -MURI -E 'my $url = URI->new(shift); $url->query_form(foo => "bar"); say $url->as_string' 'http://host/foo?keyword'
http://host/foo?foo=bar

proposed new behaviour:

perl -Ilib -MURI -E 'my $url = URI->new(shift); $url->query_form(foo => "bar"); say $url->as_string' 'http://host/foo?keyword'
http://host/foo?keyword&foo=bar

[castaway]

#65 looks sane.. what's this waiting on? (Also poking at Paws ;)

[oalders]

I think we just wanted some eyeballs on this. It looks like #65 is not passing the test suite. I'll poke some people to review this. Thanks for the nudge @castaway!

[oalders]

@skaji, @simbabque and @vanHoesel , any preference for the proposed solutions?

[vanHoesel]

undef is the more intuitive way of dealing with it. But using a new method, like query-element might be a more save way.

Is there a way to smoke all downstream dependencies ?

[oalders]

I think we could set something up to test the downstream dependencies that we know about.

[simbabque]

I agree with @vanHoesel. The undef interface makes sense. An extra method might make transitioning easier, but at the same time it also is definitely not backwards compatible. But since we're about to change core behaviour, that probably doesn't matter.

I'm not opposed to this change, once we can make sure that we're not breaking everyone's code.

The test suite seems fine - the failure is just an xt test about missing Changes.

[oalders]

I think we could test this out with https://metacpan.org/pod/Test::DependentModules

[simbabque]

I think we could test this out with https://metacpan.org/pod/Test::DependentModules

I'm doing this now.

[simbabque]

I've tried running reverse dependency tests for our master, but ran into some difficulties. I believe parts of this needs to be done manually, and we won't be able to include this as a test that Travis could run for release testing. It is really, really slow. I'll see if I can get them all to pass on master and if I have that, I will rerun for this change.

[castaway]

Any luck?

[simbabque]

Nope. It took a few hours and had lots of failures unfortunately. I gave up.

[oalders]

We could restrict this to a small subset of modules and ensure that they still pass before and after the change.

[vanHoesel]

Can I assist here somehow in running tests ? If so, how ?

[simbabque]

Can I assist here somehow in running tests ? If so, how ?

I'm afraid I don't have the code I ran any more. Sorry.

[oalders]

We could start with a GitHub workflow branched from master which can successfully install some downstream modules. That would generally be helpful anyway.

[choroba]

Any news on this? Is the recommended way to subclass URI and hack one's own solution?

[choroba]

Also, there's another way that's backward compatible: introduce a new value for the keywords. So

$u->query_form({a => "b", c => URI::KEYWORD}); # ?a=b&c

[oalders]

It looks like we had agreed on the PR to merge but we have no concept of how many downstream dependencies this would break. I think we'd need someone to test those first.

[simbabque]

@oalders and I have added a new workflow file for manually running the tests of a dependant module. We've gone with WWW::Mechanize in #126, but it seems there is something wrong with the workflow it never finished despite the test failing. See https://github.com/libwww-perl/URI/actions/runs/4843290328.

[simbabque]

This is now working. We will continue discussion in the relevant PR.