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

Implemented support for filtering based on custom fields using a where argument #21

Conversation

wcha-peter
Copy link

Allows for filtering of results of the api4 short code with custom fields containing dots, as discusses in PROJ-2246

@agileware-fj agileware-fj self-requested a review April 5, 2022 06:08
Copy link
Contributor

@agileware-fj agileware-fj left a comment

Choose a reason for hiding this comment

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

Thanks a heap for having a crack at this, Peter.

I'd like to request though, that when adding complex filters like this, we try to avoid deviating from the CiviCRM APIv4 syntax, which is why I suggested using JSON with the [ field, op, value ] syntax in the first place.

The great advantages with that are that we can then just pass it into json_decode and defer validation to the API call, and most people can recognise JSON fairly quick rather than having to interpret an unusual syntax.

Would you be able to change it to work like that and resubmit?

@wcha-peter
Copy link
Author

Thanks for that Francis.

In terms of providing json input to WordPress shortcodes, square braces are not valid anywhere within wordpress arguments, so they would need to be encoded using something else - possibly '(' to avoid conflicts with other json syntax.

However, that's starting to stray into defining our own custom syntax - would you still be fine with almost json syntax like (field, op, value) ?

@wcha-peter
Copy link
Author

Just implemented a json based solution as per my above comments, using brackets in place of square brackets. Please let me know your thoughts on this approach, and if you're happy I can flatten the commits,

Copy link
Contributor

@agileware-fj agileware-fj left a comment

Choose a reason for hiding this comment

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

Thanks @wcha-peter

Because of course shortcode parsing can't cope with square brackets anywhere.

The overall idea is okay and I'm happy to go with replacing [] with () for the syntax, but the implementation is going to need to take into consideration when values have parentheses in them before I can merge - e.g. ("custom.field", "=" , "Turtles (are cool)")

@wcha-peter
Copy link
Author

Superseded by #26

@wcha-peter wcha-peter closed this Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants