Do not use static method / properties in Duotone class.

[spacedmonkey]

Trac ticket: https://core.trac.wordpress.org/ticket/58555

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[ajlende]

I don't see any reason for this change. It was simpler before, and this introduces an unnecessary global.

[spacedmonkey]

I don't see any reason for this change. It was simpler before, and this introduces an unnecessary global.

The code as is stands, it is harder to maintain, does not follow the design pattern of other class / code. There is no new global added here, just a local variable.

[ajlende]

The code as is stands, it is harder to maintain

Can you elaborate? If the point of this class is just to set up three filters and three actions, why would making these non-static be any easier to maintain?

does not follow the design pattern of other class / code

I know of at least two pieces of code that make heavy use of static methods that I work in frequently: class-wp-style-engine.php and class-wp-theme-json-resolver.php. Both are collections of functions where we want to keep some parts private just like here.

[ajlende]

There is no new global added here, just a local variable.

This is a global variable.

Any code that runs after this file has been included in wp-settings.php will have access to the global $duotone variable.

[spacedmonkey]

Right. WordPress does this in lots of places. I don't see the problem with it....

[priethor]

So does WordPress with static methods. In practice, a global variable is similar to a class with static methods... I'm no core committer, but as a member of the release squad, I'm only in favor of last-minute style changes right before RC1 if there are objective reasons.

[spacedmonkey]

@ajlende @priethor I am going to invert this question, why is static being used in the first place. It is never uncommon in WordPress space to do this? I have heard something about keeping state, but that can be done with a global as well.

[hellofromtonya]

I respectfully disagree with the changes in this PR. I do not see compelling reasons to convert this class from a static class wrapper design pattern to OOP.

I've previously shared why I believe and support the usage of static class wrappers and when to use them vs global functions and OOP.

Class wrappers (using all static properties and methods) are an architectural design pattern and do indeed provide many benefits.

I left my feedback and opinions on the Trac ticket https://core.trac.wordpress.org/ticket/58555#comment:38.

The code as is stands, it is harder to maintain

Why is harder to maintain than OOP within the WordPress open source project?

I disagree. I think the code is not harder to maintain.

IMO OOP's requirement of a global way to access the object (i.e. via global function or variable) makes the code harder to maintain because of BC and the risk of overwriting the global object concerns.