Make WordPress Core

Opened 6 months ago

Closed 10 days ago

Last modified 10 days ago

#59249 closed defect (bug) (fixed)

shortcode_parse_atts() result for no attributes does not match the documentation

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: swissspidy's profile swissspidy
Milestone: 6.5 Priority: normal
Severity: normal Version:
Component: Shortcodes Keywords: has-patch has-testing-info has-unit-tests commit needs-dev-note
Focuses: Cc:

Description

Background: #59206.

The shortcode_parse_atts() documentation says:

 * @return array|string Array of attribute values keyed by attribute name.
 *                      Returns empty array if there are no attributes.
 *                      Returns the original arguments string if it cannot be parsed.

The middle part (empty array if there are no attributes) appears to be incorrect. In my testing, the function returns an empty string in that case.

Do we want to update the results to match the documentation, or just correct the documentation?

Some history:

Change History (21)

#1 @TobiasBg
6 months ago

Correct, it's an empty string. For more: #26927

#2 @oglekler
5 months ago

From a logical point, it is better when the return value is consistent. In many cases change of return value can introduce a regression or need of change to plugins/themes, but because of each returning in an array attribute needs to be checked before usage anyway, I cannot imagine a regression in this case.
So, I am suggesting to return empty array if there are no actual attributes.

This ticket was mentioned in PR #5367 on WordPress/wordpress-develop by @nicolefurlan.


5 months ago
#3

  • Keywords has-patch added
  • Adds conditional to shortcode_parse_atts() to return an empty array if there are no attributes on the shortcode.
  • Reorders @return options on shortcode_parse_atts() doc block to reflect order of if statement for clarity.

Trac ticket: https://core.trac.wordpress.org/ticket/59249

#4 @nicolefurlan
5 months ago

  • Owner set to nicolefurlan
  • Status changed from new to assigned

I've added a PR with the change that @oglekler suggested 👍

Last edited 5 months ago by nicolefurlan (previous) (diff)

#5 @nicolefurlan
5 months ago

Testing Instructions

To test this change, we should verify that shortcodes continue to generate the appropriate markup on the frontend both with and without attributes.

Steps to Test

  1. Create a post or page and add shortcodes with and without attributes. WordPress has several built-in shortcodes that we can use for testing: https://developer.wordpress.org/plugins/shortcodes/#built-in-shortcodes

e.g.

[caption id="id" caption_id="caption_id" align="aligncenter" width="300" caption="caption text" class="class"]Caption with attributes[/caption]
[caption]Caption without attributes[/caption]

Expected Results

  • Shortcodes should continue to generate the appropriate markup on the frontend both with and without attributes.

#6 @nicolefurlan
5 months ago

  • Keywords has-testing-info added

#7 @oglekler
5 months ago

  • Keywords needs-testing added

#8 @swissspidy
5 months ago

Is this a duplicate of #45929?

Version 0, edited 5 months ago by swissspidy (next)

This ticket was mentioned in Slack in #core by oglekler. View the logs.


4 months ago

#10 @swissspidy
4 months ago

#59613 was marked as a duplicate.

#11 @bedas
4 months ago

The patch here still does not grant consistent output and adds an additional unnecessary if check when we can just do this:
https://core.trac.wordpress.org/ticket/59613
https://github.com/WordPress/wordpress-develop/pull/5480

There is no reason why we should return a string in any case. It disrupts dependant code, which then needs to "juggle" whether or not it receives expected input.
Please see the linked trac (and pr) which has been marked as duplicate.

The patch committed here would nor resolve the issues this problem causes in dependant code.

Last edited 4 months ago by bedas (previous) (diff)

#12 @nicolefurlan
4 months ago

  • Milestone changed from 6.4 to 6.5

@SergeyBiryukov or @swissspidy do you have any thoughts on #comment:11?

@aaroncampbell pinging you as well as the component maintainer.

I'm happy to update the PR whenever we can reach a consensus.

Since this patch possibly needs an update and definitely still needs testing, I'm moving it to 6.5.

#13 @swissspidy
4 months ago

  • Keywords needs-unit-tests added

A simplified patch like https://github.com/WordPress/wordpress-develop/pull/5480 looks good. Just needs correct docblock adjustments and ideally unit tests.

#14 @swissspidy
4 weeks ago

#45929 was marked as a duplicate.

This ticket was mentioned in PR #6015 on WordPress/wordpress-develop by @swissspidy.


3 weeks ago
#15

  • Keywords has-unit-tests added; needs-unit-tests removed

I don't see harm in always returning an empty array here.

Reverts https://core.trac.wordpress.org/changeset/56488 because it's no longer needed.

Trac ticket: https://core.trac.wordpress.org/ticket/59249

#16 @johnbillion
10 days ago

  • Keywords commit added; 2nd-opinion needs-testing removed
  • Owner changed from nicolefurlan to swissspidy

#17 @swissspidy
10 days ago

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

In 57597:

Shortcodes: Always return an array in shortcode_parse_atts().

Previously, shortcode_parse_atts() would return the input (an empty string) if a shortcode had no attributes, even though the documentation said otherwise.

Always returning an (empty) array reduces confusion and improves developer experience as the return value does not have to be manually checked in the shortcode itself.

Props: nicolefurlan, swissspidy, johnbillion, bedas.
Fixes #59249.

#20 @desrosj
10 days ago

Wondering if we should call this out in the miscellaneous dev note? Even if just by a bullet point, it could help someone out that's relying on the incorrect behavior.

#21 @swissspidy
10 days ago

  • Keywords needs-dev-note added

Sure why not 👍 Either a misc dev note or the field guide I suppose

Note: See TracTickets for help on using tickets.