Compatibility with Query Monitor

[christianwach]

Hi @stephenharris this is really just a FYI because I'm not sure whether the fix should be in Event Organiser or in Query Monitor by @johnbillion.

The issue is that when Query Monitor is active, it is impossible to add a callback for the template_include hook prior to Event Organiser's callback and therefore plugins cannot replace the single Venue template.

What's happening is that Query Monitor adds a callback with priority PHP_INT_MAX and then Event Organiser's highest_priority() method adds one to that. I wasn't sure what would happen in that situation, but it turns out that this happens:

WP_Hook Object 
(
  [callbacks] => Array 
  (
    [-9223372036854775808] => Array                                          <-- The Event Organiser callback
    (
      [00000000262713dd00000000666ec44ftemplate_include] => Array 
      (
        [function] => Array 
        ()
        [accepted_args] => 1
      )
    )
    [9223372036854775807] => Array                                           <-- The Query Monitor callback
    (
      [000000002627132900000000666ec44ffilter_template_include] => Array 
      (
        [function] => Array 
        ()
        [accepted_args] => 1
      )
    )
  )
)

So it appears that the Event Organiser callback priority is set to -9223372036854775808 😕

For a temporary solution for people who want to use both Query Monitor and override the Venue template, here's an example class that modifies Query Monitor's behaviour to allow the template_include hook callback to do what it's supposed to do:

class EO_Venue_Theme_Mods {

    /**
     * Constructor.
     */
    public function __construct() {
        add_action( 'template_include', [ $this, 'template_include' ] );
        add_action( 'template_redirect', [ $this, 'template_redirect' ], 11 );
    }

    /**
     * Modifies the template include for EO Venues.
     *
     * @param array $template The found template.
     * @return array $template The modified template.
     */
    public function template_include( $template ) {

        // Defer if a template has already been found.
        if ( ( is_tax( 'event-venue' ) || eo_is_venue() ) && eventorganiser_is_event_template( $template, 'event-venue' ) ) {
            return $template;
        }

        // Use our template for Venues.
        if ( is_tax( 'event-venue' ) || eo_is_venue() ) {
            $template = 'path/to/taxonomy-event-venue.php';
        }

        // --<
        return $template;

    }

    /**
     * Modifies the Query Monitor "template_redirect" hook priority.
     *
     * This allows EO to hook in using its "highest_priority" method.
     */
    public function template_redirect() {

        // Bail if Query Monitor class is not present.
        if ( ! class_exists( 'QM_Collectors' ) ) {
            return;
        }

        // Get Query Monitor collector.
        $qm = QM_Collectors::get( 'response' );
        if ( ! $qm instanceof QM_Collector_Theme ) {
            return;
        }

        // Reduce the Query Monitor "template_redirect" hook priority.
        remove_filter( 'template_include', [ $qm, 'filter_template_include' ], PHP_INT_MAX );
        add_filter( 'template_include', [ $qm, 'filter_template_include' ], PHP_INT_MAX - 10 );

    }

}

The question, I guess, is whether Query Monitor ought to reduce its callback priority from PHP_INT_MAX to something lower or whether Event Organiser should detect that there's a callback with priority PHP_INT_MAX and choose a lower value?

Cheers, Christian

[johnbillion]

Yikes, that's an integer overflow.

I think the best option here is a fight to the death between these plugins for highest_priority() to account for PHP_INT_MAX and avoid the overflow, otherwise the same problem will occur with any other plugin that uses PHP_INT_MAX.

FYI Query Monitor uses PHP_INT_MAX here so it's sure that it's reporting the correct template files and template hierarchy. It looks like Event Organiser adjusts the template so QM does need to run later otherwise it'll report the incorrect template for Event Organiser.

[christianwach]

@johnbillion Thanks for chiming in. Given what you say, I agree with you that Query Monitor's callback ought to come after Event Organiser's... it just wasn't clear to me how to alter Event Organiser's callback, so I altered Query Monitor's :)

It seems that Event Organiser should indeed detect that there's a callback with priorityPHP_INT_MAX and choose a lower value. I can't see it making a great deal of difference to Event Organiser's functionality if it chooses PHP_INT_MAX - 1.

[stephenharris]

@christianwach Good spot. I'll publish a fix. It clearly didn't occur to me that I could just use PHP_INT_MAX rather than trying to out-bid all the other callbacks :)

[christianwach]

That'd be great @stephenharris - thanks 👍

[stephenharris]

Should be fixed in 3.12.3

[christianwach]

@stephenharris I realise this issue is closed but I can't figure out where your recent commits are to raise a new issue that points to the problematic code. So here's my FYI...

It looks as though your fix for this issue left a few stray references to $priority in the template_include() method. These references leave PHP Warning: Undefined variable $priority log entries. Lines 190 and 213.

Cheers, Christian