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

Add request method parameter for search and count #1643

Merged
merged 1 commit into from
Aug 19, 2019

Conversation

alamirault
Copy link
Contributor

This PR allow to change request method for search and count as discussed in #1441.

BC BREAK

I also moved default request method from GET to POST as suggested by @ruflin in his comment.

@ruflin
Copy link
Owner

ruflin commented Aug 12, 2019

@alamirault Thanks for taking this on and sorry for the late reply, was on paternity leave ;-) I restarted CI to see if it was just a temporary failure or not. I assume your commit it based on current master?

@alamirault
Copy link
Contributor Author

alamirault commented Aug 12, 2019

Congratulation ! I was based on master, il will rebase on current.
Some tests failed because POST is set by default and they check dynamic changes. Should I delete them?

@ruflin
Copy link
Owner

ruflin commented Aug 12, 2019

Thanks :-) Should the tests be deleted or just adapted? Can you share a link to the culprits?

@alamirault
Copy link
Contributor Author

public function testDynamicHttpMethodBasedOnConfigParameter(array $config, $httpMethod)

I think this test must be deleted. Or I should rework my PR in order to keep GET by default, change to POST only if search have data. And the $method arg (nullable by default) allow to override this behavior. I think is less predictive.

@ruflin
Copy link
Owner

ruflin commented Aug 14, 2019

If POST is the expected default, I think we should fully switch to POST and not have as you said, a less predictive behaviour. So far I can't see any downsides?

+1 on deleting the test.

@alamirault
Copy link
Contributor Author

Code is rebased and I deleted the test.

Copy link
Owner

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for all the additional tests.

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.

2 participants