thephpleague / uri-src

URI manipulation Library
https://uri.thephpleague.com
MIT License
30 stars 7 forks source link

Difference between `Query::new()` and `Query::fromVariable([])` #135

Closed davidrans closed 7 months ago

davidrans commented 7 months ago

Bug Report

(Fill in the relevant information below to help triage your issue.)

Information Description
Package uri-components
Version 7.4.1
PHP version 8.3.2
OS Platform Linux

Summary

> $q = League\Uri\Components\Query::new()            
= League\Uri\Components\Query {#5499}                

> $q->getUriComponent()                              
= ""                                                 

> $q2 = League\Uri\Components\Query::fromVariable([])
= League\Uri\Components\Query {#5528}                

> $q2->getUriComponent()                             
= "?"                                                

Expected result

I would expect getUriComponent() to return "" in both cases

Actual result

League\Uri\Components\Query::fromVariable([])->getUriComponent() returns "?"

nyamsprod commented 7 months ago

Well I see where this reasoning comes from but PHP behaviour is at "fault" on this one. Query::fromVariable output depends on http_build_query behaviour whereas Query::new does not.

<?php
var_dump(http_build_query([])); // returns ''

http_build_query never returns null (aka the absence of query), it returns the empty string instead, while Query::new() by default returns null.

Hope this clarify the output.

davidrans commented 7 months ago

Along the same lines:

> $u = League\Uri\Http::new('http://abc.com?a=1')              
= League\Uri\Http {#5714}                                      

> $q = League\Uri\Components\Query::fromVariable([])           
= League\Uri\Components\Query {#5525}                          

> League\Uri\Modifier::from($u)->mergeQuery($q)->getUriString()
= "http://abc.com?a=1&"

Seems pretty unintuitive. Maybe fromVariable() should just handle empty array input differently?

nyamsprod commented 7 months ago

No It should not, doing so would break expectation.

You should interchangeably use fromVariable and http_build_query. Both work with array like in your example but also with objects which is a less documented feature with has an even more "strange" behaviour.

IMHO fromVariable should be used when migrating from http_build_queryto proper query usage and creation. Or detecting or deciding if null should be use/return should be decided at application level, the library should not decide as it is left to the user to do so.

davidrans commented 7 months ago

Gotcha, maybe I just misunderstood the purpose of that function. The upgrade docs say to replace Query::createFromParams with Query::fromParameters, but I looked and that function is also deprecated and references fromVariable:

https://github.com/thephpleague/uri-components/blob/b94fe4097885f1b51c4c3fcb78025fbbabbb5d9d/Components/Query.php#L662

Is there a replacement for createFromParams/fromParameters that maintains similar behavior?

nyamsprod commented 7 months ago

createFromParams/createFromParameters are indeed replaced by fromVariable because of an issue in the methods design for more information check the following issue https://github.com/thephpleague/uri-src/issues/123

from v6 -> v7 createfromParams is replaced by fromParameters in v7 (after the initial release) fromParameters is deprecated in favor of fromVariable

hence why the documentation recommands fromParameters. Maybe the documentation needs a bit of update. As always a PR is welcomed if you find the proper way to clarify that.

davidrans commented 7 months ago

I appreciate the clarification :+1:

I'll just update the places I construct Querys to something like:

if ($params) {
   $q = Query::fromVariable($params);
} else {
   $q = Query::new();
}