This behavior was introduced in the patch which first added to ability to convert PHP arrays to JavaScript arrays. It was committed in CVS commit 25401. No rationale was given for the behavior. The attached patch fixes it.

CommentFileSizeAuthor
#18 drupal_to_js-array-DRUPAL-4.7.patch1.2 KBDarren Oh
#14 drupal_to_js-array-DRUPAL-5.patch1.2 KBDarren Oh
#12 drupal_to_js-array_1.patch1.15 KBnedjo
#11 drupal_to_js-array_0.patch1.17 KBnedjo
#5 common.inc-4.7_0.patch1.1 KBDarren Oh
#3 drupal_to_js-array.patch1.06 KBnedjo
#2 common.inc-4.7.patch982 bytesDarren Oh
common.inc__0.patch970 bytesDarren Oh
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented
Title: drupal_to_js converts arrays with less than two keys to objects » drupal_to_js converts empty arrays to objects
Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented
FileSize
common.inc-4.7.patch982 bytes

Here's a Drupal 4.7 patch for Chat Room users.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
FileSize
drupal_to_js-array.patch1.06 KB

Yes, if empty, it's more appropriate that the array be returned as [] than {}.

However, we shouldn't break the existing conversion. The current logic is to output arrays when we have sequential integer keys beginning with 0, and otherwise use objects. This is in line with Javascript conventions. Yes, it's possible to use string or non-integer keys for Javascript arrays, but doing so breaks many common Javascript array handling methods and approaches.

So falling through and converting arrays with string or non-sequential numerical keys as objects is appropriate. What was missed is just the empty case.

Attached patch adds a test for an empty array, plus adds some code comments to clarify the logic. This addresses the issue without changing current conversion logic.

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented
Status: Needs review » Reviewed & tested by the community

Beautiful, nedjo! The patch solves the problem. Here's a patch for Drupal 4.7.

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented
FileSize
common.inc-4.7_0.patch1.1 KB

Beautiful, nedjo! The patch solves the problem. Here's a patch for Drupal 4.7.

Dries’s picture

Dries CreditAttribution: Dries commented

I'm not Javascript expert yet, but this looks good to me. It would, however, be helpful to explain the code comments _why_ we convert to an array (and _why_ we convert to an object). I don't fully understand why we insist on returning an array ...

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented

