[wp-trac] [WordPress Trac] #32967: Site Icon: Refactor class for improved readability, extensibility and testing

[WordPress Trac]

#32967: Site Icon: Refactor class for improved readability, extensibility and
testing
----------------------------+-----------------------------
 Reporter:  Frank Klein     |      Owner:
     Type:  enhancement     |     Status:  new
 Priority:  normal          |  Milestone:  Awaiting Review
Component:  Administration  |    Version:  trunk
 Severity:  normal          |   Keywords:
  Focuses:  administration  |
----------------------------+-----------------------------
 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.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/32967>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform