Twenty Sixteen: ensure content_width setup is in its own callback function rather than in functions.php directly

[chipbennett]

For all core-bundled Themes (Twenty Ten through Twenty Fourteen), the Theme defines the $content_width global directly in functions.php, outside of any callbacks hooked into an explicit action.

Each of the Themes has a setup function, hooked into after_setup_theme, that is the appropriate action for Themes to perform Theme-specific setup actions, including defining $content_width.

With $content_width defined directly in functions.php, the action happens implicitly when functions.php is parsed, which makes the action more difficult to override explicitly via Child Theme or Plugin. Child Themes can override $content_width via similar implicit action, since the Child Theme functions.php is parsed before the template functions.php; but having an explicit action to target would be less fragile.

Attached patches move $content_width definition inside twenty{year}_setup().

[nacin]

The ability for a child theme to override this is why it is wrapped in an isset(), for what it's worth.

[obenland]

Context: Discussions ​here and ​here.

[SergeyBiryukov]

Related: #21256

[lancewillett]

Not sure where this is headed, but it's no longer awaiting review.

[jorbin]

@lancewillett - Would love to know where you think this should head.

I tend to think that making this change now to the current default themes isn't the best idea, but that it might make sense to consider this in the construction of future default themes.

[chipbennett]

Shouldn't there be a valid reason, if the ticket is to be closed?

In over a year, no such reason has been articulated.

[jorbin]

Sorry that I didn't really articulate my rational Chip. My bad.

What if a child theme has over riden twentyten_setup et al. (since they are pluggable function) without defining $content_width? If we make this change, those themes will no longer be defining $content_width.

[dd32]

Replying to jorbin:

What if a child theme has over riden twentyten_setup et al. (since they are pluggable function) without defining $content_width? If we make this change, those themes will no longer be defining $content_width.

since $content_width can already be overridden by a child theme, that seems like a good reason not to change it for existing themes, but keep in mind for future themes.

[philiparthurmoore]

@jorbin and @dd32 make good points here. Not changing the current default themes, but ensuring that $content_width is wrapped in a setup function in Twenty Sixteen, would be what I lean towards.

[chipbennett]

Replying to jorbin:

Sorry that I didn't really articulate my rational Chip. My bad.

What if a child theme has over riden twentyten_setup et al. (since they are pluggable function) without defining $content_width? If we make this change, those themes will no longer be defining $content_width.

Thanks, @jorbin. That's a fair concern. What if, instead of putting it in the setup function, it were inside its own callback?

[jorbin]

​Code refactoring should not be done just because we can. Outside of this being the "right" way to do things, what do we gain? What is the perfomance increase? What are the bugs that are fixed?

[chipbennett]

The biggest issue for me personally, as part of the Theme Review Team, is that the core-bundled Themes (and Underscores) are the most-used Themes for derivative works. Thus, when the core-bundled Themes do something one way, that method gets propagated, readily.

I think there is inherent benefit in continually raising the bar, to promote best practices - especially when the practices in question are known to be learned and used by a very wide audience.

While making this change in the next core-bundled Theme will be a good start, the problem is that the existing core-bundled Themes remain active, and actively used.

I admittedly have another, selfish motive: it is tedious continually having to explain to Theme developers during Theme reviews that, yes, they are required to follow a Theme review guideline, even though the core-bundled Theme from which they derived their own Theme doesn't follow that guideline - and that core-bundled Themes won't necessarily adhere to all guidelines, because the Theme Review Team has no direct control over core-bundled Themes.

And also, if there's ever any movement on #21256, moving $content_width inside a callback will make for a much easier transition.

[philiparthurmoore]

@chipbennett, don't want to take this ticket off topic but with regard to Underscores, I'm very strongly in favor of your approach. I'll work on getting ​this issue resolved next week so that future downloads of Underscores don't have this problem.

Don't have much say with regard to older default themes, but I also agree that this is a change that should definitely go into Twenty Sixteen. I see no reason not to do it.

[lancewillett]

I agree Twenty Sixteen should do this, for sure. I'm in favor of a separate callback like _s will do — and not adding to the existing theme setup function.

I don't think we should change older default themes — too much can break.

[karmatosed]

Twenty Sixteen has this resolved in: ​https://github.com/WordPress/twentysixteen/blob/master/functions.php#L128. As a result, I am going to flag this as invalid in this case because it's already over there. Thanks everyone, just closing the loop from GitHub.