WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#11385 closed defect (bug) (fixed)

a couple of uninstantiated options

Reported by: Denis-de-Bernardy Owned by:
Milestone: 2.9 Priority: normal
Severity: normal Version: 2.9
Component: Optimization Keywords: has-patch commit dev-feedback
Focuses: Cc:

Description

A couple of options, which are used by get_user_option(), do not seem to be instantiated anywhere.

In particular:

			case 'edit_per_page':
			case 'edit_pages_per_page':
			case 'edit_comments_per_page':
			case 'upload_per_page':
			case 'categories_per_page':
			case 'edit_tags_per_page':
			case 'plugins_per_page':

Attachments (3)

11385.diff (526 bytes) - added by Denis-de-Bernardy 4 years ago.
11385-per_page-options-filters.diff (7.0 KB) - added by nacin 4 years ago.
11385-per_page-options-filters.2.diff (6.7 KB) - added by nacin 4 years ago.
Patch without global $posts_per_page, just in case and to move this along.

Download all attachments as: .zip

Change History (17)

Denis-de-Bernardy4 years ago

comment:1 follow-up: nacin4 years ago

I can't find any option called by get_user_option() that is instantiated on the blog level. Some appear to be instantiated per-user in wp_insert_user() (admin_color, use_ssl, comment_shortcuts, rich_editing).

Granted, I see that it might be good for some of these to be instantiated somewhere, but the code already provides for that; example below. Many other options contain variable names, which is also appropriately handled.

$plugins_per_page = get_user_option('plugins_per_page');
if ( empty($plugins_per_page) )
	$plugins_per_page = 999;
$plugins_per_page = apply_filters('plugins_per_page', $plugins_per_page);

Perhaps I'm missing something?

comment:2 nacin4 years ago

I also found #2033 -- rich_editing was specifically removed as a blog-level option after it was converted to a user option, even though get_user_option at the time fell back to blog-level options (which it still does, at least by default) after checking for the option at the user-per-blog and user scopes.

comment:3 nacin4 years ago

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

comment:4 nacin4 years ago

  • Milestone 2.9 deleted

comment:5 in reply to: ↑ 1 Denis-de-Bernardy4 years ago

  • Milestone set to 2.9
  • Resolution worksforme deleted
  • Status changed from closed to reopened

Replying to nacin:

I can't find any option called by get_user_option() that is instantiated on the blog level. Some appear to be instantiated per-user in wp_insert_user() (admin_color, use_ssl, comment_shortcuts, rich_editing).

and others are not.

Granted, I see that it might be good for some of these to be instantiated somewhere, but the code already provides for that; example below. Many other options contain variable names, which is also appropriately handled.

$plugins_per_page = get_user_option('plugins_per_page');
if ( empty($plugins_per_page) )
	$plugins_per_page = 999;
$plugins_per_page = apply_filters('plugins_per_page', $plugins_per_page);

Perhaps I'm missing something?

Yes: these cause a useless query on every admin page load when they're not instantiated, since WP ends up thinking they're not autoloaded.

comment:6 follow-up: dd324 years ago

  • Keywords commit dev-feedback added

Ticket seems to be correct, Whilst i've not tested it, seems right and it'll only occur for users which have not saved the screen options for the current page.

comment:7 in reply to: ↑ 6 nacin4 years ago

Replying to Denis-de-Bernardy:

Yes: these cause a useless query on every admin page load when they're not instantiated, since WP ends up thinking they're not autoloaded.

Replying to dd32:

Ticket seems to be correct, Whilst i've not tested it, seems right and it'll only occur for users which have not saved the screen options for the current page.

Fair enough, that's convinced me to about-face on this. In that case, are there any others that should be set at the blog level?

comment:8 nacin4 years ago

Or, instead of instantiating them in wpdb->options, what if we used the parameter for get_user_option that prevents a blog-level check?

That would prevent needing to fetch the blog-level option, prevent the need to add six new options (which are autoloaded, as well), would prevent the unnecessary query generated by get_option, etc. Just in general, it prevents duplication in logic that would need to remain hardcoded and could very well be hard-coded, considering there is a filter both on get_user_option and on each of these values.

comment:9 nacin4 years ago

Patch that standardizes filters, standardizes checks to prevent $per_page being < 1, and prevents get_user_option from checking for a blog option.

Also removes an orphaned $posts_per_page = get_option('posts_per_page') in the admin.php bootstrap. The variable is never used, though it is overwritten in post.php, and appears to be an artifact that is left over from before the time of screen options.

Probably good for 3.0. To fix in 2.9, I'd suggest the simple get_user_option change of setting the third variable to false, turning off blog option checking.

comment:10 ryan4 years ago

nacin's patch looks good except for the removal of the $posts_per_page global. That option is defined in the schema. Perhaps it doesn't need to be made a global, but that can be argued in a different ticket.

comment:11 ryan4 years ago

In a future release, we should replace get_user_option() with something that doesn't unexpectedly dip into the options table by default. This isn't the first time we've made unnecessary option table queries because of it.

comment:12 nacin4 years ago

Replying to ryan:

nacin's patch looks good except for the removal of the $posts_per_page global. That option is defined in the schema. Perhaps it doesn't need to be made a global, but that can be argued in a different ticket.

Correct me if I'm wrong, but that's the reading option for the number of posts to display per page in a theme. The global $posts_per_page variable itself is unused in admin, but in the off-chance a plugin used it, it is overridden to hold the screen options per_page value in post.php.

I suppose this would make sense if it is being maintained for back compat, assuming that the admin used to rely on the same setting?

nacin4 years ago

Patch without global $posts_per_page, just in case and to move this along.

comment:13 ryan4 years ago

It's probably old cruft that needs to be removed, but I'd rather play it safe for 2.9 in case someone happens to be using it.

comment:14 ryan4 years ago

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

(In [12375]) Don't fallback to the options database when retrieving *_per_page user options. Props nacin. fixes #11385

Note: See TracTickets for help on using tickets.