[wp-trac] [WordPress Trac] #59613: Ensure $atts passed to $callback of add_shortcode() is always an array

[WordPress Trac]

#59613: Ensure $atts passed to $callback of add_shortcode() is always an array
-------------------------------------+-------------------------------------
 Reporter:  bedas                    |      Owner:  (none)
     Type:  defect (bug)             |     Status:  new
 Priority:  normal                   |  Milestone:  Awaiting Review
Component:  Shortcodes               |    Version:
 Severity:  normal                   |   Keywords:  has-patch 2nd-opinion
  Focuses:  docs, performance,       |  dev-feedback
  sustainability, php-compatibility  |
-------------------------------------+-------------------------------------
 == Problem

 According to
 [https://developer.wordpress.org/reference/functions/add_shortcode/ the
 documentation]:

 > `$callback` callable Required
 > The callback function to run when the shortcode is found.
 > Every shortcode callback is passed three parameters by default,
 including an array of attributes (`$atts`), the shortcode content or null
 if not set (`$content`), and finally the shortcode tag itself
 (`$shortcode_tag`), in that order.

 However, this part...
 > including an **array** of attributes (`$atts`)
 ...is not always true:
 - if the shortcode is used without any attributes, then `$atts` is
 actually an empty string.
 - apparently (according to code docbloc) it would also be a string of
 original arguments if it could not be parsed.

 This discrepancy causes other dependant code to always fumble with the
 type of `$atts`, for example the
 [https://developer.wordpress.org/reference/functions/shortcode_atts/
 source code of shortcode_atts] has to typecasts `$atts` to array.
 **This would not be necessary, would `$atts` indeed be always an array
 from the start.**

 Further, in more complex programming scenarios where we assume `$atts` to
 be an array (as stated by the documentation), again we have to fumble with
 `$atts`, and in some cases, we cannot do anything but just accept that it
 may or may not be an array.
 ''You can read more on that [https://wordpress.org/support/topic/callback-
 of-add_shortcode-is-not-always-an-array/ here]''

 == Cause

 Thanks to the kind help of [https://wordpress.org/support/users/bcworkz/
 @bcworkz] and further testing, I can confirm that the issue is within
 `shortcode_parse_atts` function - to be precise in the `else` statement
 [https://core.trac.wordpress.org/browser/tags/6.3/src/wp-
 includes/shortcodes.php?marks=395-395#L601 here].
 If the checks for `if ( preg_match_all( $pattern, $text, $match,
 PREG_SET_ORDER ) ) ` fail, `$atts` is transformed to string using `ltrim`.

 This makes thus poor sense to me:
 1. First of all, the documentation mentions that `$atts` **is an array**.
 It never mentions other possible types.
 2. It seem quite clear to me that `$atts` is always supposed and intended
 to be an ''array'' - never a string.
 3. Even if the docblock for `shortcode_parse_atts` states that the
 returned value can be array ''or'' string (and if string, then the
 `original arguments string if it cannot be parsed`)... I do not see the
 actual reason or justification for this.

 What is the usefulness of returning a string of arguments if it cannot be
 parsed or is empty?
 It should then safely fail into an empty ''array'', to keep consistency
 and not force dependant code to typecast `$atts`.

 == Proposal

 Simply remove the `else` block in `shortcode_parse_atts`.
 It is redundant and creates issues in particular coding structures + clear
 discrepancies in documentation.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/59613>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform