Opened 13 years ago
Closed 12 years ago
#18958 closed defect (bug) (fixed)
Can't set "Show on screen" for Custom post types
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 3.5 | Priority: | normal |
Severity: | normal | Version: | 3.3 |
Component: | Administration | Keywords: | has-patch early |
Focuses: | Cc: |
Description (last modified by )
As above. The posts_per_page "show on screen" function on custom post pages doesn't apply/save.
Attachments (9)
Change History (48)
#3
follow-ups:
↓ 4
↓ 21
@
13 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.
#4
in reply to:
↑ 3
@
13 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.
#5
@
13 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.
#6
@
13 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') ) );
#7
@
13 years ago
With this code, "Show on screen" also seems to work for me in current trunk (aside from the initial checkboxes inconsistency I've mentioned). The meta key saved is metaboxhidden_test-cpt
.
#8
@
13 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.
#10
@
13 years ago
and just to confirm, it is the dash in the CPT name. 'test_cpt' works whereas, 'test-cpt' doesn't.
#11
@
13 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
?
#12
@
13 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.
#13
follow-up:
↓ 14
@
13 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.
#14
in reply to:
↑ 13
@
13 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.
#15
@
13 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.
#18
@
13 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.
#21
in reply to:
↑ 3
@
13 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.
#22
@
13 years ago
The change to passing around objects also confused get_hidden_meta_boxes(), as $screen->base is 'post', rather than the post type.
#25
in reply to:
↑ 24
@
13 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.
#26
follow-up:
↓ 27
@
13 years ago
So we need your .2.path here or an updated one of mine from #15970?
#27
in reply to:
↑ 26
@
13 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.
#28
@
13 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.
#34
@
13 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.
#35
@
13 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')" ); } } }
@
13 years ago
Converts if to elseif. Rearranges to keep current behavior (taxonomy wins), which is the same behavior as in WP_Screen::get().
Seems okay here. What's the CPT name? Maybe the switch to sanitize_key() busted something.