Polyfill the core comment type registry - #3682
Draft
pfefferle wants to merge 9 commits into
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this tries
WordPress core is adding a comment type registry,
register_comment_type()and friends (WordPress/wordpress-develop#12311, Trac #35214), and one accessor for the comment types that listings, counts and feeds exclude by default,wp_get_default_excluded_comment_types()(#12310, Trac #65537). Both are still open.This plugin, Webmention and ATmosphere each keep their own copy of that idea. Webmention's
Comment_Typeclass is a near clone of core'sWP_Comment_Type, down to theregister_{type}_comment_type_argsfilter name. So the question was whether to polyfill the core API and have one implementation. I think yes, and this branch is the proof.How it fits together
includes/polyfill/carriesWP_Comment_Typeand the seven core functions from the two PRs, each behind aclass_exists()/function_exists()guard. On a WordPress that has them, the guard skips the copy. On one that does not, the copy is used. The plugin only ever calls the core names, so nothing changes on the day core lands. Once it has, the two files are dead code and can be deleted.Activitypub\register_comment_type()is now a mapping layer, not a shim: it translates the plugin's registration format onto core's (labeltolabels.name,singulartolabels.singular_name) and passes everything else through. It is not deprecated, it is the thing third parties keep calling.Comment::get_comment_types()reads the core registry back and hands callers the array shape they always got, so the twenty readers of that array are untouched.The
reactionflagA like, a repost or a quote is not a comment in the thread. That is a property of the type, so it is a registration arg:
reaction => true. It names the behaviour, not a widget (an earlier draft saidfacepile) and not a plugin or protocol (a later one saidactivitypub), so Webmention's and ATmosphere's likes would carry the same flag, and a future ActivityPub type that genuinely is a comment would not.Because the flag is scalar,
get_comment_types( array( 'reaction' => true ), 'objects' )finds them with core's own filter. That replaced thirteen ad-hocarray_filter()closures over theactivity_typesarray, and a private helper that only renamed the core call.A reaction is not
internal. Core'sinternalis for types likenote, the editor's private annotations, that are shown nowhere. A like is a public comment type that is shown as a reaction rather than in the thread. Two earlier drafts got this wrong, one by registering reactionsinternal => true, one by settingpublic => false, and both would have told core reactions are hidden content. Core provides the right tool instead: thedefault_excluded_comment_typesfilter exists precisely so a plugin can keep its types out of the comment section without misdescribing them. The plugin hooks it once and addsget_comment_types( array( 'reaction' => true ), 'names' )to the set. That is the whole exclusion, in core's vocabulary, and it survives the migration untouched.The accessors
Comment::had six registry accessors. They split three ways:is_registered_comment_type(),get_comment_types(),get_comment_type_slugs()andget_comment_type()each have a core equivalent (comment_type_exists(),get_comment_types( array(), 'objects' ),get_comment_types(),get_comment_type_object()). All four are now deprecated with_deprecated_function()and implemented on the core registry, so they keep working for third parties while telling them where to go. Every internal caller, 19 sites across 7 files plus the reactions block, was moved to the core API in the same commit, so the plugin never warns about itself. There is no private helper for "our types":get_comment_types( array( 'reaction' => true ), 'names' )is the lookup, inlined at each site, because core's own filter is the helper.get_comment_type_by_activity_type()andget_comment_type_attr()have no core equivalent, theactivity_typeslookup is ours. They stay.One semantic change from the deprecation worth knowing:
is_registered_comment_type( 'comment' )used to be false, since the plugin never registeredcomment. It now answers from the shared registry, so it is false on the polyfill and true on core, wherecommentis a built-in. The test pins exactly that.The exclusions, and the facepiles
The hand-rolled
type__not_inexclusions inComment::comment_query()andrest_comment_query()now mergewp_get_default_excluded_comment_types()into whatever another plugin already set, instead of naming our slugs. The reactions are in that set through the plugin's owndefault_excluded_comment_typeshook, so a site can add or remove through the same core filter. Note this branch predates #2981 and the merge here supersedes that fix's bail-on-type__not_inon this branch.The reactions block, the facepiles above the comment list, reads its types from the same registry (
src/reactions/render.php, andbuild/regenerated with it). So the facepiles and the comment list can no longer disagree about what a reaction is: both come from one source, which is what "exclude the facepiles from the comment section" needed to be reliable.Standing down once core reads the set
The hand-applied exclusions are the part that has to disappear when core lands, and they cannot simply keep running. After #12310 the stored comment count fires
pre_wp_update_comment_count_nowfirst and only runs its own excluded-set query if that returns null, so our hook returning a number would replace core's correct count with a copy of it. The same is true ofcomment_query(),rest_comment_query()andcomment_feed_where()againstWP_Comment_Query, the REST controller and both feed queries.So each of the four checks
Comment::core_reads_excluded_comment_types(), which isfunction_exists( '_wp_get_excluded_comment_types_clause' ). That helper is private to the core patch, it is what the patch routes the count, both feeds and the admin pending count through, and the polyfill leaves it undefined on purpose. Its presence is the one signal that core does the work now. When it is true the count hook returns null so core counts, and the other three return their input untouched.Neither side of the guard exercises that path, since neither the polyfill nor the shim defines the sentinel. The predicate is filterable (
activitypub_core_reads_excluded_comment_types), sotest_hooks_stand_down_when_core_reads_the_setforces it true for its duration and asserts each hook returns its input, in process, with no sentinel defined. If it ever fails, a hook is pre-empting core's own query.How to test both sides
The point of a polyfill is that both sides of the guard behave the same. You cannot patch
wp-includes/in this project, so the "core has it" side is simulated with an mu-plugin,bin/comment-type-shim/. It loads before every plugin and defines the core API, so every guard flips to the core branch.Both runs are green here. The tell that the shim is really live:
Test_Comment_Types::test_built_in_types_cannot_be_re_registeredis skipped on the polyfill branch (there are no built-ins to protect) and runs on the shim branch, so the skipped count drops by one. If a test ever passes on one side and fails on the other, the polyfill has drifted from core, which is the one thing this setup exists to catch.The installer asks wp-env for the container rather than grepping docker, because several wp-env projects can run at once and the first
-tests-cli-1match was the wrong one.How to test against the real core patches
To test against the upstream diffs themselves rather than the shim, apply them to a core checkout outside this repository and point wp-env at it.
.wp-env.override.jsonis gitignored.{ "core": "../wordpress-develop/build" }Then
npx wp-env startand run the suite with no shim installed: the guards see core's real functions and skip the polyfill.Sharing it with Webmention and ATmosphere
Nothing in
includes/polyfill/is namespaced or plugin-specific, the two files can be copied as-is. One thing has to be settled first, and it is the reason I would not merge this before talking to the Webmention side: Webmention registersrepostandlikeunder the same slugs this plugin does. With separate per-plugin globals that never collided. With one core registry the second registration wins, and core's own guard only protects built-ins. So the two plugins have to either agree on who owns those slugs or namespace them.Open questions
get_comment_type_labels()goes through_get_custom_object_labels(), a private helper shared with post types. The polyfill fills the same keys fromlabelsand the defaults instead, so$labels->nameand->singular_nameread the same, but the full label set is not byte identical. If the label object matters to a consumer, that is where to look.#[AllowDynamicProperties]on the polyfilled class matches core and is what letsicon,classandactivity_typessurvive on the object. On PHP 7.4 the attribute is parsed as a comment and dynamic properties work anyway, I checked.