WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#19830 closed defect (bug) (fixed)

wp_localize_script() combines statements improperly

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

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 8 years ago.
semicolonfix1.diff (584 bytes) - added by tw2113 8 years ago.
Fixed placement of the semicolon

Download all attachments as: .zip

Change History (10)

#1 @scribu
8 years ago

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

#2 @azaozz
8 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
8 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
8 years ago

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

@tw2113
8 years ago

#5 follow-up: @TobiasBg
8 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
8 years ago

Fixed placement of the semicolon

#6 in reply to: ↑ 5 @tw2113
8 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
8 years ago

  • Keywords commit added

#8 @azaozz
8 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.