Closed Bug 722989 Opened 12 years ago Closed 9 years ago

0-0 (etc) not a valid package name? if so the message is misleading

Categories

(Add-on SDK Graveyard :: General, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: k0scist, Assigned: evold)

Details

Attachments

(2 files)

(addon-sdk)│cfx xpi
Error: the name of your package contains upper-case letters.
Package names can contain only lower-case letters, numbers, underscores, and dashes.
Current package name: 0-0
(Pdb) name
u'0-0'
(Pdb) name.islower()
False
(Pdb) '12345'.islower()
False
(Pdb) 'a12345'.islower()
True

So I'm not sure if this should be an allowable package name or not, but in any case the error message is misleading as '0-0' contains nothing but numbers and a dash
Hrm. This page says that "It return true if all cased characters in the string are lowercase and there is at least one cased character, false otherwise"...
http://www.tutorialspoint.com/python/string_islower.htm

If that's true, then you HAVE to have at least one letter in there to pass that test...
From my (really quick) testing (generating the xpi with the package name "m0-0", then replacing every instance of "m0-0" with "0-0" within the XPI), everything seems to work correctly. The extension is installed, and the code that's generated via cfx init all appears to work.

So I'd guess this is just a weird edge case where islower() is being too strict?
Yep. I just happened across it naming addons '%d-%d' % (0, 0) for testing.  No big deal, now I'm naming them 'a%d-%d' % (0,0) but thought the error message should be fixed.
The real test should probably be name.lower()==name, since the reason for the restriction is that the name gets used in the hostname-ish part of a resource:// URI, and (inappropriately, in my opinion) it gets downcased when used. (The "authority" portion of a URL maps to a DNS name for many protocols, but resource:// doesn't need to follow that lead. Also, DNS lookups are case-insensitive, but that doesn't mean you have to downcase the string before handing it to your resolver library).

I vaguely recall doing some experiments that showed upper-case letters in the addon name getting used to register the resource:// protocol handler, but then get downcased any time the loader attempted to fetch anything, resulting in the XPI's contents being unretrievable. The secondary concern was name collisions between addons that only differ in case. So forbidding uppercase seemed like the easiest answer.
So, something like this?
Attachment #593480 - Flags: feedback?(warner-bugzilla)
Actually, looking at the code, we can delete that whole clause. There's a RESOURCE_HOSTNAME_RE.match() just afterwards which will only match on lower-case letters, so the extra islower() check is redundant. And the doctests just before it should provide the right test coverage.

So let's just delete that first "if" statement.
Comment on attachment 593480 [details] [diff] [review]
name.lower() == name

Ah, Wes pointed out that the two checks have different error messages: the first one specifically mentions upper-case characters, while the regexp one just says "invalid character". Given that upper-case characters are pretty likely (it's not particularly surprising that "&" is illegal, but there's no good reason for "A" to be disallowed), having the separate message is a good idea.

So yeah, that patch looks great. The only thing I'd change is to make it "if name.lower() != name", as that reads a bit better.
Attachment #593480 - Flags: feedback?(warner-bugzilla) → feedback+
Comment on attachment 593480 [details] [diff] [review]
name.lower() == name

looks good! I'd still suggest using "if name.lower() != name" instead of "if not name.lower() == name", though.
Attachment #593480 - Flags: review?(warner-bugzilla) → review+
Wes did this ever land?
Flags: needinfo?(kwierso)
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/d25827ff7cbfa61426ebac7ef4843a3f4256b97c
Bug 722989 - 0-0 (etc) not a valid package name? if so the message is misleading r=@warner
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
This needs a test case to make sure we don't regress.
Assignee: nobody → evold
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8536658 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1753

I don't think we should support package names that are not supported by npm, which is the case here as far as I know.
Attachment #8536658 - Flags: review?(rFobic) → review-
Comment on attachment 8536658 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1753

There is a package called 5 on npm https://www.npmjs.com/package/5
Attachment #8536658 - Flags: review- → review?(rFobic)
Attachment #8536658 - Flags: review?(rFobic) → review?(jsantell)
Comment on attachment 8536658 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1753

Looks like a valid name
Attachment #8536658 - Flags: review?(jsantell) → review+
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/4374a99cd1c1ddc5ed37487411a6b1e2c1fe0b51
Bug 722989 adding tests for names without letters

https://github.com/mozilla/addon-sdk/commit/46029f09b2c4bd079b0c51bc967bbc69c5bee961
Merge pull request #1753 from erikvold/722989

Bug 722989 adding tests for names without letters r=jsantell
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: