Skip to content

Add Formatter::formatResponseForRequest() #146

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 10, 2022
Merged

Add Formatter::formatResponseForRequest() #146

merged 2 commits into from
Feb 10, 2022

Conversation

ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Feb 7, 2022

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.

@ro0NL ro0NL changed the title Add For,atter::formatResponseForRequest() Add Formatter::formatResponseForRequest() Feb 7, 2022
Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me.

@Nyholm wdyt?

there is some code rot going on with php 8 and phpstan, but thats not related to the PR.

@Nyholm
Copy link
Member

Nyholm commented Feb 8, 2022

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
Copy link
Contributor Author

ro0NL commented Feb 8, 2022

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

im still not convinced we need moar interfaces :')

@dbu
Copy link
Contributor

dbu commented Feb 8, 2022

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
Copy link
Contributor Author

ro0NL commented Feb 8, 2022

@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
Copy link
Contributor

dbu commented Feb 9, 2022

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

@Nyholm
Copy link
Member

Nyholm commented Feb 9, 2022

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
Copy link
Contributor Author

ro0NL commented Feb 9, 2022

:shipit:

@ro0NL
Copy link
Contributor Author

ro0NL commented Feb 9, 2022

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

no worries, im playing chess on 2 boards ;) symfony/symfony#45358

@ro0NL
Copy link
Contributor Author

ro0NL commented Feb 9, 2022

about deprecating formatResponse(): #145 (comment)

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

@dbu
Copy link
Contributor

dbu commented Feb 10, 2022

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 dbu merged commit 0d8df2b into php-http:master Feb 10, 2022
@dbu
Copy link
Contributor

dbu commented Feb 10, 2022

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

@dbu
Copy link
Contributor

dbu commented Feb 11, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose request to resonse formatter
3 participants