Fix possibility extends

[ilyar]

No description provided.

[ilyar]

ready

[jeskew]

Hi @ilyar,

PSR-7 doesn't define a constructor in ServerRequestInterface, so there's no guarantee that calling new static(...) with the arguments expected by Guzzle's implementation will work. Subclasses of ServerRequest could require more or different constructor arguments.

[ilyar]

@jeskew I agree, but this is already the responsibility of the project.

Trying to use your implementation, I encountered the difficulties of extension. Maybe I want the wrong...

I had doubts in the current decision and I was researching the topic, I found the same solution in an alternate implementation PSR7 (https://github.com/slimphp/Slim-Psr7/blob/8814eb071ce04239eaaa3af18f35d90fff62e9ae/src/Request.php#L146).

[Nyholm]

I'm 👎 for introducing late static binding. The ServerRequest should be considered as a value object and you should never have the need to extend it. (Ie the code should not be written with worse performance just to allow you to extend it)

[kael-shipman]

Just wanted to weigh in here, since I created a duplicate bug report and fix over in #159 .

To the first point, re ServerRequestInterface: While PSR-7 doesn't define a constructor, your implementation of it does, and it prevents different arguments from being passed into constructors of derivative classes (except in the exceptional case of a totally oblivious programmer heedlessly repurposing the arguments you've defined). While someone can certainly define more arguments, this won't impair the functionality introduced by using static instead of the explicit class name.

To the second point, re "value object": This is a shortsighted and arbitrary argument that rests solely on the validity (or not) of your own vision for how the implementation should be used. One of the major advantages of OOP is that I don't have to copy-paste all the good work you've done to add functionality that I need for my application. As it stands, my options are limited to finding a different implementation (which is more wasteful of human resources in the open source community than using late static binding would be of machine resources), or to using my own fork of your implementation, which is also wasteful (and annoying).

Please reconsider pulling this fix.

[kael-shipman]

@ilyar Thanks for creating this fix, and thanks especially for creating a test case, which is something I forgot to do in mine :).

