WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#15064 closed enhancement (fixed)

Template function for wp-admin submit buttons

Reported by: markjaquith Owned by:
Milestone: 3.1 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

We have a lot of duplicate code surrounding submit buttons. And our HTML hasn't always been the same, so plugins have to keep up. Let's create a submit_button() API function that takes care of this.

Attachments (5)

15064.diff (3.3 KB) - added by markjaquith 10 years ago.
First swing, and a few sample implementations.
15064.2.diff (66.1 KB) - added by sbressler 10 years ago.
Replaces all remaining submit buttons in trunk, fixes some of scribu's changes in [15830]
15064.3.diff (510 bytes) - added by markmcwilliams 10 years ago.
sorts /wp-admin/user-new.php out
15064.4.diff (1.4 KB) - added by sbressler 10 years ago.
Fixes scribu's and Viper007Bond's reported bugs above
15064.dupe-ids.diff (1.2 KB) - added by duck_ 10 years ago.

Download all attachments as: .zip

Change History (44)

@markjaquith
10 years ago

First swing, and a few sample implementations.

#1 @scribu
10 years ago

  • Keywords has-patch added

#2 @scribu
10 years ago

(In [15810]) Introduce submit_button(). Props markjaquith for initial patch. See #15064

#3 @scribu
10 years ago

(In [15830]) Use submit_button() in more places. See #15064

#4 @nacin
10 years ago

  • Keywords needs-patch added; has-patch removed

Quick grep shows a few dozen more locations, easily, that can be patched by someone eager.

#5 @sbressler
10 years ago

I counted 55 files still needing some submit_button love. Attached is a patch for 22 of these.

I added two parameters to the function that I think make it much more usable across core. I also default the id attribute to be the same as the name. I considered adding a separate $id parameter, but decided against that as of yet. That said, there are still some submit inputs in the patched files that need a little more love because the id and name fields are different and thus cannot be addressed by the current submit_button method.

Btw, grep command I ran: grep -R -i -l -c --include=*.php input\.*submit *

#6 @sbressler
10 years ago

  • Cc sbressler@… added

#7 @sbressler
10 years ago

According to my search, these are the files still needing a dose of submit_button:

wp-admin/includes/file.php
wp-admin/includes/image-edit.php
wp-admin/includes/dashboard.php
wp-admin/includes/plugin-install.php
wp-admin/includes/template.php
wp-admin/includes/list-table-comments.php
wp-admin/includes/theme-install.php
wp-admin/includes/list-table-users.php
wp-admin/includes/media.php
wp-admin/includes/list-table-media.php
wp-admin/includes/list-table-plugins.php
wp-admin/includes/class-wp-list-table.php
wp-admin/link-manager.php
wp-admin/widgets.php
wp-admin/media-upload.php
wp-admin/network.php
wp-admin/plugins.php
wp-admin/press-this.php
wp-admin/theme-editor.php
wp-admin/plugin-editor.php
wp-admin/nav-menus.php
wp-admin/setup-config.php
wp-includes/post.php
wp-includes/theme-compat/comments-popup.php
wp-includes/theme-compat/comments.php
wp-includes/comment-template.php
wp-includes/post-template.php
wp-includes/admin-bar/admin-bar-class.php
wp-includes/general-template.php
wp-includes/class-snoopy.php
wp-login.php
wp-signup.php

#8 @sbressler
10 years ago

Oh, and it was actually 23/55 files already done above. Oops

#9 @scribu
10 years ago

  • Keywords has-patch added; needs-patch removed

#10 @sbressler
10 years ago

Whew, all occurrences I could find of submit inputs should now be replaced with this new method.

I changed the API of the method a bit with this patch. I added two parameters:

  • $wrap: Specified whether the button should be wrapped in a paragraph tag. True by default.
  • $other_attributes: Additional attributes that should be added to the submit button, such as accesskey, tabindex, style, id, etc. I'm supporting these attributes being specified in two different ways: either as array( 'tabindex' => '2', 'id' => 'my_id' ) or 'tabindex="2" id="my_id" (props to Nacin for the array idea). I think the array specification looks cleaner, but still allow a straight string specification so that methods like disabled() can be used.

Hopefully my method docs makes this easy to follow.

I am adding an id automatically (set to the param $name) unless another id is specifically specified in $other_attributes (given in array format).

Going through core, $wrap was set to false way more times than true. I didn't want to change the default behavior just yet without Mark's (or someone else's) input, but I certainly would recommend changing the default behavior not to wrap.

The only occurrence of a submit button that I was unable to change to use this new method was in admin-bar-class.php since it seemingly uses an HTML5 button.

@sbressler
10 years ago

Replaces all remaining submit buttons in trunk, fixes some of scribu's changes in [15830]

#11 @markjaquith
10 years ago

(In [16061]) Expand submit_button() capabilities. Replace all (or almost all) manual HTML instances in WP. props sbressler. see #15064

#12 follow-up: @nacin
10 years ago

trunk/wp-admin/user-new.php looks flipped.

@markmcwilliams
10 years ago

sorts /wp-admin/user-new.php out

#13 in reply to: ↑ 12 @markmcwilliams
10 years ago

Replying to nacin:

trunk/wp-admin/user-new.php looks flipped.

The attached patch fixes that! :)

