Make WordPress Core

Opened 13 years ago

Closed 13 years ago

#19830 closed defect (bug) (fixed)

wp_localize_script() combines statements improperly

Reported by: tobiasbg's profile TobiasBg Owned by: azaozz's profile azaozz
Milestone: 3.4 Priority: normal
Severity: minor Version: 3.3
Component: General Keywords: easy-fix has-patch commit
Focuses: Cc:

Description

When making multiple calls to wp_localize_script() on the same $handle, the individual JSONified objects are merged together with ;\n in this line.
However, each individual var statement has already been correctly terminated by a semicolon ; a few lines above.

Due to that, the resulting output has an extra semicolon after each but the last var statement (i.e. ;;).

Removing the semicolon from the string in line 151 should fix this.

Attachments (2)

semicolonfix.diff (584 bytes) - added by tw2113 13 years ago.
semicolonfix1.diff (584 bytes) - added by tw2113 13 years ago.
Fixed placement of the semicolon

Download all attachments as: .zip

Change History (10)

#1 @scribu
13 years ago

  • Owner set to azaozz
  • Severity changed from normal to minor
  • Status changed from new to assigned

#2 @azaozz
13 years ago

Generally having two consecutive semicolons in JS is acceptable (not only at the end of line). Many JS libraries still add a semicolon at the very beginning (when minimized) to prevent concatenation errors.

The second semicolon could be moved to L:146 as it matters only when there's 'l10n_print_after' part.

#3 @TobiasBg
13 years ago

Yes, that's true, as the two semicolons are not causing any issues/warnings/errors. It's just looking weird, and with just one semicolon the code looks cleaner (I feel).

Moving it from line 151 to 146 looks good to me, as the only time when the semicolon could matter would be, if the semicolon at the end of the legacy l10n_print_after is missing.

#4 @SergeyBiryukov
13 years ago

  • Keywords easy-fix added
  • Milestone changed from Awaiting Review to 3.4

@tw2113
13 years ago

#5 follow-up: @TobiasBg
13 years ago

@tw2113: Thanks for the patch! The moved semicolon should go after the $after variable though, as that variable is the one that we need to "protect" from not having a semicolon. Everything before $after will already have one from line 143.

@tw2113
13 years ago

Fixed placement of the semicolon

#6 in reply to: ↑ 5 @tw2113
13 years ago

  • Keywords has-patch added; needs-patch removed

You're welcome. Fixed with the new diff file.

Replying to TobiasBg:

@tw2113: Thanks for the patch! The moved semicolon should go after the $after variable though, as that variable is the one that we need to "protect" from not having a semicolon. Everything before $after will already have one from line 143.

#7 @SergeyBiryukov
13 years ago

  • Keywords commit added

#8 @azaozz
13 years ago

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

In [19832]:

Move an unsightly semicolon to where it belongs in wp_localize_script(), props tw2113, fixes #19830

Note: See TracTickets for help on using tickets.