WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#27863 closed enhancement (worksforme)

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

Reported by: chipbennett Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.9
Component: Bundled Theme Keywords:
Focuses: Cc:

Description

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().

Attachments (5)

twentyfourteen.content_width (916 bytes) - added by chipbennett 4 years ago.
twentythirteen.content_width (952 bytes) - added by chipbennett 4 years ago.
twentytwelve.content_width (726 bytes) - added by chipbennett 4 years ago.
twentyeleven.content_width (798 bytes) - added by chipbennett 4 years ago.
twentyten.content_width (1.1 KB) - added by chipbennett 4 years ago.

Download all attachments as: .zip

Change History (24)

#1 @chipbennett
4 years ago

  • Keywords has-patch added

#2 @nacin
4 years ago

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

#3 @obenland
4 years ago

  • Component changed from Themes to Bundled Theme

Context: Discussions here and here.

#5 @lancewillett
4 years ago

  • Keywords dev-feedback added
  • Milestone changed from Awaiting Review to Future Release

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

This ticket was mentioned in Slack in #themereview by chipbennett. View the logs.


4 years ago

#7 @jorbin
3 years ago

  • Keywords close added

@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.

#8 @chipbennett
3 years ago

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

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

#9 follow-ups: @jorbin
3 years ago

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.

#10 in reply to: ↑ 9 @dd32
3 years ago

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.

#11 @philiparthurmoore
3 years ago

@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.

#12 in reply to: ↑ 9 @chipbennett
3 years ago

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?

#13 @jorbin
3 years ago

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?

#14 @chipbennett
3 years ago

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.

#15 @philiparthurmoore
3 years ago

@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.

#16 @lancewillett
3 years ago

  • Keywords has-patch dev-feedback close removed
  • Summary changed from Move $content_width inside setup function in core-bundled Themes to Twenty Sixteen: ensure content_width setup is in its own callback function rather than in functions.php directly

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.

#17 @karmatosed
3 years ago

  • Resolution set to invalid
  • Status changed from new to closed

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.

#18 @netweb
3 years ago

  • Milestone Future Release deleted

#19 @DrewAPicture
3 years ago

  • Resolution changed from invalid to worksforme
Note: See TracTickets for help on using tickets.