WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 22 months ago

#18958 closed defect (bug) (fixed)

Can't set "Show on screen" for Custom post types

Reported by: dd32 Owned by: nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.3
Component: Administration Keywords: has-patch early
Focuses: Cc:

Description (last modified by dd32)

As above. The posts_per_page "show on screen" function on custom post pages doesn't apply/save.

Attachments (9)

18958.edit-book.png (69.7 KB) - added by SergeyBiryukov 3 years ago.
18958.patch (504 bytes) - added by SergeyBiryukov 3 years ago.
18958.2.patch (1.8 KB) - added by SergeyBiryukov 3 years ago.
18958.diff (8.3 KB) - added by ryan 3 years ago.
Speak screen in the meta box functions
18958.2.diff (11.1 KB) - added by ryan 3 years ago.
null instead of 0
18958.3.patch (4.9 KB) - added by SergeyBiryukov 2 years ago.
18958.3.diff (1.7 KB) - added by nacin 2 years ago.
18958.4.diff (1.8 KB) - added by nacin 2 years ago.
Converts if to elseif. Rearranges to keep current behavior (taxonomy wins), which is the same behavior as in WP_Screen::get().
18958.tab.patch (597 bytes) - added by ocean90 22 months ago.

Download all attachments as: .zip

Change History (48)

comment:1 ryan3 years ago

Seems okay here. What's the CPT name? Maybe the switch to sanitize_key() busted something.

Last edited 3 years ago by ryan (previous) (diff)

comment:2 nacin3 years ago

  • Keywords reporter-feedback added; needs-patch removed

comment:3 follow-ups: SergeyBiryukov3 years ago

With the "Book" CPT example from Codex, my results are a bit different.

On initial page load, all meta boxes are displayed, but only "Featured Image" is checked in "Show on screen" (see the screenshot). If I toggle the checkboxes, the changes seem to be applied correctly.

comment:4 in reply to: ↑ 3 SergeyBiryukov3 years ago

Replying to SergeyBiryukov:

On initial page load...

i.e. when there's no metaboxhidden_book record in wp_usermeta.

Happens in 3.2.1 as well.

SergeyBiryukov3 years ago

comment:5 SergeyBiryukov3 years ago

Being called from meta_box_prefs() and do_meta_boxes(), get_hidden_meta_boxes() returns different results, due to different $screen values.

In meta_box_prefs(), it's:

WP_Screen Object( [id] => book, [base] => post, ... )

In do_meta_boxes(), it's:

stdClass Object( [id] => book, [base] => book )

Note the difference in base value.

Not sure what the preferred option is, but 18958.patch at least makes it consistent.

comment:6 dd323 years ago

Ok so.. I'm shamed to say: I was using a CPT plugin to add the post type. Below is the call it's using, It's probably caused by the dash in the CPT name, in core we underscores, I'd say it's messing with the splitting on dashes which I believe the screen options stuff uses.

register_post_type('test-cpt', array(
  'label' => 'Test CPT',
  'description' => '',
  'public' => true,
  'show_ui' => true,
  'show_in_menu' => true,
  'capability_type' => 'post',
  'hierarchical' => true,
  'rewrite' => true,
  'query_var' => true,
  'has_archive' => true,
  'supports' => array('title','editor','excerpt','custom-fields','comments','revisions','page-attributes')
)
);

comment:7 SergeyBiryukov3 years ago