#14 @markmcwilliams
10 years ago

In #wordpress-dev ...

<ptahdunbar1> revision 16061 breaks the setup form

<PeteMall> ptahdunbar: I see a bunch of issues with it... pbly needs a revert

<ptahdunbar> PeteMall: yeah, reverting back to 16060 fixes it.

... so something is wrong?! :(

#15 @nacin
10 years ago

Let's try fixing it instead of a revert. It's probably just a logic error.

#16 @nacin
10 years ago

Quick glance: unusable in wp-includes as we only define it in wp-admin. I would rather leave it for wp-admin.

Also need to pull it out of install.php and setup-config.php as most of WP wouldn't be loaded here.

#17 @nacin
10 years ago

(In [16066]) Revert submit_button() for wp-includes, setup-config, install, login, signup. see [16061], see #15064, fixes #15247.

#18 @WraithKenny
10 years ago

Breaks the theme front end also

Fatal error: Call to undefined function get_submit_button() in [snip]/wp-includes/general-template.php on line 163

#19 @WraithKenny
10 years ago

nm the last, the revert fixes.

#20 @sbressler
10 years ago

Thanks for applying my patch, and my apologies for the use of the function where it shouldn't have been used. In my defense, I asked Nacin specifically about that issue this morning and received no response. So #blamenacin ;)

Any thoughts about setting wrap to false by default?

#21 @sbressler
10 years ago

By the way, for some metrics, in current use (I very well may be off by one or two):
$wrap is set to true 21 times in core and set to false 77 times.
I'd say we should switch the default to be false.

Wow, 98 occurrences - no wonder it took so long :)

Also, I second markmcwilliams patch here. Accidental revert of Scribu's patch from before I had added all the other arguments to make that case work.

#22 @markmcwilliams
10 years ago

I see no notification appeared for my patch, but it's in! [16067]

#23 @nacin
10 years ago

(In [16097]) Fix nested HTML in submit_button(). fixes #15257, see #15064.

#24 @scribu
10 years ago

(In [16107]) Fix class in wp-admin/users.php submit button. See #15064

#25 @nacin
10 years ago

true/false for a wrapper argument is kind of lame/non-descriptive and goes against our philosophy of having clear parameters for functions. We already have a bunch of arguments here.

Wrap is only used 21 times per sbressler. What if we manually wrapped <p>, and assume that most use cases is just I can haz submit button?

Granted, a plugin would need to do <p><?php submit_button(); ?></p> but at least it allows for flexibility without being overbearing to customize.

#26 @scribu
10 years ago

The submit button that creates a new tag used to have an AJAX handler attached.

It's gone now and I think it has to do with submit_button().

#27 @Viper007Bond
10 years ago

The "Update Comment" button on the edit comment (non-AJAX) screen has a big dark grey background.

@sbressler
10 years ago

Fixes scribu's and Viper007Bond's reported bugs above

#28 @sbressler
10 years ago

Above patch fixes the two reported errors above

#29 @scribu
10 years ago

(In [16362]) submit_button() fixes. Props sbressler. See #15064

#30 @nacin
10 years ago

Any objections to killing the wrapper argument? http://core.trac.wordpress.org/ticket/15064#comment:25

#31 @scribu
10 years ago

Well, if you do that, a significant portion of the benefit of the template tag would be lost.

#32 @nacin
10 years ago

Not really. submit_button() will give you a submit button.

Granted, a plugin would need to do <p><?php submit_button(); ?></p> but at least it allows for flexibility without being overbearing to customize.

Or we could split it into two tags, but that seems awkward.

#33 follow-up: @jane
10 years ago

  • Keywords 3.2-early added
  • Milestone changed from 3.1 to Future Release

Punting to 3.2-early as we are entering beta.

#34 in reply to: ↑ 33 @sbressler
10 years ago

Replying to jane:

Punting to 3.2-early as we are entering beta.

All of the diffs currently in the ticket have already been committed to trunk. Will these be reverted now and put back in for 3.2 or are you just punting any future work on this ticket?

#35 @scribu
10 years ago

  • Keywords 3.2-early removed
  • Milestone changed from Future Release to 3.1
  • Resolution set to fixed
  • Status changed from new to closed

#36 @duck_
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

If you pass an ID attribute to submit_button in the $other_attributes parameter then you will end up with a duplicated ID attribute.

Example in wp-admin/user-new.php:

<input type="submit" name="createuser" id="createusersub" class="button-primary" value="Add User " id="createusersub" />

Patch attached.

#37 @automattor
10 years ago

(In [16568]) Do not double up on id attribute for submit_button() if id is provided in other attributes param. props duck_. see #15064

#38 @markjaquith
10 years ago

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

Closing as fixed. For any further issues, please open a new ticket.

#39 @nacin
10 years ago

(In [17145]) Revert [15830] for wp-activate.php. see #15064, fixes #15976.

Note: See TracTickets for help on using tickets.