These two lines of JavaScript taught me the consequences of this error:

        var j = Math.round(chatroom.userColours.length * Math.random());
        if (chatroom.userColours[j].unUsed) {

If chatroom.userColours is an empty array, j is zero. If chatroom.userColours is an empty object, j is NaN. When j is NaN, the whole script aborts on the line 2. So the purpose of returning an array is to prevent NaN from being used as an array key.

Crell’s picture

Crell CreditAttribution: Crell commented

There's some potentially relevant commentary on the subject of JavaScript notation here: http://killersoft.com/randomstrings/2007/02/28/php-and-json-cut-987/ . (Found courtesy Planet PHP.)

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented

I read through the article, but it doesn't seem to be relevant. The main reason to return an array is that it has a length property which some scripts depend on. Do we really need a lengthy explanation of this in the comments?

Dries’s picture

Dries CreditAttribution: Dries commented

Do you have the snippet handy that shows where the array comes from (the PHP code)? Sorry for getting a little bit of topic, but I really like to grok the bigger issue.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
FileSize
drupal_to_js-array_0.patch1.17 KB

I'll try to explain better, and fix the comment.

In Drupal, we often use arrays with string keys. In Javascript, associative arrays are considered harmful:

An array is an ordered set of values associated with a single variable name. Note that you shouldn't use it as an associative array, use Object instead., http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Globa...

So we apply a test--if it looks like a Javascript array (keys of 0,1,2, etc.), we convert it as an array. If not, we convert it as an object, which is the more appropriate Js equivalent of e.g. a Drupal Forms API array, where we have strings for keys.

This is all working already. The only issue addressed here (besides documentation) is, what to do when the array is empty? The current code says, "I don't find a range of whole number keys starting with 0, so output it as an object". The patch says, if it's empty, that's a valid array, so convert it as an array.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
FileSize
drupal_to_js-array_1.patch1.15 KB

More to the point, perhaps, the JSON array notation simply won't accept associative keys--it's not an option. (JSON arrays are constructed as [value1, value2, ...]; the keys are assigned automatically.) I've changed the comment to clarify this.

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented

PHP code snippet for Dries:

  $js = array(
    'chatId' => $chat->ccid,
    'lastMsgId' => 0,
    'cacheTimestamp' => $cache_timestamp,
    'updateCount' => 0,
    'chatUrl' => $base_url .'/'. $chatroom_base .'/chatroomread.php',
    'kickUrl' => url('chatrooms/kicked/user'),
    'banUrl' => url('chatrooms/banned/user'),
    'userBase' => drupal_get_path('module', 'user'),
    'chatroomBase' => $chatroom_base,
    'smileysBase' => drupal_get_path('module', 'smileys'),
    'smileysMarker' => '------',
    'userUrl' => url('user/'),
    'chatCacheFile' => $cache_file,
    'updateInterval' => $chat->poll_freq,
    'idleInterval' => $chat->idle_freq,
    'userList' => $ol_users,
    'onlineList' => variable_get('chatroom_online_list', 0),
    'userColours' => array(),
    'sessionId' => $session_id,
    'timezone' => $timezone,
  );
  $js = (object) $js;
  $js = drupal_to_js($js);

I made this to work with my version of the patch by converting the main array to an object. That line would be unnecessary now.

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented
FileSize
drupal_to_js-array-DRUPAL-5.patch1.2 KB

A patch for Drupal 5.

skyl1ne’s picture

skyl1ne CreditAttribution: skyl1ne commented
Title: drupal_to_js converts empty arrays to objects » A patch for Drupal 5
Version: 6.x-dev » 5.1

RE: A patch for Drupal 5

Do I just paste that at the end of the common.inc file?

Thank you.

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented
Title: A patch for Drupal 5 » drupal_to_js converts empty arrays to objects

No. The patch shows which lines to change in the drupal_to_js function. Instructions for applying patches automatically are at http://drupal.org/node/60108.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

I've committed this change to CVS HEAD and DRUPAL-5. Thanks for the careful explanation, I got it now. Rock.

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented
Version: 5.1 » 4.7.x-dev
Status: Fixed » Patch (to be ported)
FileSize
drupal_to_js-array-DRUPAL-4.7.patch1.2 KB

Patch for Drupal 4.7.

skyl1ne’s picture

skyl1ne CreditAttribution: skyl1ne commented
Title: drupal_to_js converts empty arrays to objects » Drupal 5.1 patch

I was able to install the path for Drupal 5.1 without any problem, but am still unable to submit text or have it show up in the chat window. Any help would be appreciated.

Thank you.

Crell’s picture

Crell CreditAttribution: Crell commented
Title: Drupal 5.1 patch » drupal_to_js converts empty arrays to objects

Please do not change the title. Thanks.

kkaefer’s picture

kkaefer CreditAttribution: kkaefer commented

(This is a duplicate of http://drupal.org/node/78766, closing the other one since this issue has patches committed)

lachralle’s picture

lachralle CreditAttribution: lachralle commented

Please add me to the list of ppl who have applied the change to common.inc in drupal 5.1 but still can't send chat messages in both MacOpera and Camino.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

Please move comments about a bug in chat module to an issue on that module.

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented

The correct issue for Ralf's Chat room bug is 126181.

kkaefer’s picture

kkaefer CreditAttribution: kkaefer commented

For a way to force non-numeric arrays to convert to arrays instead of objects (which will result in a renumbering of course), see http://drupal.org/node/126548.

sean2000’s picture

sean2000 CreditAttribution: sean2000 commented

I have a similar problem as skyl1ne. I applied the patch (I'm using drupal 5.1), but it still isn't helping. When I type a message and push "Send", nothing happens and the messages aren't being written to the tables either.

Please contact me if you have a solution. Thanks!

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented

Please stick to the subject. There is already a bug report open for Chat room.

killes@www.drop.org’s picture

killes@www.drop.org CreditAttribution: killes@www.drop.org commented
Status: Patch (to be ported) » Fixed

applied

gurukripa’s picture

gurukripa CreditAttribution: gurukripa commented
Category: bug » support
Status: Fixed » Postponed (maintainer needs more info)

hi guys,
i want to use the chat module and i am not a wizkid like most of u..
i noticed that there is a patch..and i am using 5.1
can someone kindly certify whether the module is ready for use...or needs the patch...
my request is that someone kindly fix the module and make a release so we wont have to make a patch, since newbies like me feel nervous with this kinda thing.
look frd to someone helping me out here...tell me what to do.

thanks...

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
Status: Postponed (maintainer needs more info) » Fixed

Third time now: if you have an issue with chatroom module, look for an issue there, or open a new one:

http://drupal.org/project/issues/chatroom

Please don't reopen this issue again.

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)