Just wanted to share an opinion: To me, it seems better to define FooExtend in a separate file. I often do this by using a classes folder under tests, namespacing those classes with Test, and then including them in the autoload-dev section of the composer file. The modified composer.json might look like this:

    ....
        "files": ["src/functions_include.php"]
    },
    "autoload-dev": {
        "psr-4": {
            "\\Test": "tests/classes/"
        }
    },
    "extra": {
    ....

Then you can use the class in any test by calling \Test\FooExtend.

Anyway, take it or leave it!

[ilyar]

@kael-shipman Maybe it would be appropriate to call it so GuzzleHttp\Tests\Fixture\FooExtend?

And accordingly place tests\Fixture\FooExtend.php.

[kael-shipman]

That could work, though I always thought of Fixtures as predictable data states (i.e., NOT just classes meant to test functionality of other classes).

My vote is to make the namespace \GuzzleHttp\Test. That would keep it simple and also allow them to possibly break that set of test classes out into a separate repo if they start using them across multiple sub-projects (something I do occasionally).

Anyway, your call!

[ilyar]

fix

[Tobion]

While PSR-7 doesn't define a constructor, your implementation of it does, and it prevents different arguments from being passed into constructors of derivative classes

@kael-shipman That's not true at all. A subclass can overwrite the constructor in any way it wants. I can change the order of arguments, remove argument and just call the parent constructor with different stuff.

So I agree with @Nyholm that using late static binging here is technically not correct and unsafe to do. If you really need a subclass, you can just do something like MySubclassRequest::fromPsr7(ServerRequest::fromGlobals());. Actually such functionality might even make sense to include in guzzle\psr7 anyway. So I'd welcome you to create a method Request::fromPsr7 that works for Request and ServerRequest and uses static to also work with subclasses. This way, it's more explicit and an opt-in behavior for late static binding.

[Tobion]

Different implementatin via Request::fromPsr7

[kael-shipman]

TL;DR

1. Technically, I was wrong about arbitrary redefinitions of __construct (I admit it). But it doesn't actually matter.
2. There's a strong case to be made for using LSB in fromGlobals, and a strong case against the alternatives.
3. In short, implementing LSB in fromGlobals results in fewer unexpected errors, fewer duplicate bug reports, fewer codebase forks, less documentation to write, and less time wasted for everyone, while the cost of carelessly redefining the constructor is not only negligible (because it's easily spotted and easily rectified), but unlikely to even be incurred by anyone.

---

@Tobion Ok, I concede that another programmer can redefine the constructor arbitrarily. I was convinced otherwise because I'm usually coding against interfaces, which, as others have noted, this constructor is not bound by. (I didn't realize that method signature verification only applied when implementing interfaces and not when subclassing.)

Still, I strongly believe late static binding in fromGlobals is the way to go here. Think about this:

When I subclassed your class, the first thing I did was make sure that any methods I overrode (including __construct) were compatible with your definitions. This is standard due diligence, and I think the fear that someone would create an incompatible subclass method is unfounded.

The second thing I did was try to instantiate my new subclass using the fromGlobals method, which, by all counts, should have worked as expected. To my surprise, it didn't, and I had to debug it, eventually discovering what I thought was a bug and reporting it. This wasted my time and yours, and obvious @ilyar's, too, when he found it and reported it, and will probably waste other people's time as more people run into this and try to report it (and yours along with it as you close the repeat bug reports and point people to this discussion).

Now, consider what would happen if we implemented the fromPsr7 method as you suggest. The method itself has no obvious use-case -- after all, why would you want to create a request from a request when you could just use clone? --, which means it requires documentation and a justification for its existence (something the late static binding scenario does not). Furthermore, it would require essentially copying the internal properties of one object to the other -- a menial task that requires maintenance as implementation changes.

Even more than that, if you'll forgive my saying so, it's ugly. Would you rather type, MySubclass::fromGlobals() or MySubclass::fromPsr7(Request::fromGlobals())? Perhaps not the most significant of complaints, but at worst a worthy consideration for your fellow programmers.

Now consider simply implementing late static binding. In the event that someone subclasses responsibly, everything works as expected. No errors, no debugging, no duplicate bug reports, no bikeshedding.

In the event that someone changes your arguments and tries to use fromGlobals, they'll immediately discover that their constructor is not compatible with what's being called in fromGlobals. In that case, they may simply create a specialFromGlobals method to get around it, or they may change their constructor to match yours. In either case, the functionality of the original fromGlobals method will easily be identified as correct, and they won't engage needlessly with the community to try to get you to change your constructor to match theirs.

Conclusion

As you can see, the costs of implementing LSB in fromGlobals are unlikely ever to be incurred by anyone, and if they are, they're limited exclusively to the person trying to incorrectly subclass the original class.

The costs of taking a different approach are increased code and documentation maintenance and continued engagement with other programmers as they continue to run into what honestly looks like a bug and request that it be fixed, not to mention a proliferation of forks that makes the landscape confusing.

Please -- please -- consider incorporating this change. It costs you nothing.

Thanks,
Kael

[Krinkle]

This can be written in an cleaner way, as assertInstanceOf(FooExtend::class, $actual).

This avoids needing to manually resolve namespace paths or escape characters.

[ilyar]

done

[ilyar]

@Krinkle it is possible, because support php5.4 https://travis-ci.org/guzzle/psr7/builds/349217118

[Krinkle]

Hmm... I see. I wasn’t expecting PHP 5.4 support here in the master branch. The oldest is expect at this point is PHP 5.6, or, of supporting current oldstable Debian, maybe PHP 5.5.

Thanks for considering! Maybe another time.

[sagikazarmark]

I'm pretty confident that I will propose making all message classes in this project final if we ever hit a 2.0 version.

Reason: HTTP messages are represented as value objects, implementing an interface. From this point I don't see any valid use cases for extending the message classes.

So for now, I'm rejecting this change.

[ilyar]

Thank you for your explanations.