Skip to content

Conversation

@justlevine
Copy link
Contributor

@justlevine justlevine commented Aug 16, 2025

What

This PR changes WP_Abilities_Registry to store it's instance on ::$instance, instead of on a new global $wp_abilities variable.

(Note: I'm just code-reviewing and leaving feedback. If you think this warrants a wider discussion I can open a matching issue, otherwise I think a discussion here following by [accept | reject] is enough)

Why

Global variables are hard-to-type-and-test antipatterns. Just because legacy WP code used them before there were better patterns doesn't mean we need to introduce new ones.

Using a private instance gives core contributors and end-devs more insight into usage, and as developers working on the Abilities API, we get a much smaller surface for breaking changes to worry about.

@codecov
Copy link

codecov bot commented Aug 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (trunk@507e9ec). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff            @@
##             trunk      #19   +/-   ##
========================================
  Coverage         ?   90.64%           
  Complexity       ?       99           
========================================
  Files            ?        7           
  Lines            ?      556           
  Branches         ?        0           
========================================
  Hits             ?      504           
  Misses           ?       52           
  Partials         ?        0           
Flag Coverage Δ
unit 90.64% <100.00%> (?)

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.

@gziolo
Copy link
Member

gziolo commented Aug 18, 2025

One of the unit tests is failing with these changes:

Screenshot 2025-08-18 at 08 22 43

Unfortuntalty, the existing CI check doesn't propagate test failures and all jobs still pass.

@gziolo gziolo mentioned this pull request Aug 14, 2025
9 tasks
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.

I don't have a strong preference here. WordPress uses globals for many fundamental singletones like $post, $wp_scripts, $wp_rest_server. However, we need to make it fully testable and be able to reset the instance between test runs.

@gziolo gziolo added the [Type] Enhancement New feature or request label Aug 18, 2025
@justlevine
Copy link
Contributor Author

Unfortuntalty, the existing CI check doesn't propagate test failures and all jobs still pass.

Also unfortunately, our PHPUnit CI is passing when the tests itself fail - I'll take a look at that too! 😅

@justlevine justlevine mentioned this pull request Aug 18, 2025
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.

Using reflection makes the test pass 👍

@gziolo gziolo merged commit e61dab5 into WordPress:trunk Aug 19, 2025
17 checks passed
@justlevine justlevine deleted the dev/use-static-registry-instance branch August 20, 2025 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants