Skip to content

Conversation

@jorgefilipecosta
Copy link
Member

This PR applies feedback provided in #108, by @aaronjorbin as a preparation for the core patch.

Testing

Verify automated testing pass.
Oppenened the editor, and the browser console, made some test requests:

wp.apiFetch({ path: wp.url.addQueryArgs('/wp/v2/abilities/core/get-site-info/run', { fields:[ 'name' ] } ) } ).then(console.log);
wp.apiFetch({ path: wp.url.addQueryArgs('/wp/v2/abilities/core/get-site-info/run', {input: { fields:[ 'name' ] } } ) } ).then(console.log);

Verified things return as expected.

@github-actions
Copy link

github-actions bot commented Oct 23, 2025

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@codecov
Copy link

codecov bot commented Oct 23, 2025

Codecov Report

❌ Patch coverage is 98.15668% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.47%. Comparing base (4513526) to head (c260793).
⚠️ Report is 1 commits behind head on trunk.

Files with missing lines Patch % Lines
includes/bootstrap.php 0.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk     #128      +/-   ##
============================================
- Coverage     90.57%   90.47%   -0.10%     
+ Complexity      188      177      -11     
============================================
  Files            20       20              
  Lines          1485     1470      -15     
  Branches        120      120              
============================================
- Hits           1345     1330      -15     
  Misses          140      140              
Flag Coverage Δ
javascript 94.31% <ø> (ø)
unit 89.34% <98.15%> (-0.14%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gziolo
Copy link
Member

gziolo commented Oct 27, 2025

It looks like we need to land #126 first so CI job running against WP core trunk don't fail anymore.

@jorgefilipecosta
Copy link
Member Author

It looks like we need to land #126 first so CI job running against WP core trunk don't fail anymore.

Reviewing #126, I think this and #126 should be merged soon so then I can follow up with further syncs from core.

@gziolo
Copy link
Member

gziolo commented Oct 27, 2025

This will require a rebase with trunk now. We might need a better mechanism that detects whether these ability categories and abilities haven't been registered earlier, as now that happens automatically in WP core trunk.

@jorgefilipecosta jorgefilipecosta force-pushed the update/simplify-core-abilties-registration branch from 73161fb to ff2624a Compare October 27, 2025 12:31
if ( function_exists( 'add_action' ) ) {
add_action( 'wp_abilities_api_categories_init', array( 'WP_Core_Abilities', 'register_category' ) );
add_action( 'wp_abilities_api_init', array( 'WP_Core_Abilities', 'register' ) );
add_action( 'wp_abilities_api_categories_init', 'wp_register_core_ability_categories' );
Copy link
Member

Choose a reason for hiding this comment

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

It might be possible to simplify all the conditions used with a simple (and remove PHPCS exception):

if ( has_action( 'wp_abilities_api_categories_init', 'wp_register_core_ability_categories' ) {

The same for another action.

Copy link
Member Author

@jorgefilipecosta jorgefilipecosta Oct 27, 2025

Choose a reason for hiding this comment

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

Hi @gziolo, you mean just something like as top level code without any other condition?
if ( ! has_action( 'wp_abilities_api_categories_init', 'wp_register_core_ability_categories' ) ) {
add_action( 'wp_abilities_api_categories_init', 'wp_register_core_ability_categories' );
}

Copy link
Member

Choose a reason for hiding this comment

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

It's the common pattern used in the Gutenberg plugin. The wrapping:

if ( function_exists( 'has_action' ) ) {

might still be necessary to ensure it doesn't break when loading files too early before WP core, which happens in some scenarios.

*
* @return void
*/
// phpcs:ignore WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedFunctionFound
Copy link
Member

Choose a reason for hiding this comment

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

This is no longer necessary. I think we could copy the latest version directly from WP core.

Copy link
Member Author

Choose a reason for hiding this comment

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

I copied the last version directly from core as suggested 👍

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

There is one feedback to consider, but otherwise this looks great. Thank you for taking care of it!

@jorgefilipecosta jorgefilipecosta merged commit 9899991 into trunk Oct 27, 2025
22 of 23 checks passed
@jorgefilipecosta jorgefilipecosta deleted the update/simplify-core-abilties-registration branch October 27, 2025 13:53
@github-project-automation github-project-automation bot moved this from Needs review to Done in WordPress Abilities API: planning Oct 27, 2025
@jorgefilipecosta
Copy link
Member Author

Thank you a lot for the review @gziolo, if required I will follow up with enhancements 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Bug Something isn't working

Projects

Development

Successfully merging this pull request may close these issues.

3 participants