Simplify filter logic

[smartacus]

I am building a site using the civievent-widget and found that I could not specify a custom_filter parameter without also specifying the custom_display parameter. I was unable to configure the custom_display parameter reproduce the style of the standard display events (specifically due to date formatting), and wasn't really interested in creating a custom display event at this time.

This commit allows the custom_filter and custom_display parameters to be used separately. It also splits up the widget method in civievent-widget.php to make it easier to understand.

I'm sure that more work is required before this can be merged, but I wanted to get feedback before going any further.

[agh1]

This sounds great. Originally, the plugin just relied upon CiviCRM's public event list, letting it determine whether an event is in the past and so forth. It's also one line of code. Later, folks have wanted more nuanced filtering, and the API is the way to make that work.

I think it may be time (now or soon) to switch to using the API exclusively and just implement standard params. It looks like this is what you're doing, so that's very much welcome.

I think the key things to watch out for are:

• making sure no reconfiguration is needed for existing users
• watching out for time zone issues: make sure the behavior is the same as the existing basic mode when the server time, PHP time, and WordPress site time differ
• keeping configuration simple for most people

Let me know when you're at a point you'd like more thorough review, and I'd be happy to try it out.

[smartacus]

You are correct in your assessment of what this change does. It starts with a base set of API params and layers on additional params that are parsed from the shortcode.

The shortcode parameters are essentially the same with the following exceptions:

• admin_type is not used in the simple/custom display evaluation and may not be necessary after this change. This change should not affect current users.
• custom_filter works independently from admin_type (as mentioned above) and custom_display. Existing users using all three parameters should not be affected.
• event_type_id is essentially a shortcut to adding the event_type_id as a custom_filter parameter. This preserves the existing functionality for backwards compatibility.

I didn't make any changes around time zones. I would only expect there to be issues if civicrm_api3 and CRM_Event_BAO_Event::getCompleteInfo return times differently.

As far as I know, the only remaining work is testing and updating the documentation. I can tackle both of those, as well as fixing any bugs, if you could give it a try.

Thanks!

[agh1]

The issue is that CRM_Event_BAO_Event::getCompleteInfo() goes down to the second as far as determining whether an event is in the past--the default API params in the custom filter, which you use as the default for everyone just look for the current day.

This is not a huge deal except people are really sensitive about the time filter for showing events. I've been able to punt the issues in the past by saying, "well, we just do what CiviCRM does". Handling it through the API means this plugin is responsible for getting it right.

(The fundamental problem is that CiviCRM, and CiviEvent in particular, is in denial about time zones. If you're a national or international organization, a noon event in DC and a noon event in LA are treated as if they're simultaneous.)

But yeah, the rest of this week is pretty busy for me, and I'm on vacation next week, but I will certainly review your code and try it out after that. Thanks again!

[twomice]

@agh1 Do you still have concerns about down-to-the-second vs same-day calculations as in this context?

[twomice]

FYI, I'm reviewing this and trying it out now, hoping to be able to recommend merging.

(oops, this was just supposed to be a comment not a review. anyway...)

[twomice]

Using array_merge_recursive() is problematic here because it's not smart enough to know when to merge and when to replace. For example:

$defaultParams = array(
  'start_date' => array('>=' => '2019-03-19'), // hard-coding today's date for example purposes.
  // ... omitting other params for simplicity ...
);
$customFilterParams = array(
  'start_date' => array('>=' => '2020-01-01'),
);
var_dump(array_merge_recursive($defaultParams, $customFilterParams));
/* Ouput:
 array(1) {
  'start_date' =>
  array(1) {
    '>=' =>
    array(2) {
      [0] =>
      string(10) "2019-03-19"
      [1] =>
      string(10) "2020-01-01"
    }
  }
}
 * This will cause civicrm api to ignore the 'start_date' param entirely.
*/
$customFilterParams = array(
  'start_date' => array('=' => '2020-01-01'),
);
var_dump(array_merge_recursive($defaultParams, $customFilterParams));
/* Ouput:
array(1) {
  'start_date' =>
  array(2) {
    '>=' =>
    string(10) "2019-03-19"
    '=' =>
    string(10) "2020-01-01"
  }
}
 * This will cause civicrm api to return events matching EITHER of the 
 * defined 'start_date' criteria.
*/

I did have some hope for array_replace_recursive(), but it's also not smart enough, and will also result in weird combinations.

I think the only way to overcome this is with an additional method to merge these parameters explicitly to get the intended parameters.

[twomice]

OK, actually, I think there's hope for simply array_merge() here. It resolves the above-mentioned issues, but not sure if it causes other issues.

[twomice]

[I'm removing this comment, as it merely described an issue that was caused by confusion over html entities. Oops.]

[twomice]

As in #7 (comment), we need to wrap this in html_entity_decode().

[twomice]

I think we have to wrap this in html_entity_deccode(). This is because you can't use actual angle-brackets for things like "greater-than-or-equal-to"; normally this operator would be >=, but if you put that in the shortcode, it breaks the page. (See https://codex.wordpress.org/Shortcode_API#HTML which says "The suggested workaround for HTML limitations is to use HTML encoding for all user input, and then add HTML decoding in the custom shortcode handler."). So users need to use entities, e.g., >=, and we need to run json-based attributes like custom_display through html_entity_decode() to support these operators.

[twomice]

@agh1 and @smartacus I'm finally understanding that this all should have been a review instead of serial comments, but anyway, I've completed my review and there are only these issues that I think should be addressed before merging:

1. Use of array_merge_recursive() causes bad api parameter values for filter operators that are arrays (e.g., start_date: {">=": "2019-01-01"}. Probably using array_merge() instead will fix that, but someone should think about testing edge cases.
2. Shortcode attributes that support JSON (custom_filter and custom_display) may need to contain html code characters such as < or >, which is not supported by WP. We should update the README to suggest the use of html entities like >, and pass those values through html_entity_decode() before running them through json_decode().

FYI, I'm about to install this PR -- with the above-mentioned changes -- on a live site, and will report back here if further issues are found.