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 coalesce filtering #179

Merged
merged 5 commits into from
Oct 19, 2014
Merged

Add coalesce filtering #179

merged 5 commits into from
Oct 19, 2014

Conversation

bakura10
Copy link
Member

@Ocramius that was the idea

@Ocramius
Copy link
Member

@bakura10 it's very near to what I was suggesting as well.

I'd suggest using a key that is not a valid property name (such as $in, since $$in is not a valid variable declaration symbol), which prevents collisions.

Additionally, why not extending it to all searchable fields?

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.61%) when pulling 12a3c11 on id into ea12a93 on master.

@bakura10
Copy link
Member Author

How do you define a searchable field? The issue is that some people could try to filter by arbitrary key and making your database suffer if field is not indexed. By only restricting to "ids" (which is what you want often), it ensures filtering is done on an indexed fields. Regarding filtering arbitrary fields, we already have this through the Paginator plugin: https://github.com/zf-fr/zfr-rest/blob/master/src/ZfrRest/Mvc/Controller/Plugin/PaginatorWrapper.php#L68

But at least, you have full control over which fields make searchable.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.37%) when pulling 12a3c11 on id into ea12a93 on master.

@Ocramius
Copy link
Member

But at least, you have full control over which fields make searchable.

Good point. I think restricting by ID is fine for now then, but we may need to open an issue to support the other cases as well ("searchable" property in mappings?)

@bakura10
Copy link
Member Author

Yep, the searchable mapping is a very nice idea.

Anyway, tests and docs were added. Ready for final review!

@Ocramius
Copy link
Member

@bakura10 can you simply add the isSearchable or such to the mappings, and simply forward it to hasIdentifier for now?

/**
* The coalesce filtering key
*/
// 'coalesce_filtering_key' => 'ids',
Copy link
Member

Choose a reason for hiding this comment

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

$ids plox

Copy link
Member Author

Choose a reason for hiding this comment

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

isn't this strange?

Copy link
Member

Choose a reason for hiding this comment

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

The point is having something that EXPLICITLY cannot be an entity field: the regex for valid property names is something like /([a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]+)/: the default value provided by zfr-rest should not match against this regex.

@bakura10 bakura10 self-assigned this Oct 19, 2014
bakura10 added a commit that referenced this pull request Oct 19, 2014
Add coalesce filtering
@bakura10 bakura10 merged commit 64c4aa9 into master Oct 19, 2014
@bakura10 bakura10 deleted the id branch October 19, 2014 16:22
@coveralls
Copy link

Coverage Status

Coverage increased (+0.16%) when pulling 34638f7 on id into c31696b on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.16%) when pulling 34638f7 on id into c31696b on master.

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.

3 participants