php-http / message

HTTP Message related tools
http://php-http.org
MIT License
1.3k stars 42 forks source link

Add Formatter::formatResponseForRequest() #146

Closed ro0NL closed 2 years ago

ro0NL commented 2 years ago
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Related tickets fixes #145
Documentation -
License MIT

During 1.x lifecycle:

deprecation.INFO: User Deprecated: Class "Http\HttplugBundle\Collector\Formatter" should implement method "Http\Message\Formatter::formatResponseForRequest(ResponseInterface $response, RequestInterface $request): string": Formats a response in context of its request.

Next major release should add the method for real.

Nyholm commented 2 years ago

Im happy with this.

But this will only have a "real" effect in v2. Currently v2 is not planned.

Introducing a new interface would make more sense and would make this change "real" now. I see that it was discussed in #145

ro0NL commented 2 years ago

i will update at least logger plugin as such, using method_exsists trick till v2.

im still not convinced we need moar interfaces :')

dbu commented 2 years ago

But this will only have a "real" effect in v2. Currently v2 is not planned.

aparently this is the symfony way of "adding" methods to interfaces without breaking BC. the consumer would have to use method_exists as i understood

ro0NL commented 2 years ago

@method is phpdoc standard, yet symfony triggers the deprecation for implementors.

this is my personal preference though, as it's more easy to discover than a new contract IMHO.

for consumers methods_exists+@method vs instanceof+NewFormatter is irrelevant IMHO :)

which raises the question, what new contract would we add? AdvancedFormatter? Do we want it? (knowing there's a way to stick with just Formatter).

dbu commented 2 years ago

i agree with @ro0NL's reasoning. ok for you @Nyholm or do you have a strong feeling that that would be wrong?

Nyholm commented 2 years ago

I dont have any strong feelings.

I do like the "symfony way". But that only works well because Symfony always has a next major version planned.

Im happy with the PR in the current state if @ro0NL is happy. (Given the fact that it might never be a version 2 coming up)

ro0NL commented 2 years ago

:shipit:

ro0NL commented 2 years ago

Given the fact that it might never be a version 2 coming up

no worries, im playing chess on 2 boards ;) https://github.com/symfony/symfony/issues/45358

ro0NL commented 2 years ago

about deprecating formatResponse(): https://github.com/php-http/message/issues/145#issuecomment-1030182461

i think it makes sense :+1: ideally there's 1 responsible method for response formatting. Let me know if should be part of this PR.

dbu commented 2 years ago

if we do the new method on the same interface, i would indeed deprecate the old formatResponse method. and best to this in the same MR.

dbu commented 2 years ago

thanks! i will see if we can get the build to green again, that would be good to see before tagging the release.

dbu commented 2 years ago

and released https://github.com/php-http/message/releases/tag/1.13.0