WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 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 6 years ago.
First swing, and a few sample implementations.
15064.2.diff (66.1 KB) - added by sbressler 6 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 6 years ago.
sorts /wp-admin/user-new.php out
15064.4.diff (1.4 KB) - added by sbressler 6 years ago.
Fixes scribu's and Viper007Bond's reported bugs above
15064.dupe-ids.diff (1.2 KB) - added by duck_ 6 years ago.

Download all attachments as: .zip

Change History (44)

@markjaquith
6 years ago

First swing, and a few sample implementations.

#1 @scribu
6 years ago

  • Keywords has-patch added

#2 @scribu
6 years ago

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

#3 @scribu
6 years ago

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

#4 @nacin
6 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
6 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
6 years ago

  • Cc sbressler@… added

#7 @sbressler
6 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
6 years ago

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

#9 @scribu
6 years ago

  • Keywords has-patch added; needs-patch removed

#10 @sbressler
6 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
6 years ago

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

#11 @markjaquith
6 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
6 years ago

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

@markmcwilliams
6 years ago

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

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

Replying to nacin:

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

The attached patch fixes that! :)

#14 @markmcwilliams
6 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
6 years ago

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

#16 @nacin
6 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
6 years ago

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

#18 @WraithKenny
6 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
6 years ago

nm the last, the revert fixes.

#20 @sbressler
6 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
6 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
6 years ago

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

#23 @nacin
6 years ago

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

#24 @scribu
6 years ago

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

#25 @nacin
6 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
6 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
6 years ago

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

@sbressler
6 years ago

Fixes scribu's and Viper007Bond's reported bugs above

#28 @sbressler
6 years ago

Above patch fixes the two reported errors above

#29 @scribu
6 years ago

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

#30 @nacin
6 years ago

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

#31 @scribu
6 years ago

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

#32 @nacin
6 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
6 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
6 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
6 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_
6 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.

@duck_
6 years ago

#37 @automattor
6 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
6 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
6 years ago

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

Note: See TracTickets for help on using tickets.