Add/extendable transformers #569

Menrath · 2023-11-21T12:59:55Z

This draft pull request should for now make it easy for everyone to track the status of the proposal "flexible management of how posts gets transformed to ActivityPub objects"..

It is still under heavy, early development, but questions and discussion are also welcome here.

Proposed changes:

Transformers are now extendable by other plugins via an API.
More details will follow.

Other information:

Will follow.

Testing instructions:

Will follow.

We develop this changes here, if you are interested in following the internal discussions more closely.

…or the wp_post

…sformers also does c35ddf1

pfefferle · 2023-11-21T13:29:17Z

activitypub.php

@@ -33,12 +33,15 @@
 \defined( 'ACTIVITYPUB_CUSTOM_POST_CONTENT' ) || \define( 'ACTIVITYPUB_CUSTOM_POST_CONTENT', "<strong>[ap_title]</strong>\n\n[ap_content]\n\n[ap_hashtags]\n\n[ap_shortlink]" );
 \defined( 'ACTIVITYPUB_AUTHORIZED_FETCH' ) || \define( 'ACTIVITYPUB_AUTHORIZED_FETCH', false );
 \defined( 'ACTIVITYPUB_DISABLE_REWRITES' ) || \define( 'ACTIVITYPUB_DISABLE_REWRITES', false );
+\defined( 'ACTIVITYPUB_DEFAULT_TRANSFORMER' ) || \define( 'ACTIVITYPUB_DEFAULT_TRANSFORMER', 'activitypub/defualt' );


typo

Suggested change

\defined( 'ACTIVITYPUB_DEFAULT_TRANSFORMER' ) || \define( 'ACTIVITYPUB_DEFAULT_TRANSFORMER', 'activitypub/defualt' );

\defined( 'ACTIVITYPUB_DEFAULT_TRANSFORMER' ) || \define( 'ACTIVITYPUB_DEFAULT_TRANSFORMER', 'activitypub/default' );

includes/class-transformer-base.php

pfefferle · 2023-11-21T13:33:15Z

includes/activity/class-event.php

