Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transformers: Migrate from wp_object to items #1165

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

obenland
Copy link
Member

Non-functional changes that split out the property updates around wp_object => item from #593.

Non-functional changes that split out the property updates around wp_object => item from #593.
@obenland obenland added the Skip Changelog Disables the "Changelog Updated" action for PRs where changelog entries are not necessary. label Jan 14, 2025
@obenland obenland requested a review from a team January 14, 2025 22:26
@obenland obenland self-assigned this Jan 14, 2025
@github-actions github-actions bot added [Focus] Compatibility Ensuring the plugin plays well with other plugins labels Jan 14, 2025
@obenland
Copy link
Member Author

@pfefferle Can you share the why behind these changes?

I remember that there were some prohibited variable names we had to deal with when we did all the PHPCS stuff, is this an extension of that? I think it took offense with $object, $wp_object should be fine?

@pfefferle
Copy link
Member

This is because with the new transformers it does not have to be a WordPress object any more.

Copy link
Member

@pfefferle pfefferle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If @Menrath is fine, I am too!

@Menrath
Copy link
Contributor

Menrath commented Jan 15, 2025

I have some doubts about this change and try to explain why, but in order to do so I have give a more general answer, also targeting PR #593. So this is kind of a mixed review, I hope that is O.K.

I wonder whether item is the best term (in terms of readability and technical specificity), not just here, but especially in the context of the PR adding the outbox collection.
The ActivityPub specification describes the outbox stream as containing activities that the user has published. Consequently, I would suggest using the name activity instead of item within the outbox collection, as it aligns better with the specification and improves clarity.

For the transformer, I believe wp_object is a quite fitting name. It reflects that the entity represents a WordPress construct, such as a WP_Post, WP_Comment, WP_Term, or potentially other types. item, on the other hand, does not convey this relationship as clearly. Perhaps one could also update the PHPDoc comment for the transformer to include mixed, as this would make the expected (?) flexibility of the wp_object type more explicit.

So when I put this together: The transformer classes take care of the object which may be set in an Activity (outbox collection item).

The wording item in the Shortcodes class is fine and clear enough for me. It’s not wrong per se, as it is only used locally within that function, and the PHPDoc provides sufficient

I will take some time tomorrow to review the current status of the outbox PR in detail. So far, I’ve only taken a quick look at it.

Off topic: I want to point out the naming of the namespace Activitypub\Activity. When I first started working with this plugin, it confused me quite a bit. The namespace contains all kinds of object types, while Activity is a specific subtype of the generic ActivityPub object (e.g., Activitypub\Activitypub, Activitypub\Object, Activitypub\Object_Types\). That said, I understand this might be a legacy decision, and I think it’s something I can live with. :)

@obenland
Copy link
Member Author

Thank you for your thoughtful feedback!

We can't use object since it's a reserved word in PHP and wp_object is no longer an accurate description of the content. activity_object, which came up previously, is ambiguous with Activities being Objects themselves. I'm kind of at a loss for a more semantic name for it. :(

Building on your PHPDoc suggestion, is this problem maybe something that could be "solved" with more verbose docs for these properties?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Compatibility Ensuring the plugin plays well with other plugins Skip Changelog Disables the "Changelog Updated" action for PRs where changelog entries are not necessary.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants