-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Webfonts: change class properties from static to instance members #39361
Webfonts: change class properties from static to instance members #39361
Conversation
I'm on the fence about this one. What if instead of introducing a new Both solutions above would allow us to reset the class props during tests, without the addition of a new singleton class 🤔 |
9078b01
to
b2acc64
Compare
I moved from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
Test failures are not related to the changes proposed by the PR. |
Part of #39332.
What?
Having static variables in the
WP_Webfonts
class makes the class share state between instances. This is not needed in a lot of cases and @jeyip and I couldn't figure out why it was done like that.Why?
That are several problems with that approach, including shared state issues. The tests become aware of each other and order starts to matter. Other instances of
WP_Webfonts
might appear in the codebase, and that might lead to confusing and unexpected behavior. It's even impossible to test features of thewp_webfonts_*
API in the current state of things.I wonder why we decided to take this route 🤔
How?
By moving
$webfonts
and$providers
variables to instance members, whenever someone creates a new instance ofWP_Webfonts
, they'll start with a fresh copy.We're also moving from
static
toglobal
insidewp_webfonts
so the instance can be accessed and reset before running a new test case.Testing Instructions
Nothing should change at the implementation level. You should notice the test's expectations are a little different, but the tests are still passing.