"Show on screen" for test-cpt also seems to work for me in current trunk (aside from initial checkboxes inconsistency I've mentioned). The meta key saved is metaboxhidden_test-cpt.

Version 0, edited 3 years ago by SergeyBiryukov (next)

comment:8 dd323 years ago

  • Description modified (diff)

Oh damn, Sorry I've missed one very important word in the original ticket, The posts per page field doesn't save.

comment:9 dd323 years ago

  • Keywords reporter-feedback removed

comment:10 dd323 years ago

and just to confirm, it is the dash in the CPT name. 'test_cpt' works whereas, 'test-cpt' doesn't.

SergeyBiryukov3 years ago

comment:11 SergeyBiryukov3 years ago

  • Keywords has-patch added

Seems to be introduced in [10987].

18958.2.patch does str_replace( '-', '_', ... ) in a few more places, in order to save and read the value properly.

Or maybe we shouldn't replace the dash in set_screen_options() at all, since it's preserved in other options, such as metaboxhidden_test-cpt?

comment:12 azaozz3 years ago

Thinking we should make it a coding standard to use strings that qualify as PHP (and JS) variables names in all these cases. That would mean [A-Za-z0-9_] only. Such standard will let us avoid a lot of weird edge cases.

Perhaps even have a sanitizing function that would throw warning if something else is passed there.

Last edited 3 years ago by azaozz (previous) (diff)

comment:13 follow-up: dd323 years ago

Thinking we should make it a coding standard to use strings that qualify as PHP (and JS) variables names in all these cases.

Sounds fair, Except that dashes are universally the standard for slugs in WordPress (URL's, Filenames, CSS classes, etc). Underscores are a PHP/JS Language construct, which shouldn't make a difference here, as slugs should never be hardcoded or expressed as a variable.

It's a shame that dash was ever sanitized to an underscore really, I still can't see a good reason for it in this context.

comment:14 in reply to: ↑ 13 azaozz3 years ago

Replying to dd32:

Thinking we should make it a coding standard to use strings that qualify as PHP (and JS) variables names in all these cases.

...Except that dashes are universally the standard for slugs in WordPress (URL's, Filenames, CSS classes, etc).

Yes, they are, but trying to find the "path of least resistance". We can't go wrong with the standard for PHP and JS variables names. Also these strings aren't usually slugs that will be used in rewrite rules, most end up as option names.

comment:15 dd323 years ago

Also these strings aren't usually slugs that will be used in rewrite rules, most end up as option names.

I'd be highly surprised if most uses of CPT's & taxonomies out there specifically set the slug.. I'd go so far as to say, that in the majority of cases it'll only ever be set when they want to change the rewrite slug differently than the CPT/Tax slug and already have items loaded.

comment:16 ramiy3 years ago

also check this in RTL mode.

comment:17 ryan3 years ago

See also #15970

ryan3 years ago

Speak screen in the meta box functions

comment:18 ryan3 years ago

The meta box functions should be able to handle screen objects. Patch updates them to handle WP_Screen and use the current screen if an empty screen is passed. edit-form-advanced.php now passes 0 to all meta box calls to indicate that they should use the current screen. A bit ugly, but in a blessedly brief sort of way. Future WP_Screen improvements will hopefully replace a lot of this mess.

Last edited 3 years ago by ryan (previous) (diff)

ryan3 years ago

null instead of 0

comment:19 ryan3 years ago

In [19013]:

Update meta box functions to handle WP_Screen objects and pass objects instead of IDs to them in core files. Allow passing emptiness to get the current screen. see #18958

comment:20 nacin3 years ago

In [19019]:

Make $screen argument for add_meta_box() (previously $page) optional. see #18958.

comment:21 in reply to: ↑ 3 SergeyBiryukov3 years ago

Replying to SergeyBiryukov:

all meta boxes are displayed, but only "Featured Image" is checked in "Show on screen"

Opened #19010 for the checkboxes issue.

comment:22 nacin3 years ago

The change to passing around objects also confused get_hidden_meta_boxes(), as $screen->base is 'post', rather than the post type.

comment:23 nacin3 years ago

In [19049]:

Rework get_hidden_meta_boxes() to leverage a full WP_Screen object. Prevents custom post types from having their explicity supported meta boxes being hidden by default. see #18958.

comment:24 follow-up: ryan3 years ago

Is this fixed?

comment:25 in reply to: ↑ 24 SergeyBiryukov3 years ago

Replying to ryan:

Is this fixed?

No, per_page option for dd32's example still doesn't save.

edit_test_cpt_per_page is saved, edit_test-cpt_per_page is read.

comment:26 follow-up: ryan3 years ago

So we need your .2.path here or an updated one of mine from #15970?

comment:27 in reply to: ↑ 26 SergeyBiryukov3 years ago

Replying to ryan:

So we need your .2.path here or an updated one of mine from #15970?

Your patch in #15970 looks more comprehensive. I've added a refreshed version there, with function calls in a few more places: ticket:15970:15970.2.diff.

This ticket is probably a duplicate of that one.

comment:28 ryan3 years ago

  • Keywords early added
  • Milestone changed from 3.3 to Future Release

I marked #15970 as a dupe since this ticket has commits against it.

Punting this from 3.3. We can live with this a bit longer. I'd like to migrate the option names so we don't have to do these conversions anymore.

comment:29 SergeyBiryukov3 years ago

Closed #19465 as a duplicate.

comment:30 stephenh19882 years ago

  • Cc stephenh1988 added

comment:31 ryan2 years ago

  • Milestone changed from Future Release to 3.4

SergeyBiryukov2 years ago

comment:32 SergeyBiryukov2 years ago

18958.3.patch is the refreshed patch from #15970.

comment:33 ryan2 years ago

  • Milestone changed from 3.4 to Future Release

comment:34 nacin2 years ago

  • Milestone changed from Future Release to 3.5

We don't actually need to convert - to _ anymore. We had done that, I'm pretty sure, to avoid having hyphens in usermeta, and thus as properties for a WP_User object. But WP_User does these checks dynamically with has_prop(), __isset(), etc.

All existing screens use - to _, so we might as well keep that. Then we just need to handle hyphen support.

For edit-tags, it calculates the page properly, shows the value in Screen Options properly, but saves improperly. For post types, it only calculated properly — it showed it and saved it improperly.

18958.3.diff makes it easy — it passes 'option' to add_screen_option() for post types (already there for taxonomies), which ensures that the correct option is read from. A logic change in set_screen_options() ensures that we only do a hyphen-to-underscore conversion when not dealing with a post type or taxonomy. Finally, the list tables already properly read from the hyphened post type.

Only remaining thing is, do we clean up the usermeta table? That would require deleting all edit_*_per_page keys where * matched all the converted-to-underscore name of any taxonomy or post type with a hyphen in it.

nacin2 years ago

comment:35 nacin2 years ago

This could work as an upgrade routine:

/**
 * Execute changes made in WordPress 3.5.
 *
 * @since 3.5.0
 */
function upgrade_350() {
	if ( $wp_current_db_version < 99999 && is_main_site() && ! defined( 'DO_NOT_UPGRADE_GLOBAL_TABLES' ) ) {
		$meta_keys = array();
		foreach ( array_merge( get_post_types(), get_taxonomies() ) as $name ) {
			if ( false !== strpos( $name, '-' ) )
			$meta_keys[] = 'edit_' . str_replace( '-', '_', $name ) . '_per_page';
		}
		if ( $meta_keys ) {
			$meta_keys = implode( "', '", $meta_keys );
			$wpdb->query( "DELETE FROM $wpdb->usermeta WHERE meta_key IN ('$meta_keys')" );
		}
	}
}

nacin2 years ago

Converts if to elseif. Rearranges to keep current behavior (taxonomy wins), which is the same behavior as in WP_Screen::get().

comment:36 dominicp2 years ago

  • Cc solaveritasteliberum@… added

comment:37 ryan2 years ago

18958.4.diff works well for me.

comment:38 ryan2 years ago

In [21322]:

Fix per_page screen option for custom post types and taxonomies. Don't convert taxonomy and post type slugs from hyphen to underscore when saving the per_page usermeta. Props nacin. see #18958

comment:39 nacin22 months ago

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

In [21811]:

Remove old edit_*_per_page usermeta keys.

These per-page values were when the post type or taxonomy name has a hyphen.
Previously, these were converted to underscores. This changed in [21322].

fixes #18958.

ocean9022 months ago

Note: See TracTickets for help on using tickets.