Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move $content_width declaration inside _s_setup() #100

Closed
michiecat opened this issue Nov 2, 2012 · 9 comments
Closed

Move $content_width declaration inside _s_setup() #100

michiecat opened this issue Nov 2, 2012 · 9 comments

Comments

@michiecat
Copy link
Contributor

On one of the articles in theThemeShaper WordPress Theme Tutorial: 2nd Edition Series, which uses code based on _s, it's been suggested in the comments that $content_width should be moved inside _s_setup().

Here is the reason given in the comments (by Justin Tadlock):

"This makes sure that it’s easy for child themes to know exactly what point to overwrite it. It also allows you to get rid of the isset() check."

(I'm aware that _s is not intended to be a parent theme, though.)

Chip Bennett commented:

"We (the Theme Review Team) have been trying to move developers more toward putting everything in functions.php inside callbacks. Is there any advantage/disadvantage in doing so with $content_width?"

Opinions?

@ashfame
Copy link
Member

ashfame commented Nov 2, 2012

+1 Chances of a theme built using _s ending up being used as a parent theme is significant, like any other normal theme.

@mfields
Copy link
Contributor

mfields commented Nov 2, 2012

I looked into this a bit last night and tested with a bunch of plugins. What I discovered is that there is practically no difference between setting $content_width inside or outside a function hooked into after_setup_theme. To quote the Codex:

[after_setup_theme] is the first action hook available to themes, triggered immediately after the active theme's functions.php file is loaded.

I looked through core and there are no actions fired between the time where functions.php is loaded and when the action is fired.

Personally, I like the idea of moving the setting of this variable into a function hooked into after_setup_theme. Can anyone think of a reason not to do it?

@lancewillett
Copy link

Can anyone think of a reason not to do it?

Yes. Through many bugs with plugins and much trial and error we've found that the best approach is defining the first content_width value outside all functions.

The base definition won't work if it fires on actions like after_setup_theme or template_redirect because it will happen too late for plugins like Post by Email and Tiled Galleries (both in use on WP.com and both are in or coming soon to Jetpack).

Other secondary content_width calculations (based on widget areas, page template, etc) can be in a hooked function, however — that's fine.

@justintadlock
Copy link

The base definition won't work if it fires on actions like after_setup_theme or template_redirect because it will happen too late for plugins like Post by Email and Tiled Galleries (both in use on WP.com and both are in or coming soon to Jetpack).

Those plugins are going to run into many issues with themes adding $content_width on the after_setup_theme hook in that case, which is what we recommend at the Theme Review Team and I'm guessing many, many themes developers (such as myself) are already doing. I'd recommend passing this info along to whoever's on the dev team for those plugins to let them know to update them.

I can't think of any reason a plugin would assume the $content_width is defined any earlier than after_setup_theme. We have an appropriate hook for theme setup; plugins should respect that.

It definitely shouldn't happen on template_redirect. That hook would obviously be inappropriate.

@mfields
Copy link
Contributor

mfields commented Feb 3, 2013

I think this ticket should be closed for the following reasons:

  1. $content_width is a global variable, it is a best practice to set globals as early as possible in the global scope. _s currently does this.
  2. All "Twenty" themes bundled with core use a similar method as _s. We try to model our code after core as much as possible.
  3. _s conditionally sets $content_width meaning that child theme can override it by setting it anywhere in the file.

@philiparthurmoore
Copy link
Collaborator

I think this ticket should be closed...

Agreed. Closing it.

@ramiy
Copy link

ramiy commented Feb 6, 2013

Guys, i created documentation on the codex for $content_width:
http://codex.wordpress.org/Content_Width

And check out the tract ticket #21256 (http://core.trac.wordpress.org/ticket/21256), may be relevant to this discussion.

@chipbennett
Copy link

Underscores doing something the right way - such as defining $content_width inside the Theme setup callback hooked into after_setup_theme - should not be dependent upon Plugins that are doing something the wrong way.

I disagree with the argument that globals should be set as early as possible. They should be set where they would logically be expected to be set. In the case of a Theme-defined global, that would be after_setup_theme.

Lots of Theme developers are now using Underscores as the basis for the Themes that they submit to the Theme directory - and they're all having to modify it to move the $content_width definition inside the setup function where it belongs.

Do I need to go submit patches for all the Twenty-X Themes, so that Underscores can then conform to the core-bundled Theme behavior?

And @raimy +1 to your patch!

@lancewillett
Copy link

I agree with comments on the core Trac ticket that content width needs a complete re-think.

About this specific change: I still feel we should keep the current code as-is, in both _s and core themes. It's a pragmatic approach, working around things that are broken (like certain plugins). It speaks to one of the most central WordPress values of backward compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants