Jetpack: Recoverable fatal error: jetpack/modules/infinite-scroll/infinity.php on line 1763

[mahnunchik]

Steps to reproduce the issue

1. Enable Infinite Scroll -> Load more posts as the reader scrolls down
2. Install and enable AMP plugin https://amp-wp.org/ in Standard mode.
3. Load any page
4. See error

What I expected

No error

What happened instead

Recoverable fatal error: Object of class Closure could not be converted to string in /var/www/html/wp-content/plugins/jetpack/modules/infinite-scroll/infinity.php on line 1763
There has been a critical error on your website.

Jetpack: 9.0.2
AMP: 2.0.4

[mahnunchik]

Load more posts in page with a button the same error.

[jeherve]

Thanks for the report. Could you let me know what theme you experienced this with?

[mahnunchik]

Theme based on https://github.com/automattic/_s

/**
* Add theme support for infinite scroll.
*
* @link https://jetpack.com/support/infinite-scroll/
*/
add_theme_support( 'infinite-scroll', array(
    'type' => 'click',
    'wrapper' => false,
    'render' => function () {
        while ( have_posts() ) {
            the_post();
            get_template_part( 'template-parts/content', 'blog' );
        }
    },
) );

[jeherve]

Is your theme available for download, so I can give it a quick test?

Thanks!

[mahnunchik]

It isn't available but it doesn't have a big differences with _s

[jeherve]

I've tried reproducing with a new theme built off _s, but I've had no luck.

Do you have any additional Infinite Scroll customizations in place, using filters such as infinite_scroll_settings maybe?

[anomiex]

@jeherve: The problem here is a combination of two things.

1. The code quoted in https://github.com/Automattic/jetpack/issues/17461#issuecomment-707222366 is setting the 'render' property to a Closure. That seems ok as far as the documentation for the 'render' property goes: it calls for a "function", and that's what it's getting.

2. In The_Neverending_Home_Page::amp_load_hooks(), the code is assuming the 'render' function is a string, or is stringifyable:

   $template = self::get_settings()->render;
   
   // ...
   
   if ( is_callable( "amp_{$template}_hooks" ) ) { // ← line 1763
           call_user_func( "amp_{$template}_hooks" );
   }
   

   That fails when 'render' is closure. It'll also fail for an array callable (e.g. array( 'ClassName', 'method' ) or array( $obj, 'method' )). It may also fail more subtly for a string referencing a class static method (e.g. ClassName::method) rather than a global function, as the theme would have to do some particularly weird stuff to define the amp hooks function in a way that it'll actually be called.

   Also of note is that the code will try calling amp__hooks() if the theme didn't provide a render function at all, which is probably also not intended.

As for what we might do here, I see two options.

1. If by "function" we really mean a string representing a function rather than any other kind of PHP callable, we should add validation to get_settings() to reject any non-string and any string containing :: for 'render' (and 'footer_callback'). That will break any themes that are using some other kind of callable explicitly and immediately, instead of only doing so if used in conjunction with the AMP plugin.
2. Rather than trying to construct another function name by string concatenation, add an additional setting to specify the AMP hook callback explicitly. We could (deprecatedly?) default it for back-compat to the name determined by string concatenation if it's not otherwise specified and 'render' is a string not containing ::.

I see the AMP code was added in #16048 by @pereirinha, maybe they can provide further insight here.

[westonruter]

2. In The_Neverending_Home_Page::amp_load_hooks(), the code is assuming the 'render' function is a string, or is stringifyable:

I just came across this myself. To me this seems like a strange way to implement extensibility. Shouldn't there rather be an action like:

do_action( 'jetpack_initialize_infinite_scroll' );

Then instead of the theme compatibility doing this:

https://github.com/Automattic/jetpack/blob/c663b902b456070da928a82555c68a7838411f46/modules/theme-tools/compat/twentytwenty.php#L164-L174

It could instead do:

add_action( 'jetpack_initialize_infinite_scroll', function() {
    add_filter( 'jetpack_amp_infinite_footers', 'twentynineteen_amp_infinite_footers', 10, 2 );
    add_filter( 'jetpack_amp_infinite_output', 'twentynineteen_amp_infinite_output' );
    add_filter( 'jetpack_amp_infinite_older_posts', 'twentynineteen_amp_infinite_older_posts' );
} );

Or rather, why not just:

add_filter( 'jetpack_amp_infinite_footers', 'twentynineteen_amp_infinite_footers', 10, 2 );
add_filter( 'jetpack_amp_infinite_output', 'twentynineteen_amp_infinite_output' );
add_filter( 'jetpack_amp_infinite_older_posts', 'twentynineteen_amp_infinite_older_posts' );

Why the indirection?

[pereirinha]

Thanks all for the inputs on this.
I'll revise the comments and do some testing, but my first impression is that @westonruter suggestion of having do_action( 'jetpack_initialize_infinite_scroll' ); seems to be the best path.

I'll update this issue as soon as possible.

[westonruter]

I also have started a PR with what I believe is an improved approach to implementing infinite scroll in AMP: https://github.com/Automattic/jetpack/pull/17497.