WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#20506 closed defect (bug) (fixed)

Undefined variable $screen_layout_columns when filtering out screen options

Reported by: griffinjt Owned by: ryan
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.3.1
Component: Warnings/Notices Keywords: has-patch dev-feedback
Focuses: Cc:

Description

When using the screen_options_show_screen filter to remove the screen tab completely, an undefined variable notice appears with WP_DEBUG on. Specifically, the $screen_layout_columns variable is undefined because the current code expects it to be set and to be an integer.

This patch fixes it by setting it equal to itself while still casting it as an integer. Can't find any issues in my testing.

Attachments (9)

edit-form-fix.diff (808 bytes) - added by griffinjt 10 years ago.
20506.patch (2.8 KB) - added by SergeyBiryukov 10 years ago.
edit-form-fix.2.diff (5.4 KB) - added by griffinjt 10 years ago.
20506.diff (4.7 KB) - added by ryan 10 years ago.
Store layout columns in num_columns option rather than a global. Deprecate the global. Use get_option( 'num_columns' ) instead of the global.
20506.2.diff (5.0 KB) - added by ryan 10 years ago.
Cast user option to int. Remove auto check.
20506.3.diff (5.6 KB) - added by ryan 10 years ago.
get_columns()
20506.4.diff (5.2 KB) - added by ryan 10 years ago.
Store num columns in object var.
20506.5.diff (5.1 KB) - added by ryan 10 years ago.
Tidying
20506.minor-typo-fix.patch (633 bytes) - added by SergeyBiryukov 10 years ago.

Download all attachments as: .zip

Change History (28)

#1 @SergeyBiryukov
10 years ago

There are some more instances of using unchecked $screen_layout_columns value (20506.patch). Perhaps we should set it somewhere outside of WP_Screen::render_screen_layout() if the latter is not called.

#2 @griffinjt
10 years ago

Yes, it needs to be outside out that method.

Actually, the whole method needs to be separated into two methods: one with the logic and the other with the output. I've split WP_Screen::render_screen_layout() into the former and WP_Screen::get_screen_layout().

Since it's separated, I've called WP_Screen::get_screen_layout() within the render_screen_meta method, just before the check is made to see if the screen options should be output.

Needs some testing, but it has worked fine for me in all my tests.

#3 @SergeyBiryukov
10 years ago

  • Milestone changed from Awaiting Review to 3.4

#4 @ryan
10 years ago

nacin and I discussed introducing get_user_option() and deprecating the global.

Lines like this:

echo (int) $screen_layout_columns ? (int) $screen_layout_columns : 'auto';

Would become:

echo get_current_screen()->get_user_option( 'layout_columns', 'auto' );

#5 @nacin
10 years ago

echo get_current_screen()->get_user_option( 'layout_columns', 'auto' );

The one issue with that is the second argument is $default in this case, which is not unusual. (get_option() works the same way.) But it would be inconsistent with the WP_Screen::get_option() method, whose second argument is a specific array key of the requested option, rather than a default. I'm now kicking myself for that.

#6 follow-up: @nacin
10 years ago

I also just don't know how many of these $screen_layout_columns calls are necessary for functionality. Some of them were added in [18736] but most of that got backed out of core.

#7 in reply to: ↑ 6 ; follow-up: @azaozz
10 years ago

Replying to nacin:

$screen_layout_columns was mostly needed when we used to output the right postbox container on the Edit/Write screens dependent on how many columns were selected. As that changed in 3.4 and the placement of that postbox container is fully controlled from css, perhaps we can retire the global completely.

#8 in reply to: ↑ 7 ; follow-up: @azaozz
10 years ago

Replying to azaozz:

Hmm, or maybe not, I take this back :)

Implementing the user setting for columns is much more straightforward when having a method (or a global).

#10 in reply to: ↑ 8 @griffinjt
10 years ago

Replying to azaozz:

Replying to azaozz:

Hmm, or maybe not, I take this back :)

Implementing the user setting for columns is much more straightforward when having a method (or a global).

In light of [20570], I don't see why would couldn't retire the global now. :-)

#11 @ryan
10 years ago

The screen_layout_columns global is still used in edit-form-advanced.php, edit-link-form.php, and dashboard.php. The remaining uses are internal to WP_Screen.

#12 @azaozz
10 years ago

Right, [20570] was just a cleanup. The way columns work is by setting a class on the container depending on the user setting, then if the screen is narrow, overwriting the CSS to move the column(s) to the right.

#13 @griffinjt
10 years ago

Ahh, gotcha. Thanks for explaining. :-)

The global could be converted into a static method I suppose, although the global appears to work fine.

Last edited 10 years ago by griffinjt (previous) (diff)

@ryan
10 years ago

Store layout columns in num_columns option rather than a global. Deprecate the global. Use get_option( 'num_columns' ) instead of the global.

#14 @ryan
10 years ago

Move layout column setup into render_screen_meta(). Use add_option( 'num_columns' ) and get_option( 'num_columns' ) for setting and getting the user selected number of columns. Retain the screen_layout_columns global for back compat. Switch admin screens from the global to get_option().

@ryan
10 years ago

Cast user option to int. Remove auto check.

@ryan
10 years ago

get_columns()

@ryan
10 years ago

Store num columns in object var.

@ryan
10 years ago

Tidying

#15 @ryan
10 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

In [20579]:

Clean out layout columns API in WP_Screen.

  • Move layout column setup into render_screen_meta() so that the number of columns is available earlier.
  • Store the user provisioned number of columns in an instance var.
  • Access the var with get_columns()
  • Move all templates away from the screen_layout_columns global to the get_columns() method.
  • Deprecate the global
  • Remove the no longer needed check for 'auto' in the user option.
  • Cast the user option to an int.

Props griffinjt
fixes #20506

#16 follow-up: @ryan
10 years ago

griffinjt, since my patch is a bit different, please try it out and let us know if it solves the problem you're seeing.

#17 in reply to: ↑ 16 @griffinjt
10 years ago

Replying to ryan:

Looking good on my end - no issues that I can see. The only thing I would change in your patch (nitpicky) is that the way you have called the global doesn't follow coding standards - see https://twitter.com/#!/nacin/status/192611263321358338 by nacin regarding the issue.

Should be $GLOBALS['screen_layout_columns'] instead (only spaces is the key is a variable).

Last edited 10 years ago by griffinjt (previous) (diff)

#18 @SergeyBiryukov
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

There's a typo in the comment (20506.minor-typo-fix.patch).

#19 @azaozz
10 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In [20591]:

Fix typo in a comment, props SergeyBiryukov, fixes #20506

Note: See TracTickets for help on using tickets.