Opened 11 years ago

Closed 11 years ago

#3504 closed Bug (fixed)

CKPackager creates invalid JavaScript

Reported by: Josh Nisly Owned by: Wiktor Walc
Priority: Normal Milestone: CKEditor 3.0
Component: Project : CKPackager Version: SVN (CKEditor) - OLD
Keywords: Confirmed 3.0RC Review+ Cc:

Description

When packing the en.js lang file, the packager mungers the file in such a way to cause the JavaScript Lint tool to complain. The specific problem is that in the "colors" object, it removes the quotes from the members. For example, "'008080' : 'Teal'," becomes "008080:'Teal',".

Attachments (2)

3504.patch (536 bytes) - added by Wiktor Walc 11 years ago.
3504_2.patch (1.8 KB) - added by Frederico Caldeira Knabben 11 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Confirmed 3.0RC added
Milestone: CKEditor 3.0
Owner: set to Wiktor Walc

I've confirmed it in the nightly build. Some of the color entries are inside single quotes, while others don't have it. We should have some checks that automatically add the quotes if the name starts with a number.

Actually, I would take a look at the ECMA specs to properly check whether or not to have quotes there.

Changed 11 years ago by Wiktor Walc

Attachment: 3504.patch added

comment:2 Changed 11 years ago by Wiktor Walc

Keywords: Review? added

In the Ecma-262 specs (7.8.3 Numeric Literals), we have the following:

NumericLiteral ::
DecimalLiteral
HexIntegerLiteral

and later on:

DecimalLiteral ::
DecimalIntegerLiteral...
(...)
DecimalIntegerLiteral ::
0
NonZeroDigit DecimalDigitsopt

..so only 0 and numbers starting with 1-9 are ok. Because numbers starting with 0 are treated as octal numbers (Annex B, B.1.1), I think all we need to do is to modify regular expression in CKPackager and put numbers in single quotes if they start with '0'.

Changed 11 years ago by Frederico Caldeira Knabben

Attachment: 3504_2.patch added

comment:3 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

The fix looks good. I've added some tests for it with the new patch. Please use it for committing, if you think the changes are good.

comment:4 Changed 11 years ago by Wiktor Walc

Resolution: fixed
Status: newclosed

Fixed with [3501] and [3502] (I have used the second patch with tests included).

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy