Compatibility with WP 5.8

[johnbillion]

https://core.trac.wordpress.org/ticket/50105

I haven't had time to investigate whether AMF needs to handle the change. See comments toward the end of the discussion.

cc @roborourke FYI

[roborourke]

Ah neat, cheers for the heads up. It's a good change and looks like we probably will need to handle it, or just assume there's always another page.

[t-fritsch]

Hi,

I came here after posting this comment https://github.com/humanmade/amf-wordpress/issues/12#issuecomment-915240934 in amf-wordpress repo since it looks like my problem is more AMF-related 🙂

Do you have any plans on supporting WP 5.8+ ?

I wonder if returning the whole response object instead of only the response body in Provider::remote_request() could be a solution to the infinite scrolling removal, or if if would break too many things in linked projects ?

https://github.com/humanmade/asset-manager-framework/blob/58d08f19e55d67a4fc8b9b0351a00058f4cb482c/inc/Provider.php#L223

[roborourke]

Do you have any plans on supporting WP 5.8+ ?

Absolutely, we just need to work out how best to handle it in a straight forward and generic manner. We're still pre v1 release so it's fine to make breaking changes if that's what the best solution involves.

One option I can think of is to add optional methods to a provider that receive the full response body and return / extract the pagination data eg. $this->get_total() etc...

[t-fritsch]

good news ! Do you have any ETA on this ? Any luck this comes before the end of october ? 😅

I haven't digged into code and packages that rely on AMF, but maybe that returning the whole response object (the one that wp_remote_request returns) could be more consistent with the WP api (this is what wp_remote_request does) and with the Provider method name (remote_request), what do you think ? \ If someone needs to only get the response body, he could then do something like :

$response = $this->remote_request( $url, [ ... ]);
$responseBody = $response['body'];

[roborourke]

Returning the whole response body would still require provider implementors to manually set the appropriate pagination headers so I feel it'd be easier to work with if that were abstracted away a bit, it'd allow us to enforce return types etc...

[roborourke]

Hm, can't rely on the remote_request() helper being used by all providers, AMF Unsplash for example has its own fetch method. The request_items() method is the one that will absolutely get called, and that calls the abstract request() method...

request() currently returns a MediaList but perhaps we can add a properties object to enforce the following return structure:

class MediaResponse {

    private $media_list;
    private $total;
    private $per_page;

    function __construct( MediaList $media_list, int $total, int $per_page ) {
         $this->media_list = $media_list;
         $this->total = $total;
         $this->per_page = $per_page;
    }

    // getters for each property?

}

Then the request_items() method can set the headers accordingly. Thoughts on that approach @tfrommen @johnbillion ?

[tfrommen]

@roborourke I would have to take a look at all the code myself, which I could do next Monday, but I completely agree with what has been said before. We should abstract anything that is common (or would be beneficial to any implementation, provider, use case), and, if possible, not require refactoring all the things.

Changing the response to something that's more complete sounds good. But I haven't checked how WordPress (5.8+) handles the response and what it expects where, so can't comment on the best way to do that yet.

[roborourke]

Cheers @tfrommen, so the specifics are that the media library ajax response now returns X-WP-Total and X-WP-TotalPages headers (in keeping with REST API responses) to enable the "Load more" button now in the media modal. This replaces the old infinite scroll implementation.

We need to be able to consistently set these headers. I feel like a breaking change and a minor version bump is acceptable at this juncture to provide a robust solution.

[roborourke]

This is now complete, thanks to @tfrommen for the reviews. I have made the following releases with WordPress 5.8 support:

• Asset Manager Framework 0.13.0
• AMF WordPress 0.3.0
• AMF Unsplash 0.3.0