WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 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 2 years ago.
20506.patch (2.8 KB) - added by SergeyBiryukov 2 years ago.
edit-form-fix.2.diff (5.4 KB) - added by griffinjt 2 years ago.
20506.diff (4.7 KB) - added by ryan 2 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 2 years ago.
Cast user option to int. Remove auto check.
20506.3.diff (5.6 KB) - added by ryan 2 years ago.
get_columns()
20506.4.diff (5.2 KB) - added by ryan 2 years ago.
Store num columns in object var.
20506.5.diff (5.1 KB) - added by ryan 2 years ago.
Tidying
20506.minor-typo-fix.patch (633 bytes) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (28)

griffinjt2 years ago

SergeyBiryukov2 years ago

comment:1 SergeyBiryukov2 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.

comment:2 griffinjt2 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.

griffinjt2 years ago

comment:3 SergeyBiryukov2 years ago

  • Milestone changed from Awaiting Review to 3.4

comment:4 ryan2 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' );

comment:5 nacin2 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.

comment:6 follow-up: nacin2 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.

comment:7 in reply to: ↑ 6 ; follow-up: azaozz2 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.

comment:8 in reply to: ↑ 7 ; follow-up: azaozz2 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).

comment:10 in reply to: ↑ 8 griffinjt2 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. :-)

comment:11 ryan2 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.

comment:12 azaozz2 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.

comment:13 griffinjt2 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 2 years ago by griffinjt (previous) (diff)

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

comment:14 ryan2 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().

ryan2 years ago

Cast user option to int. Remove auto check.

ryan2 years ago

get_columns()

ryan2 years ago

Store num columns in object var.

ryan2 years ago

Tidying

comment:15 ryan2 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

comment:16 follow-up: ryan2 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.

comment:17 in reply to: ↑ 16 griffinjt2 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.

Version 0, edited 2 years ago by griffinjt (next)

comment:18 SergeyBiryukov2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

comment:19 azaozz2 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.