Opened 9 years ago
Closed 9 years ago
#14588 closed defect (fixed)
xmppSession.js has duplicate case but different code paths
Reported by: | Douglas Hays | Owned by: | Colin Snover |
---|---|---|---|
Priority: | high | Milestone: | 1.7.3 |
Component: | Dojox | Version: | 1.7.1 |
Keywords: | Cc: | Bryan Forbes | |
Blocked By: | Blocking: |
Description
Starting with the initial checkin of xmppSession.js in [14275]
switch(n.nodeName){ case 'status': case 'show': p[n.nodeName]=n.firstChild.nodeValue; break; case 'status': p.priority=parseInt(n.firstChild.nodeValue); break;
there's a duplicate case for status but the code paths are different and I'm not sure which is correct. This is breaking the RAD tooling build of Dojo.
Attachments (2)
Change History (13)
comment:1 Changed 9 years ago by
Priority: | high → blocker |
---|
comment:3 Changed 9 years ago by
Milestone: | 1.7.2 → 1.7.3 |
---|
1.7.2 RC released, bumping milestone on remaining tickets.
comment:4 Changed 9 years ago by
I've attached a very small patch that would clean up the switch, while preserving current code functionality and restoring the intent that seems to be set by the original line 482. That being said, lines 481-483 will never be ran, so they could theoretically just be cut.
comment:5 Changed 9 years ago by
Keywords: | needsreview added |
---|---|
Priority: | low → high |
Here is the relevant section from the XMPP RFC:
2.2.2.2. Status The OPTIONAL <status/> element contains XML character data specifying a natural-language description of availability status. It is normally used in conjunction with the show element to provide a detailed description of an availability state (e.g., "In a meeting"). The <status/> element MUST NOT possess any attributes, with the exception of the 'xml:lang' attribute. Multiple instances of the <status/> element MAY be included but only if each instance possesses an 'xml:lang' attribute with a distinct language value. 2.2.2.3. Priority The OPTIONAL <priority/> element contains non-human-readable XML character data that specifies the priority level of the resource. The value MUST be an integer between -128 and +127. A presence stanza MUST NOT contain more than one <priority/> element. The <priority/> element MUST NOT possess any attributes. If no priority is provided, a server SHOULD consider the priority to be zero. For information regarding the semantics of priority values in stanza routing within instant messaging and presence applications, refer to Server Rules for Handling XML Stanzas (Section 11).
Here's what happened: someone copied the status line with the intent to change it to "priority" (hence the parseInt and assignment to priority), wrote the right action, but forgot to change the label. So the supplied patch actually horribly breaks the code.
Changed 9 years ago by
Attachment: | rfc_3921.patch added |
---|
Fixed ability to read priority. Changed priority to correct default.
comment:10 Changed 9 years ago by
Keywords: | needsreview removed |
---|---|
Owner: | changed from Dustin Machi to Colin Snover |
Status: | new → assigned |
I’ve committed a one-line fix to the case that just fixes the switch-case. haysmark, according to the RFC quote you provided, since this is a client setting the priority it doesn’t need to be zero by default to conform to the RFC, servers just have to assume no priority means zero. Since that would be a non-bugfix behavioural change I don’t know if it should make it into 1.x or not, I don’t know if there is really a problem with priority being non-zero.
If you‘re cool with the fix I committed please resolve fixed. Otherwise let me know and we can work something out.
Cheers,
comment:11 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Bulk update of open ticket priorities.