cuny-academic-commons / bp-event-organiser

Allows Event Organiser plugin events to be assigned to BuddyPress groups and generates a group calendar page for each group
GNU General Public License v2.0
3 stars 1 forks source link

Fix event content filter swap #60

Closed christianwach closed 4 years ago

christianwach commented 6 years ago

This PR does two things:

  1. Fixes the removal of the Event Organiser filter on the_content which cannot be done on the template_include filter since EO always adds its callback with the highest possible priority.

  2. Allows plugins to restore the default Event Organiser content filter callback via the introduction of a new action called bpeo_removed_default_canonical_event_content. See the action's docblock for the simplest way to do so.

r-a-y commented 6 years ago

@christianwach - Couldn't we just bump our 'template_include' priority to a higher number? What's EO's priority at?

Update - Ahh I see what EO is doing. Weird stuff indeed. Will take a closer look.

r-a-y commented 6 years ago

Let's just use an anonymous callback function instead of introducing a new function and hook:

add_action( 'get_header', function() {
    remove_filter( 'the_content', '_eventorganiser_single_event_content' );
} );

Let me know if that works for you, @christianwach.

christianwach commented 6 years ago

@r-a-y TVM for reviewing this so promptly - much appreciated!

As you've presumably gathered from the ahem unusual way in which I constructed this, I found it to be a bit of a conundrum as to what the best implementation might be.

I introduced bpeo_removed_default_canonical_event_content because the callback from the_content is conditionally applied at the next available hook. I thought the intention of this (as well as the implementation of cancelling the callback switch) would be clearer than with the anonymous callback route. I'm not sure whether you suggested that before following my link to EO's $this->highest_priority( 'template_redirect' ) priority setting, so will hold fire on any further commits at this stage.

TIA for your thoughts on this.

r-a-y commented 4 years ago

Just looking over this one again. Thanks for opening this PR and sorry for the long gap in responses, @christianwach!

I couldn't really verify point 1 on the latest version of EO (3.9.1).

Regarding point 2, there are two ways you can restore EO's default event content:

  1. add_filter( 'bpeo_enable_replace_canonical_event_content', '__return_false' );

https://github.com/cuny-academic-commons/bp-event-organiser/blob/d3bd473b5d72e8a75384f44ae4cde8c5cdf7bc72/includes/functions.php#L431-L434

  1. remove_filter( 'template_include', 'bpeo_remove_default_canonical_event_content', 20 );

https://github.com/cuny-academic-commons/bp-event-organiser/blob/d3bd473b5d72e8a75384f44ae4cde8c5cdf7bc72/includes/functions.php#L443

I'm going to close this out, but feel free to reply if you have any other comments.

r-a-y commented 4 years ago

I was able to duplicate point 1 after testing on a non-bp-default theme.

To address this, I basically went with the anonymous function method I mentioned here.

One thing this PR has is the 'bpeo_removed_default_canonical_event_content' hook. I don't really think this is necessary since you can remove BPEO's content modification as I outlined above. However, let me know if I might be misintepreting anything from the PR!