WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 5 years ago

#23477 closed defect (bug) (fixed)

r21789 creates a breaking change in behavior for get_submit_button

Reported by: vhauri Owned by: afercia
Milestone: 4.8 Priority: normal
Severity: normal Version: 3.5
Component: Administration Keywords: has-patch
Focuses: docs Cc:

Description (last modified by nacin)

A recent change made to get_submit_button (http://core.trac.wordpress.org/changeset/21789/trunk/wp-admin/includes/template.php) has caused an unwanted change in behavior for a button on a site I maintain. Previously, the string passed as the second argument, $type, was matched against a switch statement with several unique cases. In the event of a non-match in the switch, the default action prior to [21789] was to assign the parameter's value as the $class string without altering it.

The change introduced in [21789] now breaks a string into individual classes, which prevents the use of the unique class names within a string of additional class names. For example, the string button-secondary save-button action used to generate a class string in HTML of button-secondary save-button action, but now generates the unexpected result button save-button action (note lack of '-secondary') because it matches against the following if check on line 1600:

if ( 'secondary' === $t || 'button-secondary' === $t )

It should be noted that the phpdoc associated with the function does not name the use case that I was employing, but that the behavior of passing in a space-separated class string has previously resulted in the string being passed through untouched (and the comment here indicates it was known and expected behavior (http://core.trac.wordpress.org/browser/trunk/wp-admin/includes/template.php?rev=21781#L1649), and this is no longer the case. Not sure if this is worth a programmatic fix, or if it might just be worth noting in the function docblock that certain classes are now 'reserved' and cannot be passed into the $type string.

Attachments (2)

allow-secondary-button-class.patch (5.7 KB) - added by jakemgold 8 years ago.
Allows dev to specify arbitrarily unallowed classes, while removing those class requests in core.
23477.diff (1.7 KB) - added by afercia 5 years ago.
Updated docs.

Download all attachments as: .zip

Change History (18)

#1 @vhauri
8 years ago

  • Severity changed from normal to minor

#2 @vhauri
8 years ago

  • Summary changed from Change 21789 creates a breaking change in behavior for get_submit_button to Change #21789 creates a breaking change in behavior for get_submit_button

#3 @nacin
8 years ago

  • Description modified (diff)
  • Summary changed from Change #21789 creates a breaking change in behavior for get_submit_button to r21789 creates a breaking change in behavior for get_submit_button

Could you provide a specific example, with expected and actual results? Could make it easier to understand.

#4 @vhauri
8 years ago

Clarification of above example:

Function call:

$button_str = get_submit_button( __( 'delete selected users' ), 'button-secondary save-button action', false, false, array( 'id' => 'doaction' ) );

Expected value of $button_str:

<input type="submit" name="" id="doaction" class="button-secondary save-button action" value="delete selected users">

Actual value of $button_str:

<input type="submit" name="" id="doaction" class="button save-button action" value="delete selected users">

#5 @SergeyBiryukov
8 years ago

  • Component changed from Template to Administration
  • Keywords ui-focus added
  • Version changed from trunk to 3.5

#6 @helen
8 years ago

Still a little confused, perhaps because the ticket sounds like it's a general breakage. What I'm seeing here is that button-secondary and secondary will silently not come through as classes if specified in type, because they are now replaced by just using the generic button class.

#7 @jakemgold
8 years ago

Agree with Vasken. Developers should still be able to manually specify "button-secondary" - no reason to expect that it would disappear.

We should remove all references to button-secondary in calls to this function elsewhere in core and allow button-secondary as a specified class.

#8 @jakemgold
8 years ago

  • Cc jake@… added

#9 @jakemgold
8 years ago

Attaching patch that removes the skip over "button-secondary" and "secondary", and also removes the class from the argument in core buttons created using this function (which has an... odd effect of having a few empty strings passed as arguments).

As a side note, there are a decent number of hardcoded HTML buttons using those classes (which I left untouched).

@jakemgold
8 years ago

Allows dev to specify arbitrarily unallowed classes, while removing those class requests in core.

#10 @jakemgold
8 years ago

  • Keywords has-patch added

#11 @chriscct7
6 years ago

  • Keywords ui-feedback dev-feedback added
  • Severity changed from minor to major

#12 @afercia
5 years ago

  • Keywords close added; dev-feedback removed
  • Severity changed from major to normal

I'd be in favor of closing this ticket. The CSS class button-secondary should simply not be used, because buttons using it:

  • they would be not responsive
  • the focus style is wrong
  • the button base class looks exactly the same (and responsiveness and focus are OK)

#13 follow-up: @SergeyBiryukov
5 years ago

For reference, the core usage of secondary and button-secondary was cleaned up (as suggested in allow-secondary-button-class.patch) in [35182] and [38672].

The docs, however, still mention secondary:

  • submit_button():
     * @param string       $type             Optional. The type and CSS class(es) of the button. Core values
     *                                       include 'primary', 'secondary', 'delete'. Default 'primary'
    
  • get_submit_button():
     * @param string       $type             Optional. The type of button. Accepts 'primary', 'secondary',
     *                                       or 'delete'. Default 'primary large'.
    

It might be a good idea to remove it from here too.

@afercia
5 years ago

Updated docs.

#14 in reply to: ↑ 13 @afercia
5 years ago

  • Focuses docs added; ui removed
  • Keywords ui-feedback close removed

Replying to SergeyBiryukov:

The docs, however, still mention secondary

New patch updates the docs to only mention 'primary', 'small', and 'large'.

#15 @afercia
5 years ago

  • Milestone changed from Awaiting Review to 4.8
  • Owner set to afercia
  • Status changed from new to assigned

#16 @SergeyBiryukov
5 years ago

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

In 39936:

Docs: After [38672], update $type parameter description for submit_button() and get_submit_button().

Remove the mention of secondary and delete classes as core values, add small and large.

Props afercia.
Fixes #23477.

Note: See TracTickets for help on using tickets.