If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Plugin ID invalid message doesn't match the test

RESOLVED FIXED

Status

Other Applications
ChatZilla
--
minor
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Stewart, Assigned: James Ross)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cz-0.9.87])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.19) Gecko/20090102 SeaMonkey/1.1.14
Build Identifier: ChatZilla 0.9.86 [SeaMonkey 1.1.14/0000000000]

According to the error message, an ID can contain alphanumeric characters, dash (_) and hyphen (-).

According to the code, an ID can contain alphabetic characters (no numbers), dash (_) and hyphen (-).

Oops?

Reproducible: Always

Steps to Reproduce:
1. In a plugin, set:
    plugin.id = "seamonkey-1_1_14-menu-fix";
   (for example)
2. Attempt to load plugin

Actual Results:  
Plugin <...> does not have a valid id. Plugin ids may only contain alphanumeric characters, underscores (_) and dashes (-).


Expected Results:  
Plugin “seamonkey-1_1_14-menu-fix” is now enabled.
(Reporter)

Comment 1

7 years ago
Created attachment 483682 [details] [diff] [review]
Fixes the ID check to match the error message
Attachment #483682 - Flags: review?
(Reporter)

Comment 2

7 years ago
Created attachment 483683 [details] [diff] [review]
Fixes the ID check to match the error message (v2)

Missed a spot. :(
Attachment #483682 - Attachment is obsolete: true
Attachment #483683 - Flags: review?(gijskruitbosch+bugs)
Attachment #483682 - Flags: review?

Comment 3

7 years ago
It would be preferable to refactor that bit of code so that the conditions aren't all repeated.

Also, \w includes \d. Assuming your intention was to force the ID to start with a letter, the regex you want is /^[A-Za-z][\w-]*$/
Assignee: rginda → sandreas41
Severity: normal → minor
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 4

7 years ago
Created attachment 521215 [details] [diff] [review]
Add numbers to allowed regexp
Assignee: sandreas41 → silver
Attachment #483683 - Attachment is obsolete: true
Attachment #521215 - Flags: review?(samuel)
Attachment #483683 - Flags: review?(gijskruitbosch+bugs)

Comment 5

7 years ago
Comment on attachment 521215 [details] [diff] [review]
Add numbers to allowed regexp

It's a little odd that we're checking all the conditions twice...  (Yes, I do kind of see the reason for it there.)
Attachment #521215 - Flags: review?(samuel) → review+

Comment 6

7 years ago
Why not just match it once and save the result in a local variable?
(Assignee)

Comment 7

7 years ago
Created attachment 522343 [details] [diff] [review]
Correct regexp and remove duplication of checks
Attachment #521215 - Attachment is obsolete: true
Attachment #522343 - Flags: review?(samuel)

Updated

7 years ago
Attachment #522343 - Flags: review?(samuel) → review+
(Assignee)

Comment 8

7 years ago
http://hg.mozilla.org/chatzilla/rev/0b940fd89a46
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Whiteboard: [cz-0.9.87]
You need to log in before you can comment on or make changes to this bug.