Site Icon: Refactor class for improved readability, extensibility and testing

[Frank Klein]

While the current implementation of the WP_Site_Icon class is a working implementation of the site icon feature, I'd ask to consider refactoring the code before shipping.

Here are the issues that I see with the current implementation:

1. The feature adds a new global variable, $wp_site_icon. We should consider looking into ways that would avoid adding a new global variable.
2. The class is very large, and has several tasks. These tasks are executed by several quite large methods. It is therefore difficult to read and understand. We should consider ways of breaking the single class up into several classes, and have smaller methods that deal with a single task.
3. Method names could be improved by using action verbs, making it clearer what a particular piece of code does.
4. There is no designated routing method for the admin. Redirects occur in several places, and make the flow of screens difficult to understand. We should consider bundling all redirections in a single method if possible.
5. Superglobals are accessed directly, which makes testing harder. We should consider looking at ways that would allow tests to inject this data.
6. Since the class is and most methods are public, I assume that it is meant to be extensible by plugins. We should come up with a few use cases, and see whether we give plugin authors the right interfaces to add features.

I consider that breaking the class up into three different classes would be a good starting point:

1. A site icon class. This class would hold properties of the site icon, and provide methods to access and modify these properties. This could be singleton, since we only ever would want one site icon per site.
2. An admin class. This class would handle this display and the flow of admin screens. It would rely on the processing class to do the work.
3. A data processing class. This class would deal with handling the data provided by the admin UI, and give feedback back to the admin class to allow screens to be updated.

There is no patch attached to this ticket, since I consider that we should take a moment to discuss the right approach first. Once we have an improved design for this feature, the coding is the easy part.

[obenland]

This will have to be done before beta3.

[obenland]

Authored by Frank Klein

[obenland]

Authored by Frank Klein

[obenland]

Authored by Frank Klein

[obenland]

Authored by Frank Klein

[obenland]

I think this is too big a change to get in without rushing it. The current implementation may not be ideal, but if we want to get it as close to ideal as possible, we should take more time and get it right.

[toscho]

Then make at least the class final and all properties private.

[Frank Klein]

I agree with Obenland here, the code posted was just a rushed first approach.

But I also concur with @tosho's proposition, which goes back to my initial suggestion to lock down the API in 4.3 and attack this in earnest in 4.4.

If we ship the current code, people will build upon it, and we'll have issues maintaining backwards compatibility once we refactor.

[swissspidy]

In 33329 the WP_Site_Icon class became quite small because of the removal of the settings page. It only has 8 public methods now. Perhaps this is already enough refactoring? Or at least enough to finish this in 4.3

[Frank Klein]

Absolutely, props to Konstantin.

I consider this as invalid, since the original issue no longer exists.