+ *
+ * @see https://www.w3.org/TR/activitystreams-vocabulary/#dfn-event
+ */
+class Note extends Base_Object {


Suggested change

class Note extends Base_Object {

class Event extends Base_Object {

pfefferle · 2023-11-21T13:34:46Z

includes/activity/class-event.php

@@ -0,0 +1,23 @@
+<?php


why these classes? I tried to avoid them, because otherwise we also have to map all other types https://github.com/Automattic/wordpress-activitypub/blob/master/includes/transformer/class-post.php#L398

We are still evaluating all pros and cons. At the moment we have only tried to get an idea of what the consequences would be. As is often the case: the deeper I dig into this repository, the more I can understand certain design decisions.

True, but there are things that are simply legacy decisions! Be aware that this project started as a spare time thing and still has some crappy code everywhere. So do not try to understand everything and feel free to challenge us, that's what I also try here ;)

I discuss to understand, not to win :)

Thank you, for offering help, even at our Drafting Stage. I gladly accept this invitation for discussions :)

One of my goals is, that it is very easy an straight forward, for feature developers adding their own transformers to their plugins. Very likely they will have to extend the Base_Object class anyway, especially if they want to add any custom fields that are not defined yet in our Base_Objet class, which until now is consistent with the Activitystreams Vocabulary Standard, e.g., Mobilizon makes usage of PeerTubes commentsEnabled feature within its Event object which is a object type available in the ActivityPub standard.

Other developers might have a look how thinks are done in this project, and might get confused if the activitypub plugin itself does things a bit differently than the future documentation for how to add proper activitiypub support for my custom post type might advice them to do.

Another related issue is, that when adding a custom field, one also has to add the appropriate json-ld context.
Though the filter activitypub_json_context is available, editing the array directly seemed kind of rough to me and applying the filter within the transformer code leaded to a runtime issue, so I had to apply this filter in a different part of the code, which is not ideal in my opinion: doing one "thing" on two different places.

I will share our repository with some example transformers registering to the ActivityPub-plugin hopefully today, after doing some renaming and initial documentation.

I will share our repository with some example transformers registering to the ActivityPub-plugin hopefully today, after doing some renaming and initial documentation.

That would be perfect!

I agree with your point, but I am not sure if the "extensions" (Event-Activity) should be part of this repo or part of yours. I agree, that it has to be as easy as possible and maybe we have to change the current context implementation for that.

You can maybe have a look what I did to extend the base object(s) for the Follower (https://github.com/Automattic/wordpress-activitypub/blob/master/includes/model/class-follower.php) or internal User mapping (https://github.com/Automattic/wordpress-activitypub/blob/master/includes/model/class-user.php).

pfefferle · 2023-11-21T13:38:06Z

includes/class-transformer-base.php

+		$plugins_data = get_plugins( '/' . $plugin_directory );
+		$plugin_data = array_shift( $plugins_data );
+
+		return $plugin_data['Name'] ?? esc_html__( 'Unknown', 'activitypub' );


please be downwards compatible to at least PHP 7.0!!

follow WordPress coding style and coding style of this plugin

…tion.eu:2222/Event-Federation/wordpress-activitypub into add/extendable-transformers

…t in parent base transformer class

fix: make in_array comparison strict

pfefferle · 2023-11-25T09:55:05Z

includes/transformer/class-transformers-manager.php

+	 * @access private
+	 */
+	private function require_files() {
+		require ACTIVITYPUB_PLUGIN_DIR . 'includes/transformer/class-base.php';


you do not have to require classes, they are all auto-loaded.

pfefferle · 2023-11-25T09:56:06Z

includes/transformer/class-transformers-manager.php

+ * @since version_number_transformer_management_placeholder
+ */
+class Transformers_Manager {
+	const DEFAULT_TRANSFORMER_MAPPING = array(


there are a lot of function/attributes that are already defined by the Base object, or that should be defined by the base object (like the instance method).

pfefferle · 2023-11-25T09:57:23Z

includes/transformer/class-transformers-manager.php

+	public function get_transformer( $object ) {
+		switch ( get_class( $object ) ) {
+			case 'WP_Post':
+				$post_type = get_post_type( $object );


this looks a bit complicated and redundant. the way you implemented it, does not require a switch/case, I think.

pfefferle · 2023-11-25T09:58:40Z

includes/rest/class-outbox.php

@@ -105,7 +105,8 @@ public static function user_outbox_get( $request ) {
 			);

 			foreach ( $posts as $post ) {
-				$post = Post::transform( $post )->to_object();
+				$transformer = \Activitypub\Transformer\Transformers_Manager::instance()->get_transformer( $post );


Suggested change

$transformer = \Activitypub\Transformer\Transformers_Manager::instance()->get_transformer( $post );

$transformer = Transformers_Manager::instance()->get_transformer( $post );

you do not have to use the full path (with namespaces) when using use.

pfefferle · 2023-11-25T10:02:27Z

includes/transformer/class-transformers-manager.php

@@ -0,0 +1,292 @@
+<?php


I would remove the transformers in the name, because this is already defined by the namespace and I would go with factory instead of manager because this is a known pattern.

Thanks for those suggestions. Another naming I thought about is using registry, that is the naming the Gutenberg project uses for its blocks.

pfefferle · 2023-11-25T10:04:56Z

@Menrath thanks for your work, I really appreciate it!

I added some feedback, and just wanted to mention to please take it as "feedback" and not as "must be changed"! I simply would love to start a discussion about some things!

pfefferle · 2023-11-27T08:11:40Z

includes/transformer/class-transformers-manager.php

+	 *
+	 * @return bool True if the ActivityPub transformer was registered.
+	 */
+	public function register( \ActivityPub\Transformer\Base $transformer_instance ) {


maybe also worth having a look: https://github.com/pfefferle/wordpress-webmention/blob/main/includes/functions.php#L12

We tried to copy the core functionality of WordPress' Custom Post Type Registry.

And here is how we register the default types: https://github.com/pfefferle/wordpress-webmention/blob/main/includes/class-comment.php#L89

I did not take a similar approach in the first place because I wanted to avoid using a global variable.

Menrath · 2023-12-12T12:56:06Z

As this Pull request is prepared by two persons at once and things at the moment change quite frequently, we thought it would be best to keep minor changes in a seperated dev branch. We will keep merging and squashing better understandable packages in the branch of this pull request soon.

Menrath · 2023-12-22T10:37:24Z

@pfefferle I decided to close this PR and am currently preparing a new one after reviewing b744dc5. It'll keep the commit history cleaner. And at the current state for me it's easier to go through all the changes again manually than merging dozens of files.

My aim is to keep the filter for applying the transformer as you used it as a fallback, but keep the transformer setting in the admin as proposed here, with a new UI though.

I you disagree I can arrange with that. However, what is most important to me is that the admin-users see in the settings page which transformer is used and which plugin provided it.

If you manage to give some short feedback before christmas break that would be awesome!

Menrath added 7 commits November 20, 2023 18:15

initial draft: add/extendable-transformers

38be586

fix the wrongs calld of the get_transformer

3ae71bb

Automatically set the wp_post when returning a transformer instance f…

6d40ebf

…or the wp_post

fix: add missing self prefix of class constant

0ccb6c9

add: untestet draft of the migration for the activated post type setting

246600b

update phpdocs

2113d3e

Merge remote-tracking branch 'origin/master' into add/extendable-tran…

0501fc5

…sformers also does c35ddf1

github-actions bot added [Status] In Progress OSS Citizen labels Nov 21, 2023

pfefferle reviewed Nov 21, 2023

View reviewed changes

includes/class-transformer-base.php Outdated Show resolved Hide resolved

pfefferle reviewed Nov 21, 2023

View reviewed changes

pfefferle and others added 16 commits November 21, 2023 15:25

Merge branch 'master' into add/extendable-transformers

3a5b530

transformer: move all related code to \Activitypub\Transformer namespace

369b32b

transformer: move all related code to \Activitypub\Transformer namespace

c857eee

typo

0bd6ecc

fix phpcs errors

804cb0a

change hook name for registering transformers

ed1b6b7

follow WordPress coding style and coding style of this plugin

Merge branch 'master' into add/extendable-transformers

75f1da1

Merge branch 'add/extendable-transformers' of ssh://code.event-federa…

2a79297

…tion.eu:2222/Event-Federation/wordpress-activitypub into add/extendable-transformers

fix creation of new transformer instance

81f971b

fix: make in_array comparison strict

8338ea4

cleanup: remove function get_post_content_template that is now presen…

47b7d84

…t in parent base transformer class

fix: forgotten merge conflict

d25a5c2

fix phpdocs

82928fb

fix init of new transformer instance

9c20c8f

move transofmrer function get_post_content_template to base class

768d190

fix: make in_array comparison strict

fix phpdocs and some readability

e5d19f2

fix phpcs via phpcbf

c7ff99e

pfefferle reviewed Nov 25, 2023

View reviewed changes

pfefferle reviewed Nov 27, 2023

View reviewed changes

Menrath closed this Dec 22, 2023

github-actions bot removed the [Status] In Progress label Dec 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add/extendable transformers #569

Add/extendable transformers #569

Menrath commented Nov 21, 2023 •

edited

pfefferle Nov 21, 2023

pfefferle Nov 21, 2023

pfefferle Nov 21, 2023

Menrath Nov 21, 2023

pfefferle Nov 25, 2023

Menrath Nov 27, 2023 •

edited

pfefferle Nov 27, 2023

pfefferle Nov 21, 2023

pfefferle Nov 25, 2023

pfefferle Nov 25, 2023 •

edited

pfefferle Nov 25, 2023

pfefferle Nov 25, 2023

pfefferle Nov 25, 2023

Menrath Nov 27, 2023

pfefferle commented Nov 25, 2023

pfefferle Nov 27, 2023

Menrath Nov 27, 2023

Menrath commented Dec 12, 2023

Menrath commented Dec 22, 2023 •

edited

	\defined( 'ACTIVITYPUB_DEFAULT_TRANSFORMER' ) \|\| \define( 'ACTIVITYPUB_DEFAULT_TRANSFORMER', 'activitypub/defualt' );
	\defined( 'ACTIVITYPUB_DEFAULT_TRANSFORMER' ) \|\| \define( 'ACTIVITYPUB_DEFAULT_TRANSFORMER', 'activitypub/default' );

	class Note extends Base_Object {
	class Event extends Base_Object {

	$transformer = \Activitypub\Transformer\Transformers_Manager::instance()->get_transformer( $post );
	$transformer = Transformers_Manager::instance()->get_transformer( $post );

Add/extendable transformers #569

Add/extendable transformers #569

Conversation

Menrath commented Nov 21, 2023 • edited

Proposed changes:

Other information:

Testing instructions:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Menrath Nov 27, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfefferle Nov 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfefferle commented Nov 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Menrath commented Dec 12, 2023

Menrath commented Dec 22, 2023 • edited

Menrath commented Nov 21, 2023 •

edited

Menrath Nov 27, 2023 •

edited

pfefferle Nov 25, 2023 •

edited

Menrath commented Dec 22, 2023 •

edited