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

!!! BUGFIX: Browser arguments are the request parsed body #2050

Merged
merged 2 commits into from Aug 6, 2020

Conversation

albe
Copy link
Member

@albe albe commented Jul 7, 2020

Since 6.0 the Browser $arguments were accidentially moved to the request query parameters, while they should reflect the post body arguments.

The docblock even states that:
@param array $arguments Arguments to send in the request body

Note: this slightly changes behaviour if you adapted your usage of Browser since 6.0.0. If you want to send query parameters, you should instead provide an Uri instance with the parameters, as was already the case prior to 6.0. (This is the reason for marking this bugfix as breaking.)

Since 6.0 the Browser `$arguments` were accidentially moved to the request query parameters, while they should reflect the post body arguments.
@albe
Copy link
Member Author

albe commented Jul 9, 2020

Note to self: I should probably add a test for this.

@albe
Copy link
Member Author

albe commented Jul 15, 2020

Looks good to me now. Test confirms the expected behaviour. Maybe we should mark this BUGFIX as "breaking" to make aware of the changed (=fixed) behaviour?

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for taking care!

@bwaidelich
Copy link
Member

Damn, I think I was a bit too quick with that one..
Is 6.0 the right target branch and should this be marked "breaking"?

If we want to revert this one => #2071

@albe
Copy link
Member Author

albe commented Aug 6, 2020

Well, it was breaking when upgrading from 5.x -> 6.x until now - but unintentionally at that. That's why I think it should land in 6.0+ - but maybe it should be marked as breaking and be clearly mentioned, so that an upgrade from older 6.x is not so unexpectedly changing things again (if you never came from 5.x for example).

@kdambekalns kdambekalns deleted the albe-browser-arguments branch August 11, 2020 16:48
@kdambekalns kdambekalns changed the title BUGFIX: Browser arguments are the request parsed body !!! BUGFIX: Browser arguments are the request parsed body Aug 11, 2020
@kdambekalns
Copy link
Member

I'd say we keep it like this now. I adjusted the description slightly and marked it as breaking.

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

Successfully merging this pull request may close these issues.

None yet

3 participants