Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#26927 closed defect (bug) (duplicate)

Fix docs of shortcode_parse_atts()

Reported by: tobiasbg's profile TobiasBg Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.5
Component: Shortcodes Keywords: has-patch commit
Focuses: docs Cc:


While debugging an issue with a Shortcode without attributes, like [shortcode], I tripped over the unexpected return value of shortcode_parse_atts() in that case (an empty string).
Instead of returning an (empty) array, which would be much more fitting and in line with what the function does, it returns an (empty) string.

The problem: This was added in [6939] for #5892 six years ago. :-/
I'm not exactly sure for what cases this was necessary after [6911] for #5914 was already in, but presumably there's some edge case strings that the huge regex in shortcode_parse_atts() does not match.

I would love to make shortcode_parse_atts() return arrays consistently, especially as most people paste the return value to shortcode_atts() which per documentation expects an array (but also accepts the string), or explicitely use it as an array in their Shortcode handlers.
It should be enough to change the return value in the else branch to

$atts = array( ltrim($text) );

However, as this has been around for six years, and as the edge case with the empty string is mentioned in the, and as we have specific unit tests that check for that empty string, we probably can't change this without breaking back-compat for some plugins :-(

Then, at least, let's fix the inline docs to make the return value clear.

Attachments (2)

26927.patch (1.2 KB) - added by TobiasBg 10 years ago.
Fix inline docs, as fixing the function's return value is not really feasible
26927.2.patch (1.3 KB) - added by DrewAPicture 10 years ago.
Better long description / wrapping

Download all attachments as: .zip

Change History (12)

10 years ago

Fix inline docs, as fixing the function's return value is not really feasible

#1 @TobiasBg
10 years ago

#23307 also raises this issue, and gives another good reason for why a consistent array return value would be better.

#2 @kpdesign
10 years ago

  • Focuses docs added
  • Keywords has-patch docs-feedback added

#3 @kpdesign
10 years ago

  • Type changed from enhancement to defect (bug)

This ticket was mentioned in IRC in #wordpress-dev by DrewAPicture. View the logs.

10 years ago

#5 @DrewAPicture
10 years ago

  • Keywords docs-feedback removed
  • Milestone changed from Awaiting Review to 3.9

10 years ago

Better long description / wrapping

#6 @DrewAPicture
10 years ago

  • Keywords commit added

26927.2.patch builds off of 26927.patch by rewriting the long description to reduce redundancy and fix line-wrapping on the @return description.

#7 follow-up: @SergeyBiryukov
10 years ago

Per nacin's comment in #23307, consistently returning an array might be better here.

#8 in reply to: ↑ 7 @DrewAPicture
10 years ago

Replying to SergeyBiryukov:

Per nacin's comment in #23307, consistently returning an array might be better here.

If you guys want to haggle over a code change in the other ticket, have at it. Docs can always catch up :)

#9 @SergeyBiryukov
10 years ago

Documented values, on the other hand, are harder to change :)

I'd suggest closing this ticket as a duplicate of #23307 and making a decision there.

#10 @TobiasBg
10 years ago

  • Milestone 3.9 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Yes, fixing the return value for that edge case (in #23307) is definitely the better solution, especially as backward compat issues seem to be neglectable according to nacin. Most of Drew's work on the docs can of course be reused there.
Fixing the documentation now and then still making a code change is indeed more problematic.

Closing in favor of #23307 which has some work on the code changes already.

Note: See TracTickets for help on using tickets.