Make WordPress Core

Opened 3 months ago

Last modified 7 weeks ago

#59249 assigned defect (bug)

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

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: nicolefurlan's profile nicolefurlan
Milestone: 6.5 Priority: normal
Severity: normal Version:
Component: Shortcodes Keywords: 2nd-opinion has-patch has-testing-info needs-testing needs-unit-tests
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 (13)

#1 @TobiasBg
3 months ago

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

#2 @oglekler
2 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.


2 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
2 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 2 months ago by nicolefurlan (previous) (diff)

#5 @nicolefurlan
2 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
2 months ago

  • Keywords has-testing-info added

#7 @oglekler
2 months ago

  • Keywords needs-testing added

#8 @swissspidy
2 months ago

Is this a duplicate of #45929?

Edit: looks the other way around

Last edited 2 months ago by swissspidy (previous) (diff)

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


8 weeks ago

#10 @swissspidy
8 weeks ago

#59613 was marked as a duplicate.

#11 @bedas
8 weeks 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 8 weeks ago by bedas (previous) (diff)

#12 @nicolefurlan
7 weeks 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
7 weeks 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.

Note: See TracTickets for help on using tickets.