Add withServerParams() API to Slim PSR-7 request objects.

[soatok]

This was missing from the latest release, although I expected it to exist. I mostly need it for unit tests, but I also wrote an IP address anonymizer that might work well with Slim's PSR-7 library.

[coveralls]

Coverage increased (+0.001%) to 99.734% when pulling c34f5f9 on soatok:master into 573678c on slimphp:master.

[l0gicgate]

@adriansuter this isn't part of the spec. It's just a helper function. I'm not opposed to adding it, it does seem like more of a usage specific helper though.

@soatok why do you need to mutate the server params though? During a request's life, it should technically be instantiated with the correct server params from the get go no? Can you please enlighten us as to why that's necessary?

[soatok]

I described this in my first comment.

See also: https://github.com/soatok/faq-off/blob/2170ddc184974b5848ab46bd0e4d6a57aea0a1b3/src/FaqOff/Endpoints/Entry.php#L120-L127

This creates a mutated copy of $_SERVER (but wrapped in the PSR-7 object) that uses my anonymizer to store IP addresses, user agents, etc. that have been processed by a RPF with a key that is rotated every 24 hours to maximize end user anonymity.

This could be easily implemented at a framework level if I wanted to just make an anomymizer middleware by allowing mutation. My current implementation forces me to just rebuild a new Request object instead of a cloned mutant.

[adriansuter]

@l0gicgate Shouldn't that go to https://github.com/slimphp/Slim-Http?

[l0gicgate]

@adriansuter the thing is I'm not sure it belongs on either repository, I'm uneasy with modifying server params after request object has been instantiated. I understand that for this particular use case it makes things easier, but what other use cases is that for and are we opening something up that we shouldn't.

[soatok]

The current usage (as of my PR) is consistent with the other get*() methods that exist with Request.

Its absence is noteworthy compared to withRequestTarget(), withUri(), and especially withCookieParams(), which complement the existing get*() methods respectively.

From an API consistency and end user expectation perspective, this should be included if the other methods are to remain as well.

[l0gicgate]

@soatok those methods are intended to modify parameters that should be modified as per the PSR-7 interface specs. Modifying server params isn't something that should be done. Those params are passed from the top down, they're not meant to be mutated.

[adriansuter]

I think there is a reason why this method didn't make it into the PSR-7 specs.

[soatok]

I can work around its absence but it's hacky.

I still contend that having a more consistent API (a with method paired with every get) will satisfy user expectations better than what Slim-Psr7 currently does.

If you want to exclude the capability for the reasons stated in this discussion, it should be a dummy method that throws an exception instead of simply not existing.

public function withServerParams() {
    throw new NotImplementedException("You're being very silly, developer.");
}

[l0gicgate]

@soatok I think you're the one being "very silly". Following the PSR-7's spec design, modifying server params isn't part of the interfaces for a reason. In that regard, we will not be implementing this functionality.

[soatok]

I think you're the one being "very silly".

I'd like you to revisit my comment again for a moment.

Exception is thrown when developer uses the API that I, and probably others, would expect to exist.

Exception message calls such developers silly.

You insist that I'm, in fact, the one being silly.

I don't think that was ever up for debate. Maybe you misunderstood the intent, but I wasn't namecalling. I'll send an alternative patch with a less tongue-in-cheek exception message.

[l0gicgate]

No need for such patch, don't waste your time. It's not part of the PSR-7 spec so there are no expectations there, we're not going to implement it. Thanks.