Improve transformer

[janboddez]

Adds "improved" error handling to code that uses the Transformer class(es). (Really, all it does is add a bunch of is_wp_error( $transformer ) checks!)

(The current version of the plugin has some checks in place, but not enough to prevent fatal errors down the line, should one ever attempt to pass an invalid object or transformer [to the rest of the code].)

As a bonus, it allows somewhat experienced PHP developers to exclude certain posts (and, potentially, comments) from federation. (Which is an oft-requested feature!)

Proposed changes:

• Add better "transformer error" handling to JSON templates and ActivityPub dispatcher
• Add better "transformer error" handling to outbox and collection endpoint
  • As a result, "method chaining" is no longer possible (I think); I'd suggest using the "Factory" everywhere, make things a bit simpler
    • If this is somehow not an option, then Activitypub\Transformer\Base::transform() has to become filterable, too (to prevent "unwanted" posts from showing in outboxes)
      • This PR currently includes these changes, but they could be scrapped if this function is no longer used
    • If it is, maybe deprecate this function in favor of Factory::get_transformer()? 😬

Other information:

• As mentioned above, this could, potentially, be used to exclude posts from federation.
• While I see some challenges (related to #668) using the filter for that purpose, that's ... a different issue, really. (Probably should not make things this confusing ... but I wanted to mention it regardless.)
• The only "real" downside I see is that by allowing outboxes and collections to be filterable, that the per-page counts will no longer "be correct" (But only if such a filter is actually in use. Also, I'm not sure if any software currently relies on outboxes, or if they expect a page to hold a certain number of items always.)
  • Reason for this is filtering happens after fetching the items from the database ... (There's currently no way to filter "outbox queries." Unless maybe we use the federated custom field for that.)
  • So that could be a reason to leave (only) the outbox/collection endpoints as they currently are.

• [ ] I did not run or add automated tests (no Docker on this machine, although I don't expect problems, the code does not change the plugin's default behavior) 😬
• [x] I absolutely tested in production

Testing instructions:

I'm going to just outline what I did (and do):

• Add an mu-plugin like this one:

<?php

add_filter( 'activitypub_transformer', function ( $transformer, $obj ) {

	if ( $obj instanceof \WP_Post && in_category( 'disable-activitypub', $obj ) ) {
		return new \WP_Error( 'unsupported_object', __( 'This object does not support ActivityPub.', 'activitypub' ) );
	}

	return $transformer;

}, 10, 2 );

• Create a new post, and make sure it has the disable-activitypub category
• Save it as draft, to ensure Gutenberg has applied this category before "federation" is scheduled
• Publish the post
• Watch no outgoing POST requests happen
• Finally, try searching for the post's URL in, e.g., Mastodon. Nothing should show up.
• Try visiting the ?activitypub version of the post, in your own browser. You should "simply" see the post, not JSON.
• If you used such a mu-plugin (i.e., an activitypub_transformer filter that does not return an actual transformer) with the currently released version of the plugin, you'd instead get a fatal PHP error.

The mu-plugin can "easily" be adapted to "exclude" post formats, posts with certain custom fields, and so on. (The only requirement is that these are stored before scheduling runs, which could be tricky when you use the block editor. As per the linked issue above.)

[janboddez]

In the future, one could envision a per-post checkbox to disable federation, and store its value in a custom field. You'd then "simply" have to look for that in an activitypub_transformer callback.

(The issue there would be to get Gutenberg to save the checkbox value before ActivityPub requests are scheduled, which is what #668, even if it currently isn't a problem, is all about.)

It could even be used to fix #631. (In that issue, I describe being able to federate a private post, simply because, well, it was made private after it got scheduled but before federation happened.) "Simply" use a activitypub_transformer callback to ensure a post isn't private.

[janboddez]

This has one "side effect," potentially. If a post's transformer is somehow invalid (e.g., because it was filtered and a site author wanted to exclude certain posts from "federation"), then it'll be skipped, while it was returned by the query. So your "page count" wouldn't match the actual number of items on that "page." I don't think this is a huge issue, though. (Still, if we could somehow exclude items during the "query," rather than skip over them while looping, that'd be even better.)

This could be fixed by allowing the get_posts() arguments of the outbox and collection queries to be filterable. (Technically one could already use pre_get_posts and somehow detect that we're in an endpoint.)

If someone wanted to disable a certain post format, they need to disable active federation but also conneg for the posts in question (this could be done, at least for now, by filtering the transformer as outlined above).

They would also have add a tax_query to the get_posts() used in the collection and outbox queries.

Anyway, this PR isn't so much about disabling federation; it just catches possible errors. :-D

[janboddez]

OK, I'm not too familiar with GitHub reviews and (un)resolving conversations, but:

• I removed the check from the Base::transform() function. This also means that it should probably never be used anymore. (I think it's safe to remove, in fact, as I replaced all calls with Factory::get_transformer() instead, which should be 100% compatible.)
• I removed the checks from the templates. That means the transformer still gets called twice, but it's (for now) the only way I see. (The "trick" I used to test still works, ;-) https://jan.boddez.net/notes/f8dab23f83?activitypub)
  • The reason I think it should go in the filter is that it's "too late" to still return HTML from the template. Or that trying to do so would be much, much messier.

[pfefferle]

It thought more about removing it from the "controller" and simply load the template and throw an error there if there is one? this way we have to only call it once! To hacky?

[janboddez]

It thought more about removing it from the "controller" and simply load the template and throw an error there if there is one?

I don't know. While that would work, most other web servers would simply return HTML, if they didn't "understand" application/activity+json requests. Seems "nicer" to me.

Although now you basically instantiate two transformers (and only use one), I'm not sure what the real overhead is in terms of CPU or memory consumption. I'd say it's pretty small. I also thought about using the cache or even a transient, but I think that will be way worse in terms of performance.

If you went the error route, I would personally be tempted to create a workaround in a mu-plugin or so.

[janboddez]

I also thought about using the cache or even a transient, but I think that will be way worse in terms of performance.

Oh, maybe we can use that trick where you call a helper function (\Activitypub\current_transformer() or something, just thinking out loud) to set a static variable, and then call it without argument later on to retrieve the value.

Since you know the transformer is indeed valid and the template is about to be loaded, it should be absolutely safe to store it for the rest of the request. That way you'd instantiate only one transformer, and you could do it before the template actually loads.

[pfefferle]

Like the singleton approach?!?

[janboddez]

Like the singleton approach?!?

That would cause collections to fail, I think. More like this: https://github.com/Automattic/wordpress-activitypub/blob/f402e842ff20cd06a1008a20a70c6796ddb3dc56/includes/functions.php#L859-L873