I have an issue on fieldgroup that occurs on the fields-overview screen. When dragging the fields there, onDrop gives an js-error on the rowHandler.
rowHandlers.field is not a constructor
[Break on this error] var rowHandler = eval('new rowHa... + data.rowHandler + '(row, data);');

Now there are a couple of ways to look at this.
1/ A very easy fix:

Drupal.fieldUIOverview = {
     // ... So here check if the function exists. 
      if (typeof(eval('rowHandlers.' + data.rowHandler)) == 'function') {
        // Create the row handler, make it accessible from the DOM row element.
        var rowHandler = eval('new rowHandlers.' + data.rowHandler + '(row, data);');
        $(row).data('fieldUIRowHandler', rowHandler);       
      }   

Eval will not give the actual error that occured, so by checking the type of the var will tell us if it is a function. That's the only way i know in js to really have the expected result.

2/ Field_group's extra attach goes like this on the fields overview:

Drupal.behaviors.fieldUIFieldsOverview = {
  attach: function (context, settings) {
    $('table#field-overview', context).once('field-field-overview', function() {
      Drupal.fieldUIOverview.attach(this, settings.fieldUIRowsData, Drupal.fieldUIFieldOverview);
    });
  }
};

So you could say, ok to alter the onChange or onDrop, or maybe not even hooking the format trigger for fields in that form, i could create my own object as well.
Drupal.fieldUIOverview.attach would be Drupal.fieldGroupUIOverview.attach or something.

What do you think? (i provided a patch for the js check if you find option 1 the best for all other contribs that want to do exotic stuff.)

Comments

yched’s picture

You need to upload files with the *.patch extension if you want the test bot and http://drupal.org/project/dreditor to recognize them.

+      if (typeof(eval('rowHandlers.' + data.rowHandler)) == 'function') {        

Couldn't we just do

if (data.rowHandler !== undefined) {        

instead ?

Also, beware of trailing whitescpaces.

Stalski’s picture

The problem is still there as "extra_field" and "field" are defined, they just can not be called since the methods are not needed. Unless you implement extra_fields and fields with empty methods.

The real question is to know if we can call it (evaluate it), without javascript braking. As soon as i try to use the js api object, then it will target all .draggables, so field and extra_fields as well.

Stalski’s picture

Issue http://drupal.org/node/964062 is dependent on this.

Stalski’s picture

StatusFileSize
new854 bytes

New patch

Stalski’s picture

Update:
- we could change the fact that on manageFields js_settings are not set in php for the fields, so field_group can do it only for its own rows
- maybe a faster approach is possible on the check if the rowHander exists, if it is needed what so ever!

yched’s picture

Status: Active » Needs review
StatusFileSize
new1.95 KB

Cleaner fix, after IRC discussion.

- Drupal.fieldUIOverview.attach needs to run only on rows that have js settings.
- field_ui_table_pre_render() needs to stop adding #js_settings on rows that don't have it.

yched’s picture

StatusFileSize
new1.92 KB

Oops, same patch without the console.log().

yched’s picture

StatusFileSize
new4.38 KB

This requires another check in onDrop : some rows don't have a rowHandler, because we're not watching them. onDrop only needs to act on watched rows.

There's another, different bug when dragging a group row on "Manage fields": onDrop relies on the presence of a hidden "region" row to detect which region we are in (var regionRow = $(row).prevAll('tr.region-message').get(0);)
"Manage fields", because it has only one region, did not add that region row, and the above fails.

If we want rows to be able to have JS behaviors in "Manage fields" (and field group is a use case), that region row must be here.

Attached patch should fix that.

yched’s picture

Title: Check function exists in javascript for rowHandlers » Errors when integrating fieldgroup rows on "Manage fields" screen

better title

Stalski’s picture

It works very nice. Good work.

lsolesen’s picture

When will this be implemented in the released development version?

Stalski’s picture

bumping , field_group really needs this, maybe others too.

Stalski’s picture

Status: Needs review » Fixed
Stalski’s picture

Status: Fixed » Reviewed & tested by the community
cgoffin’s picture

I applied the patch to a Drupal 7.0-beta2 version and it still gives the javascript error.

cgoffin’s picture

It works against de latest drupal 7 dev version.

Stalski’s picture

Yes, needs to be dev

donquixote’s picture

we applied this patch on a site with beta3, and it fixed the problem. thx everybody.

webchick’s picture

Category: feature » bug
Status: Reviewed & tested by the community » Fixed

I don't totally grok everything going on in this patch, but looks like it fixes a pretty nasty bug in contrib.

Committed to HEAD. Thanks!

Stalski’s picture

thx a lot!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.