opentracing / opentracing-php

OpenTracing API for PHP
Apache License 2.0
505 stars 56 forks source link

removes fromHeaders in order to use fromRequest. #34

Closed jcchavezs closed 6 years ago

jcchavezs commented 6 years ago

The aim of this PR is to make explicit we support headers in the PSR way. The reason why I removed the fromHeaders and went for the fromRequest is because it makes it explicit the usage of the PSR request whereas headers was just mimicking text_map so in my opinion this is more expressive.

Relates to #31

Ping @felixfbecker @beberlei @lvht

taoso commented 6 years ago

Had you @jcchavezs @felixfbecker ever think the question that what is the real difference between the text-map carrier and http-header carrier?

As OT spec says

They are both map, isn't it?

The only difference between them is their character set for key and value. For Text Map, you can use any character set. And for HTTP Header, you can only use the character set defined in the RFC7230.

If we really need to shift a HTTP Header carrier, what we only need to do is offer API like this

<?php
public static function fromHeaders(array $headers)
{
    foreach ($headers as $key => $value) {
        check_character_set_according_rfc($key);
        check_character_set_according_rfc($value);
    }

    return new self($headers);
}

You removed the fromHeaders method. It is a big pity.

jcchavezs commented 6 years ago

@lvht http://www.php-fig.org/psr/psr-7/ follows also 7230 and it is the standard, one way to make sure we are compliant with it, removing sanitization from our side is relying on the Psr\RequestInterface which specifies it (of course, you could implement the interface and avoid the charset validation but we relay on contracts). And I do agree, they are both text maps, the difference is how you create them. TextMap is a simple you create out of an array, HttpHeaders is the carrier you create out of the headers model from Psr which is compliant with 7230 and is the standard PHP request. The reason I removed the fromHeaders method is because if we want to make sure it comes in the PSR format the only way to attach to the contract is receiving the request.

taoso commented 6 years ago

HttpHeaders is the carrier you create out of the headers model from Psr which is compliant with 7230 and is the standard PHP request.

@jcchavezs You can create a HttpHeaders carrier out of the headers model from Psr does not necessary means you can only create it out of the headers model from Psr.

Psr is the community standard, but it does not means every must depends on it. It is common for project to get the http headers from the $_SERVER variable. In my opinion, how to get/send http headers is just a choice, not a limit.

As I said,

The only difference between them is their character set for key and value. For Text Map, you can use any character set. And for HTTP Header, you can only use the character set defined in the RFC7230.

As a OT binding, what we only need to do is to make sure that both the key and value of input array is suitable for RFC7230 and let other works to the user.

So I still propose the API like

<?php
public static function fromHeaders(array $headers)
{
    foreach ($headers as $key => $value) {
        check_character_set_according_rfc($key);
        check_character_set_according_rfc($value);
    }

    return new self($headers);
}
jcchavezs commented 6 years ago

Psr is the community standard, but it does not means every must depends on it. It is common for project to get the http headers from the $_SERVER variable. In my opinion, how to get/send http headers is just a choice, not a limit.

This sounds reasonable to me. I added a fromServer as well and using getallheaders underneath. I don't think we need to accept parameters on this one, if you want to pass an array of parameters TextMap is good enough.

What I would not to is to check the charset. It adds an overhead we won't want to have. As I said, if we trust standards we don't need to worry about this.