Skip to content
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

Release AddPathPlugin fix in 1.9.x #171

Open
garex opened this Issue Mar 10, 2019 · 9 comments

Comments

Projects
None yet
4 participants
@garex
Copy link

garex commented Mar 10, 2019

Q A
Bug? no
New Feature? no
Version 1.9.1

Actual Behavior

AddPathPlugin was broken in 1.x and fixed in 2.x branch.

Expected Behavior

AddPathPlugin must be fixed in 1.x branch too.

Steps to Reproduce

Not required.

Possible Solutions

Release AddPathPlugin fix in 1.x branch. For 3rd-party developers fix of this plugin will be simpler than adopting to 2.x. Currently I'm using https://github.com/janephp/open-api-runtime/ which depends to ^1.4.

@garex

This comment has been minimized.

Copy link
Author

garex commented Mar 10, 2019

Only drop : Promise and declare(strict_types=1); from it.

@garex

This comment has been minimized.

Copy link
Author

garex commented Mar 10, 2019

Currently I need to copy-paste it to my project and this is stone age..

@dbu

This comment has been minimized.

Copy link
Contributor

dbu commented Mar 10, 2019

if i remember correctly, we did this only for master because the behaviour changes slightly. i would recommend that you copy the plugin into your application namespace and configure it as a service and register it with the client.

@garex

This comment has been minimized.

Copy link
Author

garex commented Mar 10, 2019

@dbu

This comment has been minimized.

Copy link
Contributor

dbu commented Mar 11, 2019

i think this is one of those borderline cases where its not a straight up bug fix but a fix that can have a different unexpected behaviour. however, i notice now that we did not label #141 as BC break.

@php-http/httplug how do you think about backporting that fix to version 1? if memory serves me correctly, we started out on 2.0 because we wanted to do that thing with a marker header on the request to know if we already saw the request...
i vote for putting it into 1 as the potential for a surprise is much smaller than the nuisance of long running processes "randomly" not working.

@joelwurtz

This comment has been minimized.

Copy link
Member

joelwurtz commented Mar 11, 2019

We could also keep the same behavior and only fix the memory bug in 1.x (unsetting the id after response resolution) ?

@dbu

This comment has been minimized.

Copy link
Contributor

dbu commented Mar 12, 2019

i think we discussed that option in #141 and realized it also changes the current behaviour. i think if we do change it, we should change it to do the same in 1.9 as in 2.

@Nyholm

This comment has been minimized.

Copy link
Member

Nyholm commented Mar 12, 2019

Didn't we decide that if we make any changes to AddPathPlugin we would exclude one use-case. There are no solution that would fix all problems. That is why we left it as it was and recommended people to implement their own AddPathPlugin if their edge case was not covered.

@garex

This comment has been minimized.

Copy link
Author

garex commented Mar 12, 2019

@Nyholm which use-case? I think better to have correclty working version, than non-working + suggestion to implement own version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.