bpolaszek / uri-factory

PSR-17 compliant PSR-7 Uri factory
MIT License
4 stars 0 forks source link

use Http::createFromString instead #1

Closed nyamsprod closed 6 years ago

nyamsprod commented 6 years ago

https://github.com/bpolaszek/uri-factory/blob/c67bbffd4ead250df4cba2733276bfdd80ccc8ad/src/Adapter/LeagueUriAdapter.php#L30

You should use the League\Uri\Http::createFromString method instead of the League\Uri\Factory::create method . With the latter you have no guarantee that the returns oject is a Psr7 UriInterface object. For instance this code will fail

        $factory = LeagueUriAdapter::factory();
        $uri = $factory->createUri('ftp://www.example.net');
        $this->assertInstanceOf(UriInterface::class, $uri);

Because it will return a League\Uri\Ftp class which does not implement the Psr7 UriInterface.

bpolaszek commented 6 years ago

Hi @nyamsprod,

Thank you for reporting this, I will fix it in the morning.

Btw, how have you been noticed of the creation of this repository? It's pretty fresh and I've told nobody about it for the moment :-)

Ben

nyamsprod commented 6 years ago

@bpolaszek because I know everything :D

nyamsprod commented 6 years ago

not really because it reminded me of https://github.com/thephpleague/uri-components/blob/master/src/Components/Query.php and before adding more features to my classes I usually tries to see what's be done elsewhere

nyamsprod commented 6 years ago

So I am backporting some of your ideas in my class which is super easy btw. Hope you'll find ideas in my class too.

bpolaszek commented 6 years ago

I must admit this is the component I used before creating bentools/querystring last week along with this repository.

I needed a more convenient way to manipulate params in a multidimensionnal array query string, and to render it like foo[]=bar instead of foo[0]=bar like many libraries and http_build_query() do.

nyamsprod commented 6 years ago

yes my class deals with pairs but not with params I've already added separator manipulation thanks to your repo. Dealing with query parameters is a pain and parse_str/http_build_query limitations and quircks does not help that why I use string to let the developper deals with this "pain"

bpolaszek commented 6 years ago

I know what this is, I've been facing headaches with query strings and parameters for years, but what I achieved last week with bentools/querystring and bentools/uri-factory eventually fits all what I need in all my projects so far :-)

nyamsprod commented 6 years ago

yes but even your solution has some limitation because it's a remove all or keep all numeric indexes you can not filter out the indexes you want to keep :) ... anyway any code that uses deep array in query string is bad IMHO. where I work when I found this kind of usage I make patches because it add unnecessary headaches for maintenance and query building.

bpolaszek commented 6 years ago

That's what I solved: https://github.com/bpolaszek/querystring/blob/master/doc/ManipulateParameters.md => go to Remove a parameter at a specific path.

Consider the following query string:

filters[status][]=pending&filters[status][]=reopened&filters[users][]=1&sort[createdAt]=desc

$qs->withoutParam('filters', 'status', 0); 
// filters[status][]=reopened&filters[users][]=1&sort[createdAt]=desc

$qs->withoutParam('filters', 'status'); 
// filters[users][]=1&sort[createdAt]=desc

$qs->withoutParam('filters'); 
// sort[createdAt]=desc

This really helps in my use case. And I have to work with external APIs where I can't manage the query string syntax.

nyamsprod commented 6 years ago

see what you mean. What's missing in my lib is a equivalent to your Query::withoutParam to deal with the 3rd example. The rest can be dealt with maybe in a less friendly way.

$qs->merge('filters[status][]=reopened'); //but one has to understand how merge works
$qs->withoutPairs('filters[status]'); // this is how it is done in JavaScript for instance

I know how to do the 3rd example but right now it is quite ugly :D and involved using ::createFromPairs maybe I should add a withoutPairStartingWith(string $pattern) method which filter out pairs that matches the starting pattern.

(string) Query::createFromPairs(array_filter($qs->getPairs(), function ($key) {
        return strpos($key, 'filters') !== 0;
    }, ARRAY_FILTER_USE_KEY));
bpolaszek commented 6 years ago

In my opinion, when you're manipulating a query string, you're manipulating parameters (so, an array), more than a string. Otherwise I would have used a pure string manipulation class like stringy. Because you have keys and values, and you mostly want to replace the value of a key (or creating it by default) when manipulating the query string.

But I've seen too much of $url = $url . '&foo=bar' without even checking the presence of the question mark :dizzy_face:

Besides, using static methods each time is pretty cumbersome to my eyes, I definitely prefer the use of immutable setters.

nyamsprod commented 6 years ago

@bpolaszek I agree but the problem is PHP's parse_str which produce an array which does not represents the query string its a one way transformation with data loss (by the way) the correct representation is using query key/pair which produce a key/pai collection without data loss. But PHP users are not familiar with this simple fact.

bpolaszek commented 6 years ago

Yes indeed. And I've also seen, in some APIs, query strings like status=foo&status=bar which parse_str() resolves to ['status' => 'bar'] instead of ['status' => ['foo', 'bar']].

By default, bentools/querystring relies on parse_str() to convert a string into parameters, because it will work most of the time.

But you can also implement your own logic to decode the string, and then, instanciate a QueryString object with an array of parameters: (maybe I should add an UrlDecoderInterface and a NativeDecoder for that purpose)

use function BenTools\QueryString\query_string;
$qs = query_string(['status' => ['foo', 'bar']]);

Then, you can create your own renderer for (string) $qs renders back to status=foo&status=bar instead of status[]=foo&status[]=bar:

use BenTools\QueryString\Renderer\QueryStringRendererInterface;
use BenTools\QueryString\Renderer\NativeRenderer;
$qs = $qs->withRenderer(new class implements QueryStringRendererInterface
{
    public function render(QueryString $queryString): string
    {
        return preg_replace(
            '/\%5B\d+\%5D/',
            '', // Perfectible, but it works
            NativeRenderer::factory()->render($queryString)
        );
    }
});
print $qs; // status=foo&status=bar

The goal of bentools/querystring is to help you instanciate, manipulate, and render query strings, no matter its structure's standardness.

bpolaszek commented 6 years ago

@nyamsprod Based on your comments I have externalized parse_str() ito a NativeParser class - which means you can implement your own logic to instanciate query strings: https://github.com/bpolaszek/querystring/blob/master/doc/Instanciation.md#create-your-own-parser

We aren't in the correct repository, but anyway, your comments really help! :sunglasses:

nyamsprod commented 6 years ago

you're welcome I've added a Query::withoutParams in mine so 👍