Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix sorting bug when no existing class property is specified in query string parameter #193

Closed
wants to merge 1 commit into from

Conversation

okapon
Copy link
Contributor

@okapon okapon commented Mar 8, 2018

I'm using KnpPaginatorBundle. And I want to sort array of objects.

Now v1.3.5, When some user sent bad query string parameter, then the object does not have getter method, NoSuchPropertyException exception will be thrown (This problem related #174 and be caused by #159).

Before v1.3.4, if sort property does not exist, the property has just been ignored.

My solution is below.
Checking property existence using propertyAccessor::isReadable(). It is simple.
But there are still problem, because of php5.x usort() bug. https://bugs.php.net/bug.php?id=50688

Because propertyAccessor::isReadable() throws exception internally, then unfortunately usort() crash bacause of it's bug.
The error like this.

Warning: usort(): Array was modified by the user comparison function

So I separate checking logic according to the version of php.

@okapon okapon force-pushed the master branch 2 times, most recently from e57158d to 2b53de8 Compare March 8, 2018 17:07
@okapon okapon changed the title Fix sorting bug when no existing class property is specified Fix sorting bug when no existing class property is specified in query string parameter Mar 13, 2018
@okapon
Copy link
Contributor Author

okapon commented Jun 8, 2019

I think my problem already fixed by #207. So I close this PR. Thank you.

This problem is not solved yet. Because NoSuchPropertyException is thrown in this problem.
I will send PR again.

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

Successfully merging this pull request may close these issues.

1 participant