I may have missed it, but would it be possible to add a button that deletes multiple fields I don't want/need anymore (a red cross button seems good)? Suppose I create a new node of a type where I can an unlimited number of fields, and I add too many, I would want to be able to delete those three empty fields before (re)submitting my node.

CommentFileSizeAuthor
#249 cck-196421-190.patch17.53 KBjghyde
#202 cck-196421-202.patch33.41 KBmarkus_petrux
#198 cck-196421-198.patch31.7 KBmarkus_petrux
#190 cck-196421-190.patch25.36 KBmarkus_petrux
#184 content-edit.js_.196421-184.txt6.15 KBmarkus_petrux
#181 center_remove_button.patch505 bytesAgileware
#178 cck_remove_icon-196421-178.patch23.37 KByched
#174 remove.png944 bytesmarkus_petrux
#173 content-edit.js_.196421-172.txt5.51 KBmarkus_petrux
#167 cck-196421-167.patch22.33 KBmarkus_petrux
#157 Netflix- Queue_1238621197725.png10.5 KBrconstantine
#154 cck-delta-unremove.png12.96 KBcoltrane
#153 cck-196421-153.patch21.5 KBmarkus_petrux
#153 remove.png1.24 KBmarkus_petrux
#148 cck-196421-148.patch21.54 KBmarkus_petrux
#133 cck-196421-133.patch15.35 KBmarkus_petrux
#131 cck-196421-131.patch15.33 KBmarkus_petrux
#127 cck-196421-127.patch16.55 KBmarkus_petrux
#121 cck-196421-test-121.patch3.25 KBcoltrane
#117 cck-196421-117.patch17.92 KBmarkus_petrux
#114 cck-196421-multiple-values-delta-114.patch11.26 KBcoltrane
#84 cck-196421-84.patch17.81 KBmarkus_petrux
#75 cck-196421-75.patch15.17 KBmarkus_petrux
#75 button-remove.png1.03 KBmarkus_petrux
#75 button-throbber.gif2.14 KBmarkus_petrux
#71 cck-196421-71.content-edit.js_.txt2.99 KBmarkus_petrux
#71 cck-196421-71.remove-button-example.png3.21 KBmarkus_petrux
#70 delete.png1.03 KBmarkus_petrux
#69 cck-196421-69.patch9.78 KBmarkus_petrux
#52 cck-196421-52.patch11.06 KBmarkus_petrux
#48 cck-196421-48.patch11.06 KBguidot
#47 cck-196421-47.patch11.06 KBmarkus_petrux
#43 cck-196421-43.patch9.47 KBmarkus_petrux
#40 cck-196421-40.patch9.34 KBmarkus_petrux
#39 cck-196421-39.patch9.32 KBmarkus_petrux
#36 cck-196421-36.patch7.61 KBmarkus_petrux
#33 cck-196421-33.patch4.23 KBmarkus_petrux
#29 multigroup-fix.patch2.23 KBeMPee584
#27 multigroup-fix.patch2.22 KBeMPee584
#13 cck-fix-multigroup-delta.patch711 byteseMPee584
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

KarenS’s picture

As of right now blank fields get deleted automatically when you save the node.

We had discussed adding a way to specifically delete a field, and maybe someone will want to create a patch to do this. We have also discussed whether or not we should be deleting blank values automatically, and I guess if we had a way to select the ones you want to delete we wouldn't do that any more.

I'll leave this out here as a feature request, but as far as I know, no one is working on it now.

yched’s picture

Title: Deleting unwanted multiple fields? » Deleting unwanted multiple values ?

This is something I wish we had too - In my mind, this kind of goes along with the new 'add more values' and reordering features.
This raises some non trivial UI questions, though. So this is not top priority for now - at least for me.

KarenS’s picture

Version: 6.x-1.x-dev » 6.x-2.x-dev

Bumping and moving to 6.2 where it really belongs. This issue is now a blocker for #119102: Combo field - group different fields into one.

hass’s picture

Subscribe

eMPee584’s picture

quick thought (i'd post code but only 26 hours left to site launch):
a 'protect' checkbox column (tooltip description: Do not remove this field if left empty) that does exactly what it says.

Woodside’s picture

Subscibe

KarenS’s picture

One possibility is to add a 'Delete' checkbox to the right of each field. The fields are already in a drag n drop table, so adding another cell to the table might not be too hard to do. It could be right next to the 'Weight' cell (you only see the Weight cell if javascript is turned off, but it's right after the field in the table row). Then alter the code where we clean out empty values to check for that value instead of automatically deleting all empty fields.

I think offering a 'Delete' checkbox where you can delete either a filled out or empty value is easier to understand than offering a 'Protect' checkbox, since with the protect checkbox you have to have some additional text that says you have to empty values to delete them, but empty values that you don't want to delete must be protected. Plus a simple 'Delete' checkbox is a nice simple UI and closer to the standard way of doing things like this.

But there are really two issues here -- the last little bit of this is to decide if deleting field should also shift up the ones behind it, leaving no empty spaces. Shifting the values to eliminate empty ones works OK for regular fields, but screws up things like multigroup where the empty value must be retained as a placeholder and positions should not be changed.

Since we now have drag n drop, we could assume people will do their own re-arranging and they can leave empty values in the middle of filled out values. But what if they really do want to get rid of them completely? We need a way to tell the difference between 'completely remove this item from the database' and 'leave an empty placeholder in this spot in the database'.

yched’s picture

One advantage of our current behavior (empty values are pruned except for the first one) is that this allows for a reliable 'IS EMPTY' Views filter.
If we allow empty records between non-empty values, a node having [some value], [empty value], [some value] will be selected by the 'IS EMPTY' filter, while it does have values.

Also, allowing values to be separated by empty values changes the philosophy of 'multiple values'.
Currently, multiple valued fields are only a sequence of values, where order matters but not position : the 2nd 'position' has no special meaning compared to the 4rth. A fair amount of the current code and concepts relies on this limitation, and I'm afraid opening the feature of 'position matters' brings it's own can of worms.
This is why I went on a 'prune empty values in core fields' campaign almost 2 years ago, [edit: and why the D6 refactoring that allows us to enforce this consistently for all field types is a great thing] - position should not matter.

This being said, I understand why this is problematic with multigroups, of course...

KarenS’s picture

I can understand the problems with Views empty/not empty filters if we store it like this:

delta | value
==========
0         First thing
1         
2         Next thing

But now we store it like this:

delta | value
==========
0         First thing
1         Next thing

And instead we could store it like this (which would work right in multigroup):

delta | value
==========
0         First thing
2         Next thing
markus_petrux’s picture

Is there anything that can be done to help here?

Is it that CCK would need to be able to work with non-consecutive deltas?

eMPee584’s picture

The problem also occurs in multigroups where you leave a field empty in one or more group instances and fill it in in the next one. On saving, the value moves up to the first multigroup because the empty values seem to be pruned.. mmh.

markus_petrux’s picture

Sure. I found this issue because I was trying to figure out what's preventing the "content_multigroup" module be.

eMPee584’s picture

Title: Deleting unwanted multiple values ? » Deleting unwanted multiple values / multiple values delta issues
Status: Active » Needs review
FileSize
711 bytes

This module must be one of the most complex beings in this universe. Some of the inner workings very closely resemble black magic. It is also extremely mind-boggling to debug.
Anyways, the attached patch fixes the behaviour that field values in multigroups move from the intended position to the first empty one. It is my believe this also fixes the issues with the file upload fields, please someone test that aswell. http://hfopi.org/wanted works as intended now.

KarenS’s picture

Looks like a simple patch and looks like it would work.

@yched, do you see any reason why this change would be a problem?

markus_petrux’s picture

When $max is computed in content_multiple_value_form(), it may give a value for a $delta that's already taken?

KarenS’s picture

Right, good catch. Any place we compute a maxium delta we need to do it using max(array_keys()) instead of count(). Are there any other places that happens?

markus_petrux’s picture

It's hard to tell... we have to look for places where $delta is computed.

I believe I've found another one in content_add_more_js()

$delta = max(array_keys($_POST[$field_name])) + 1;
KarenS’s picture

I did a search for places we use count() and the only ones that affect this issue are the two you point out in #15 and #17. I'm stilling trying to think of any place else this might be an issue, but haven't come up with anything.

markus_petrux’s picture

includes/content.admin.inc also has several snippets that might need revision. When 'delta' is evaluated based on 'multiple' in content_alter_db_analyze(), and maybe somewhere else.

yched’s picture

Well, allowing non-consecutive deltas does make me nervous, because the assumption of consecutive deltas is spread in various places, and because of the underlying conceptual shift (delta values are meaningless vs delta values are meaningful).
Places we might forget in the code are probably no big deal, and will eventually be caught at some point.
Consequences of the conceptual shift I don't clearly map...

Then again, I see no other way forward for the combo field issue anyway.

Can someone check the above changes do not break the tests we currently have ? (requires simpletest module, 2.x branch [edit: also requires schema.module])

KarenS’s picture

@yched - I think it's a good change, regardless of the future of multigroup, because it allows other modules to care about exactly what the delta values are to assign meaning to them. It just feels to me like there is no reason we should care if deltas are consecutive and that it's unnecessarily limiting to make that assumption. Especially now that we have drag n drop to re-arrange items it seems like it should have more meaning.

I agree we have to be careful not to break things because that isn't what we've assumed so far, but I am starting to think it will be safe to do this if we are careful to find the places where we have made assumptions about delta values.

But that makes me think of something else that needs to be checked -- someone needs to test a 'normal' unlimited multiple value field with empty values in the middle to make sure it behaves logically. It should display the empty values between filled values, allow you to empty out values in the middle without saving the empty items in the database, allow you to add new values that are properly appended to the end of the list, store the right values when you save, and present the right information in the next edit.

markus_petrux’s picture

I could try to work out the patch, but I first prefer to ask if you see that's ok, and also I would need someone else to work on the tests.

yched’s picture

Fine, OK by me :-)

One thing that bugs me is that there's currently no visual difference on node view between 0 => foo, 1 => bar (consecutive deltas) and 0 => foo, 2 => bar (delta 1 was left empty). It's only when you edit the node, or view it's 'devel load' tab, that you realize it has empty values. This could lead to hard-to-figure-out 'my View doesn't work as expected' bug reports (Views relationships and sorts have a 'use delta n' option).
I'm thinking we might want to visually display those missing values. For instance, the second case above would show :
Field foo :
foo
-
bar
That way, users can *see* the empty value they left between two values, which in most non-multigroup cases is not what they intended.
Can probably be discussed, this doesn't need to block the patch.

@markus_petrux: Thks for the offer ! The test Karen mentions can be appended at the end of cck/tests/content.crud.test's testFieldContentUI(), and largely adapted from what's already in there. It would be great if you could put it in place, and leave out the parts that beat you, we would take it from there.

markus_petrux’s picture

I agree the 'visual' hint matters here. I'll try to see what can I do. :)

eMPee584’s picture

Well maybe preserving the deltas should only happen within multigroups then?

yched’s picture

eMPee584: I considered that too - but a field can be moved in and out of a multigroup (it can, can't it ?) so we could still potentially end up with 'regular' fields having non-consecutive deltas. Remapping deltas when a field is moved out of a multigroup would then mix data completely if you put the field back in :-) So I think we do not want to go there...

eMPee584’s picture

FileSize
2.22 KB

There's another check that needs to be changed, see attached patch (yes i am trying to sneak the variable name changes in with it too ;)

yched’s picture

eMPee584: I'm not familiar enough with the code in content_multigroup to have a personal opinion about variable names, but at any rate let's stick with '_' as a word separator :
$groupname -> $group_name
$subgroupname -> $subgroup_name

eMPee584’s picture

FileSize
2.23 KB

true.

markus_petrux’s picture

Before looking at multigroup, I think it is necessary to complete the inners of CCK to be able to deal with non-consecutive deltas.

As being said in #24 I'm looking at this, OMG, this change is really complex. So I'll be posting my findings as I go... and I'll post a patch when I feel it could be stable.

Here's one thing I would like to comment.

In content_storage(), for 'load' operation. We need to build the node struct preserving deltas as they are in the database. So we need to change this:

$additions[$field_name][] = $item;

into:

// Preserve deltas when loading items from database.
if (isset($row['delta'])) {
  $additions[$field_name][$row['delta']] = $item;
}
else {
  $additions[$field_name][] = $item;
}

Please, correct me if I'm wrong. It will help me advance.

yched’s picture

Markus : Right

markus_petrux’s picture

Ok, thanks. Here's another doubt, which is related to how items are deleted and the Views IS EMPTY/NOT EMPTY stuff discussed in #8 and #9.

a) Which should be the role of content_set_empty() ? This one shifts items that are considered empty, but now it should not shift any item, but simply mark field columns with NULL values. Using code, it would look something like the following:

function content_set_empty($field, $items) {
  // Prepare an empty item.
  $empty = array();
  foreach (array_keys($field['columns']) as $column) {
    $empty[$column] = NULL;
  }

  // Keep track of empty values.
  $filtered = array();
  $function = $field['module'] .'_content_is_empty';
  foreach ((array) $items as $delta => $item) {
    $filtered[$delta] = $function($item, $field) ? $empty : $item;
  }

  // Make sure we store the right number of 'empty' values.
  $pad = $field['multiple'] > 1 ? $field['multiple'] : 1;
  $count = count($filtered);
  if ($count < $pad) {
    $delta = max(array_keys($filtered)) + 1;
    for ($i = $count; $i < $pad; $i++) {
      $filtered[$delta++] = $empty;
    }
  }

  return $filtered;
}

Is this the right way to do it?

b) Another related question: to remove items... we need a checkbox for each delta in the form as suggested by KarenS in #7, and that's how items will be really removed from the database, which seems to be correct behavior for multiple valued fields, but also it is what the multiple group module would need as well. Correct? Wrong?

There's always more than one way to do things, some of them are even right, but it is not easy to see for me because I could not take something important into account, so I prefer to keep asking. I hope you don't mind.

markus_petrux’s picture

FileSize
4.23 KB

Well, here's a patch that I believe it's getting closer, but it still needs more love.

I'm trying to keep track of deltas. It doesn't have a checkbox to delete items yet.

I would appreciate a review, just to check I'm on the right track. :-)

markus_petrux’s picture

Status: Needs review » Needs work

Of course, "code needs work". For example...

In addition to the above patch, I had to change the following in content_field().

From this:

// Fill-in items.
foreach ($items as $delta => $item) {
  $element['items'][$delta] = array(
    '#item' => $item,
    '#weight' => $delta,
  );
}

To this:

// Fill-in items.
foreach (array_keys($items) as $weight => $delta) {
  $element['items'][$delta] = array(
    '#item' => $items[$delta],
    '#weight' => $weight,
  );
}

The "add more" button does not work correctly yet.

Also, at 'presave' state it is needed to rebuild the deltas based on the weight of the fields, but here the multigroup module, which is broken now, needs to be involved somehow probably. Because we should only remove a delta if all fields in the multigroup are empty.

Please, correct me if I'm wrong. Meanwhile, I'll try to keep going...

KarenS’s picture

Thanks for all this work. It looks like you're doing a good job of uncovering the relevant issues. We need a solution that doesn't make any assumptions about whether multigroup is available, so it would either require a hook to let multigroup intervene or check that multigroup will be able to do something in its own presave.

markus_petrux’s picture

Status: Needs work » Needs review
FileSize
7.61 KB

Ok, here's a patch that seems to work on my development environment. :-D

  • Empty items in multiple value fields don't shift. Deltas remain.
  • The "Add more items" button works, with and without javascript enabled.
  • A new column labeled "Delete" is added to the right of the table, and the rows render a simple checkbox, which is how items should be removed now.

The multigroup module needs to be reworked, but this is something that could be managed from its own issue: #119102: Combo field - group different fields into one

Aside, another thing that could be done here... when javascript is enabled, the checkbox to delete items could be turned into a X icon. When clicked, the checkbox could be checked, and the table row hidden. This could be implemented as a small Drupal.behavior on a separate .js file. Sounds good? Any suggestion for the icon?

markus_petrux’s picture

@KarenS/yched:

Aside, another thing that could be done here... when javascript is enabled, the checkbox to delete items could be turned into a X icon. When clicked, the checkbox could be checked, and the table row hidden. This could be implemented as a small Drupal.behavior on a separate .js file. Sounds good? Any suggestion for the icon?

For the moment I'm not going to implement this "UI enhancement". The delete checkbox itself seems to be intuitive enough. At least for the people that is in need for this feature on the company I'm working. Also, there are a lot of possible ways to do it, so if something needs to be done, I would do it as you people wish, which would be good for us as well.

So this is just waiting for review. Please, critique.

Now, I'll try to work on the multiple group module.

It would be nice to see CCK 2.2 with this feature when ready.

yched’s picture

Markus : thx for this. I'm currently on the road and cannot easily review myself.

I'm wondering how the 'remove' checkboxes play with imagecache / imagefield : those currently come with their own checkboxes already...

markus_petrux’s picture

FileSize
9.32 KB

I'm wondering how the 'remove' checkboxes play with imagecache / imagefield : those currently come with their own checkboxes already...

I've looked at filefield and yes, it provides a button to "Remove" files, which makes the checkbox I added here redundant. Also, the checkbox I added here knows nothing about the work that needs to be done to cleanup the uploaded file, which looks like this is problematic.

The filefield module also makes assumptions on the way deltas are computed, which is bad news because we are trying here to ensure CCK can work with non-consecutive deltas, which seems to be required to provide a consistent behavior for the multigroup module.

Probably, a) CCK would have to provide an method to allow CCK related modules compute deltas and b) all modules that need to compute deltas need to be adapted to such method.

Anyway, here's another patch that's equal to the one in #36, plus a few more changes. It fixes a bug in function content_max_delta, where "{$table}" in SQL statements were removing the {} around the table name due to PHP parser.

I'm not sure now which is next step. :(

markus_petrux’s picture

FileSize
9.34 KB

Here's a new patch. It simplifies a bit the content_set_empty() related stuff.

I've been working on the multigroup module, and I believe I'll get it working soon. I'll post what I have now in the other issue: #119102: Combo field - group different fields into one

What would be missing here is decide a method to allow modules such as the filefield/imagefield deal with its own implementation of the delete delta item stuff. Maybe they will have to alter the form to hide the delete checkbox provided here and set its value when the user clicks on the filefield "Remove" button?

yched’s picture

Do we really need to introduce a 'remove' checkbox to begin with ?
The main focus right now is to fix the "empty values cleanup messes multigroups" issue. The answer for that is "don't garbage collect deltas, and store non-consecutive deltas when there are empty values between actual values". Unless I'm missing something, I don't think this *requires* that we add a 'remove' checkbox ?

markus_petrux’s picture

It looks like it is more intuitive to let the user choose which delta item should be removed. It is not clear at first that you have to set the field empty on the form to tell CCK that you really want to remove it from the database. Think about a spreadsheet, for example.

If we don't add a 'remove' checkbox, how does the content module know when a delta item needs to be removed from the database? How can the content module know a delta item is part of a multigroup, which is when it would keep it even if empty, unless all deltas for the group are empty?

markus_petrux’s picture

FileSize
9.47 KB

Well, here's a patch that works without adding any 'remove' checkbox.

Instead, there's a hidden element '_keep_empty' that is set to 0 by default, so the content module will behave as always. No change is necessary in any other module, and users will still have to empty values in order to tell the content module that they want to delete a field.

The multigroup module will set that hidden element '_keep_empty' to 1 by default, and it will clear it when the form element for the group is validated, which where the fields are moved back to their original position. But it will clear this flag only when ALL fields in a delta group are empty.

This patch also fixes the bug in function content_max_delta(), where "{$table}" in SQL statements was wrong and PHP parser was removing the {} around the table name, which is a problem when $db_prefix is used.

I'll also post a new version of the multigroup module here: #119102: Combo field - group different fields into one

eMPee584’s picture

Ok first Markus, thumbs up. It almost works here, too, and it includes fix for #329328: Multigroup / Display Fields: Subgroup has not all options displayed correctly.
So on my setup (http://hfopi.org/wanted) that has some plain and simple textfields, the only problem i could find was that after upgrading to the latest and patched CCK, when on existing nodes with multigroups, values within those propagated upwards into the empty deltas (hope you understand what i mean), my guess is that's because the keep_empty element isn't set properly on loading field instances. After correcting and saving the node it seems to work. Oh and the patch from #173824: CSS: hidden inline field is a bit unexpected, add option for visible inline is also needed for inline labels to show up properly on each multigroup.
Another thing i didn't notice before: dragging a group after changing the field contents in it resets the changes! Don't know how to fix that one though..

markus_petrux’s picture

@eMPee584: Maybe you started with data altered with another version of the patch related to this issue? Maybe that caused storing deltas in a different way as it would be in normal situation that is causing now unexpected results? When saving data with the patch applied, you say it seems to get fixed, but then it happens again?, or it gets fixed permanently since then?

In regards to #173824, I attached a one liner patch there that I believes it solves the issue with the multigroup.

Another thing i didn't notice before: dragging a group after changing the field contents in it resets the changes! Don't know how to fix that one though..

OMG! This is completely unexpected as it worked for me here. Could you please describe when the changes were lost? During the drag-n-drop operation? using preview? after sending the form? These operations worked for me here, but I could have missed something.

Thanks for reviewing! :-D

markus_petrux’s picture

Hi all,

Just wanted to mention that I'll try to post a new patch here. There's a need to cover use case that will now, the latest patch as-is, shift delta values when it shouldn't. The case is when you node_load and node_save. Delta values will be shifted for empty items because the keep_empty flag is not set on node load, it is only set when building the edit form.

On the other hand, we've been testing the patch here with all core fields, including optionwidgets, and a few other fields such as addresses (which is complex with several columns) and it all worked, but we have an issue with date which I'm not yet sure if it's a date issue or not.

To get more feedback for this patches, maybe we could post a note at http://groups.drupal.org/reviewers ?

markus_petrux’s picture

FileSize
11.06 KB

Well, here's a patch that fixes the node_load/node_save issue I mentioned in #46, and it also covers a few changes required to help the multigroup module work. I'll post an updated version in a moment, please see #119102: Combo field - group different fields into one.

Part of the problems with the date field are fixed here, but require a patch I posted in the date issues queue: #324290: warning: array_key_exists() [function.array-key-exists]

guidot’s picture

FileSize
11.06 KB

@markus_petrux: Looks like there's a typo in line 66:

+ * @param bollean $keep_empty_items

You meant boolean, don't you? Attached a fixed patch.

haggins’s picture

Could someone explain how to install this patch? Is it sufficient to copy cck-196421-48.patch to the cck's module-path and run update.php?

thx in advance!

markus_petrux’s picture

@#49:

http://drupal.org/patch
http://drupal.org/patch/apply

@guidot: Thanks. I fixed the typo on my working copy as well. Just in case another patch needs to be rolled up.

markus_petrux’s picture

Is there anything else that can be done for this issue to help getting it in?

markus_petrux’s picture

FileSize
11.06 KB

Patch re-rolled due to changes in line offsets. No other change made.

KarenS’s picture

I'm working through this now. A couple preliminary comments:

1) A number of the changes in the patch are not changes at all, you just changed one way of doing something to another way of doing the same thing, like the following, which actually changes nothing at all. This makes it harder to see what significant changes are in the patch. For future reference, please try to avoid making those kinds of changes, especially in complicated patches :) I may go back and take those things out of the patch to make it easier to evaluate.

@@ -2437,14 +2475,13 @@ function content_max_delta($field_name, 
   // in the node table to limit the type.
   else {
     $db_info = content_database_info($field);
-    $table = $db_info['table'];
     if (!empty($type_name)) {
-      $delta = db_result(db_query("SELECT MAX(delta) FROM {$table} f LEFT JOIN {node} n ON f.vid = n.vid WHERE n.type = '%s'", $type_name));
+      $delta = db_result(db_query('SELECT MAX(delta) FROM {'. $db_info['table'] ."} f LEFT JOIN {node} n ON f.vid = n.vid WHERE n.type = '%s'", $type_name));
     }
     else {
-      $delta = db_result(db_query("SELECT MAX(delta) FROM {$table}"));
+      $delta = db_result(db_query('SELECT MAX(delta) FROM {'. $db_info['table'] .'}'));
     }

2) I'm not sure I agree about getting rid of the 'remove' checkbox. That's a more intuitive way of doing things that asking people to empty all values out. I'm wondering about re-introducing that. If it breaks filefield, we need to fix filefield. It seems to me that this is the behavior we should have as we move into core -- an explicit way to remove values rather than asking them to empty values out. I'll play around with this and see, maybe it can be 'step 2' in a multi-step process to keep things clean.

markus_petrux’s picture

1) The change in content_max_delta() is relevant and necessary. This function seems to be made to support the multigroup module, but it is buggy. Existing code does not work when $db_prefix is used due to PHP parser removing braces in "{$table}".

2) The 'remove' button thing was discussed earlier in comments #38 to #42, and in comment #43 was where I started to not using 'remove' button for multiple values/multigroup. Please, note that fields like filefield/imagefield already provide their own 'remove' button, so what we could do here would have to be in sync with those fields if we don't want to break them. Aside, a 'remove' button is something that could be implemented by an add-on module, which is what we, here at the office, planned to do at some point, once the multigroup and this issue is committed to CVS.

KarenS’s picture

The db_prefix stuff is a separate patch that has nothing to do with this issue and we should separate that out. If that is broken, we'll fix it, but it's confusing to throw that in here. I'll go ahead and commit the db_prefix fixes as a separate patch right now to keep that code out of this patch.

I realize that filefield has its own remove button. My point is that this is something that CCK *should* be doing, we shouldn't avoid doing it because filefield is already doing it in a different way, we should get things fixed correctly. I don't think this should be a separate module, it should be The Way Things Work. The problem with committing a different change and then fixing it later is we make it harder to track what we're doing and why.

Filefield is only alpha code now, there should be no reason why we can't get it fixed. If nothing else, we can let filefield do its own thing and only provide the delete button otherwise, in the same way we let modules decide whether or not they will handle their own multiple values or default values.

I know that it was discussed already and yched was in favor of deferring that change, I'm just saying I disagree and think we should get it working right now instead of doing some intermediate fix and then changing things again later.

markus_petrux’s picture

I couldn't agree more in your point of view, and I'm really gratefull that you can take the time for this issue. :)

In regards to the db_prefix stuff... I know it could have been fixed from a separate issue, but it's been easier for me this way, and probably for everyone else that has been testing while I've been posting updated versions of the multigroup module.

Again, thanks for getting back to this, and I hope that something I did here helps in one way or another.

Cheers

KarenS’s picture

Right, I know it's been a mess to drag around all these patches while we've been unable to work on this, so I understand that. I just committed the db_prefix fix, so that's already done now.

I'm examining these changes and moving them as appropriate into the D7 fields in core code, too, so that we can make sure meaningful delta values are baked into that code from the beginning (or at least try to keep this conceptually possible as that code rolls forward). So I'm actually hand-coding these changes so I can do that.

And your work has been extremely useful, so thanks!

yched’s picture

about 'delete' button : fine by me, if you guys feel this is the way to go. I'm quite frankly completely beyond on this thread and on the multigroup one right now. We just need to make sure that filefield gets a chance to to what it currently does with its own button, whether the field is in single, multiple, in a multigroup...

More generally, my main concern with this patch is that this (non-consecutive deltas as a requirement for combo fields) is *not* the direction that is being taken for 'Fields in code D7'. We don't want to be cornering users with fields and field data they can't upgrade to D7.

We're sort of in a 'features get implemented in the dev branch' situation, and the dev branch currently is D7 (or "also D7") - and, even better/worse, it will soon be *core* D7 :-/

KarenS’s picture

Actually I think the changes in here are changes that should go into dev. I don't think we should arbitrarily be changing the the delta and forcing people to manually empty field contents to delete a value. The whole business of handling value removal in this haphazard way is really messy.

I would propose that in both D6 and D7 we create a method to specifically identify values to be deleted instead of just blindly removing all empty values. It's exactly because I think this belongs in D7 that I'm trying to push this forward. Multigroup and whether it belongs in D7 is an entirely different issue and I'm not as sure about that, and for that matter it can remain a contrib module forever.

But we need a standard way of handling the removal of multiple values. We never addressed handling adding multiple values in CCK until D6 and ended up with contrib modules all creating their own, different, ways to work around it that made it really hard to get back control. If we don't establish a standard method of identifying the specific items to be removed we will create the same problem with removing multiple values: we will have not only filefield but other fields coming up with their own different solutions to fix what we haven't done in CCK and contrib modules will have no way to do anything to fix it that won't break those modules -- and we'll take that whole mess into D7.

I think it's really important to let users identify which values should be removed, which also means that we must not remove values that they haven't identified, which is part of the reason I think preserving the delta is also important.

markus_petrux’s picture

What's critical for the multigroup module is that deltas for individual fields may end up empty, but they need to be in sync with other fields in the multigroup, so we need to teach cck how to deal with non-consecutive deltas.

In regards to the development and maintenance of the multigroup, I emailed you both a few days ago about it (but I got no answer). If you don't feel it worths including to CCK2, then if you don't mind I would be glad to maintain it as a separate module. We need this for the project I'm working on, and it will benefit us if it's used by other sites. In fact, we already decided to go with CCK2 + the patch in this issue. The reason we need this is that it saves us from developing a bunch of fields with just one or two columns. So it worths for us whatever it happens here. I thought, it was necessary to mention something about our motivation on this. Just that.

KarenS’s picture

More thinking out loud. If a multiple value field has the following values:

nid | value      | remove
====================
0    | 'First'   |
1    | 'Second'  | X
2    |           |
3    | 'Fourth'  |

I think I would expect the 'remove' button to completely delete the value and shift up the ones below, and I would expect the empty value that is not removed to be left in the db. But in this case the deltas *would* change (2 would become 1 and 3 would become 2). So maybe I am wrong about preserving the delta and instead what I want is a way to preserve a space for empty data, which can only be done with a 'remove' button to differentiate between 'a removed value' and 'a saved empty value'.

If the user has a 'remove' button and does not use it, I would interpret that to mean they want the empty value. The deltas would still be consecutive in that case and there will be a value in the database for that empty value.

This feels like a logical way to handle this, but it is not the same as what I was originally saying, nor exactly what multigroup wanted, but we could work around this in multigroup by getting rid of the 'remove' buttons for individual fields so that the only thing you could do to wipe out a value in the middle of other values is to empty it and in the scenario above, that will be enough for multigroup to work without losing relationships between values. Then multigroup could move the remove button to the group level instead of the field level so you could choose to remove the delta 2 value of the group, which would remove *all* the values in that row, which would also preserve relationships between field values.

It's getting late and I'm probably rambling, I'll mull it over some more and maybe I can get it to a point where it makes more sense. Or throw in the towel and decide it's too complicated to try to change.

KarenS’s picture

And markus, I got your email but haven't had time to respond. I appreciate your offer to maintain multigroup as a separate module (and we may take you up on that), but yched is totally right that we need to be sure that this patch is taking us in the right direction for D7 or we will create problems for everyone. So my ramblings are my attempt to find a path through here that will do The Right Thing for CCK going into D7 as well as make multigroup possible.

rconstantine’s picture

Elsewhere I've already chimed in on how I think the multigroup module is very important. It eliminates many instances where a programmer like myself has to develop a one-off module to gain spread-sheet-like ability and instead allows my co-workers, who are more like DBAs or analysts, to create a content type on the fly which includes said spread-sheet-like input for our users.

A logical progression of this in the future would be to allow a computed_value field as one or more elements of this grouping thereby summing a row, for example. That would be really cool.

For my immediate uses, I have several Word docs that we're converting to Drupal content types and which contain grids of information that just don't break up well into separate multiple text fields. In other words, one row of three items where rows can be added looks better and makes more sense to my users than three fields that can each have added rows themselves, especially when there is an inherent association between the columns of a row. And many of these documents have many such grids. I'd rather not develop a custom module for each one and I'm sure I can't be the only one that would realize this kind of benefit in their work.

Anyway, I am very glad to see all of your work and consideration in all of this. I don't know whether this stuff can get into D7 core or not, but it is my opinion that this is the type of feature that make non-programmers wet themselves with delight and choose one tool over another. Keep up the good work.

markus_petrux’s picture

KarenS #61: If I understand you correctly, this is more or less how I first tried to implement this feature here (before #41/42 above).

When a 'remove' button is added, the user may set a value empty or press the 'remove' button, which means two different things. The former keeps the delta with an empty value or NULL, the later removes the delta from the database and causes the values following the one removed to shift deltas.

I believe the latest patches here can easily be changed to turn the '_keep_empty' flag into a 'remove' checkbox. Then, for fields like filefield/imagefield there would be a need to let them capture the event when the user checks the 'remove' checkbox so they can do their own job rather than providing their own 'remove' button.

I would say that the most common comment from people that has been testing this is directly related to the way values are removed from multiple value fields/multigroup, so adding a button for that will make a lot of people happy. It's really more intuitive.

KarenS’s picture

The biggest problem I still see is that filefield can't 'empty' a value, it can only 'remove' it, so I'm not sure how to prop open an empty filefield value in multigroup. I think we'll need to play around with an example that includes filefield to be sure we can either use it as-is or create a relatively simple patch to filefield that will get it working correctly. If it involves big changes to filefield, it will create another bottleneck in getting this all working, so we must avoid that.

Maybe we will need both the 'remove' and the 'keep empty' values behind the scenes to make this work (i.e. maybe multigroup inserts 'keep empty' into each field as hidden values and cck honors that and doesn't throw those values out and then multigroup follows up by deciding which values should actually be removed and which should just be emptied).

I think a 'remove' checkbox makes all kinds of sense if we can figure out the *right* way to implement it. I hope yched is not too upset that I have opened up this can of worms, but I do think it would be better to get handling for removal of values nailed down and handled appropriately. The affect on multigroup is secondary to that goal, I mostly want a good UI for removing values. But if we can do that, it should also make multigroup feasible.

To deal with the issue of what happens when you drag a field out of multigroup after it has empty values, maybe we display a warning message to let the user know there are empty values in that field. Empty values aren't 'wrong' so long as we have a method where the user can decide whether or not they want them there and get rid of them if they like. I wouldn't think people would often create multigroup fields with lots of empty data and then break them back into separate fields later, so hopefully that will not be too much of an issue.

markus, if you see a way to get this working and want to create another patch, I'll be glad to review it. I really don't have time to work on this, but I'm going to try to make some time (hopefully not too much time) to move it forward. If it turns into a time sink I'll have to pull away and work on other things, but hopefully we can come up with a good, user-friendly, D7-friendly solution.

markus_petrux’s picture

I may try it, but it would have to wait until next week.

One thing I'm not sure is if there's anything in the code 'behind the scenes' that you would prefer resolved in some other way. I mean the _keep_empty flag (ie. the implementation of content_set_empty() ). If that's ok, then the 'remove' check might be a simple UI problem that needs to find its own way with minimal impact on things such as filefield, etc. ¿?

Another doubt... with D7-Fields in mind... what might be more critical to decide now, I think, is the way data is stored in database. Do you see any problem here with the current implementation. What we would have to change here in order to be more D7-friendly? ...only, that warning in the manage fields pane, when the user tries to move a field out of a multigroup leaving empty values? Sorry, I'm not sure sure what you really mean with D7-Friendly :(

Dave.Ingram’s picture

I attempted to apply the patch in #52 against the latest dev version on the CCK page and got the following error:

Hunk #3 succeeded at 763 (offset 1 line).
Hunk #4 succeeded at 882 (offset 1 line).
Hunk #5 succeeded at 942 (offset 1 line).
Hunk #6 succeeded at 1023 (offset 1 line).
Hunk #7 succeeded at 2455 (offset 3 lines).
Hunk #8 FAILED at 2466.
Hunk #9 FAILED at 2478.

It gave me a .rej file so I can correct it manually, but did I do something wrong to begin with? I placed the patch in the cck folder and ran "patch -bp0 < cck-196421-52.patch".

Thanks for any help!

Dave

markus_petrux’s picture

#67: line offsets are caused by this patch. And the rejects are because those changes have been already applied on a separate commit. Hope that helps until a new patch here can be re-rolled from latest HEAD.

New patches may come to re-roll the current approach as well as to try out what's been said in #65/66.

markus_petrux’s picture

FileSize
9.78 KB

Patch re-rolled. Changes since #52: a) fix line offsets. b) fix to content_max_delta() already applied in CVS.

Next, I'll try to work on the new approach to 'remove' items from multiple valued fields with a 'remove' button in the UI, or something on that direction. And well, let's see where it may lead us... :)

markus_petrux’s picture

FileSize
1.03 KB

@KarenS: I have created a small image, delete.png, based on views/images/sprites.png that contains the images for normal/hover 'Delete' button, that would be used for the UI to remove items for multiple valued fields, when managed by CCK. I'm attaching the png here for review. Any other suggestion is welcome. :)

Also, a doubt: where do we put this image? Maybe in cck/images? There's no folder to hold images for CCK yet, so better ask before rolling out patches, etc. :)

markus_petrux’s picture

Here's a screenshot to see how this is looking. The example image shows a plain text field with multiple values, and a small remove button to the right of the table row of each delta item. The icon is based on the remove icon that comes with Views.

It is shown in grey color, and red on mouse over. When clicked, the row of the deleted item is hidden using a fadeOut effect. There's also a hidden checkbox that will be checked when the button is clicked. When javascript is not enabled, only the checkbox is displayed, so it all degrades gracefully.

I'm also attaching the javascript file, content-edit.js that deals with these buttons. Here I tried to implement a method that could be extended by external modules, CCK fields by simply attaching an onRemove handler to remove buttons. The code contains documentation on how to use it.

I think this could be used by fields such as filefield/imagefield to attach their own processing.

This javascript part is working on my dev environment, and now I need to work on the server side to change the way multiple value items are deleted, etc.

I might be missing something, so please let me know how this is looking.

KarenS’s picture

I think this is looking very nice. It would be good to try to create a patch for filefield that would use the new code, as a proof of concept that filefield works correctly with these changes and as an example of how other modules can use this feature (plus that way others can do some testing of that part as well).

The logical place for the image would be to add a new 'images' folder (who knows, maybe there will be others later).

rconstantine’s picture

I think I've finished the patch for nested fieldgroups, so starting Wednesday and barring any comments of bugs from the CCK maintainers, I'll be looking at this patch here and making sure they both play nice together.

@Markus, as of today, do I just need the stuff posted from #69 and later? Or can you roll another patch at day-end today? I think you mentioned a concern about allowing/restricting certain fields from being placed inside fieldgroups, right? Could you expound on that and also list any other things you think I might have to address? Thanks. I think for that one concern, I can whip up an override for the isvalidswap javascript function.

[oops, this should've been in this thread: http://drupal.org/node/119102]

markus_petrux’s picture

@rconstantine #73: Probably not. We're trying to add a 'remove' button to multiple value fields, and that will change a few things. Also, we'll try to address possible changes in filefield/imagefield, so they can integrate with the new approach as easily as possible.

In regards to integration between multigroup and nested fieldgroups, I don't think it will be possible for a multigroup to contain other fieldgroups because the complexity of the code of the multigroup that deals with FAPI movement of fields from one layout to another at pre-render time and then back to original positions before validation/submit handlers are executed, and also because the multigroup code is fired by fieldgroup hooks.

markus_petrux’s picture

Hi all,

Here's the first patch using the new approach. That's adding a 'remove' button for multiple value fields set to unlimited, when the mutil values features is managed by CCK (not by the widgets themselves, like checkboxes, freetagging, etc.)

In addition to the patch, the images folder needs to be created in the cck directory, and then move a couple of small images that I'm also attaching here.

I'm not sure if it all works as it should, specially with the multigroup module in mind. I need to work a bit more on the function content_set_empty(), which is the one that decides if an item is taken or no, based on if it's empty, or flagged for removal.

The patch can be tested with widgets that do not manage their own multiple values. For exemple, try it with a text field with multiple values set to unlimited.

Please, let me know how this is looking.

rconstantine’s picture

#74, I agree that fieldgroups should not nest inside of multigroups and that is one thing I would work on - making sure the user can't do that. However, it seems to me that multigroups should be able to be nested inside other fieldgroups and is how I would use it. I think I mentioned before the cck_fieldgroup_tabs module and that's one reason multigroups should be able to go inside of fieldgroups.

So for now, I guess I'll keep checking on these threads to see when you arrive at a place where it looks like I can begin looking at the points I mentioned. If it occurs to you at that time, I'd appreciate a note similar to "@rconstantine - try your integration now" or something. Best of luck with your current changes. Things are looking good.

markus_petrux’s picture

Sure, I'll let you know when I feel the multigroup is ready posting a note to #300084: Nested fieldgroup

Since the patch above, in comment #75, seems to be looking good, I already started to work on the multigroup module. I'll have to post a new patch here that works with multigroup, which is now broken, though.

I also think it should be possible to place a multigroup inside a fieldgroup, at any level. But a multigroup should not be able to contain fieldgroups. Maybe you can already add a check somewhere in your patch to ensure that? Note that a multigroup is also a fieldgroup, but a different type. Normal fieldgroups are of type 'standard', and multigroups are of type 'multigroup'. You can get this attribute from $group['group_type'].

Sadly, I don't have time right now to check your patch in the Nested fieldgroup issue, but be sure that I'm following it, so I can chime in when it reaches a point where integration with multigroup can be tested.

rconstantine’s picture

@Markus - great, thanks. Regarding the disallowing of fieldgroups inside multigroups, I think it will have to take place in the multigroup module. I think I mentioned elsewhere that it's probably a javascript override, and I'll be happy to write it once you think your patch is to that point. So I'll keep looking at both threads in anticipation.

drewish’s picture

subscribing.

tombigel’s picture

subscribing.

markus_petrux’s picture

@rconstantine - Sweet. I think I could load a javascript file from the multigroup module for the 'Manage fields' screen. Such file could be named content_multigroup.manage_fields.js and contain the override for tableDrag's isValidDrag method that you said, or whatever else it may need to implement for that page. It would be great if you could provide the contents of this file while I'm finishing the rest of changes here.

Aside, I believe the multigroup module may have other pieces of code that will need some kind of ammendment to work with nested fieldgroups. I would be checking that when the main functionality is working again.

flk’s picture

hmm I am currently playing around with the module and was wondering whats the best way of creating new deltas.

I want to be able to create a field outside of the group which can populate the multigroup (seems to work fine when doing one field .. populate the $_POST['group_name'] but if the deltas are more than one it seems to only accept the last one)

markus_petrux’s picture

@Shakur #82: Try with $form_state['item_count']['group_name'] = $count;

Example:

$count = count($mydata);
$form_state['item_count']['group_name'] = $count;
for ($i = 0; $i < $count; $i++) {
  $form_state['values']['group_name'][$i]['field_a']['value'] = $mydata[$i]['field_a'];
  $form_state['values']['group_name'][$i]['field_b']['value'] = $mydata[$i]['field_b'];
}
...
...
drupal_execute($type .'_node_form', $form_state, $node);

Edited: to add and then extend the example.

markus_petrux’s picture

FileSize
17.81 KB

Hi all,

The past week I could not spend much time on this, and I could just work at short intervals which makes things a bit more difficult because sometimes there are things that need enough hours at a time to really advance...

Well, here's a new patch that implements the 'remove' buttons, and this one works with a new version of the multigroup module that I'll post in a minute, here: #119102: Combo field - group different fields into one

It needs a lot of testing as I have not had time to test with different kind of widgets. Any help here is much appreciated, and I hope there won't be much bugs/things to fix... but you never know so...

Here's a patch that should be appied to current CVS code from the D6 branch of CCK. It should also work with the latest nightly snapshot.

Aside from this patch, you need the 2 small icons that can be found attached to comment #75 above.

@KarenS/yched: I'm not really sure what would have to be done to filefield/imagefield so they can reuse the 'remove' button implemented here. Anyway, I hope the documentation on top of the file content-edit.js helps to see how to bind an onRemove event handler. The multigroup module does that to invoke the onRemove handlers of the widgets it contains. But... the 'remove' button in filefield/imagefield is based on ahah, and that provides a non-javascript method to do their job that I don't know how it can be resolved with the 'remove' button implemented here. :(

markus_petrux’s picture

@KarenS/yched: I'm now installing a dev environment to play with filefield/imagefield from where a possible patch in there issues queue could be raised for everyone to evaluate. I guess it won't be possible to digest that unless CCK maintainers approve what's been done here.

Aside, while I tried to play with #300084: Nested fieldgroup... that patch needs more work, IMHO, and for the moment I will leave it as-is... it is not so critical for us right now. I'm wondering if the issue here and the multigroup module could be evaluated before nested fieldgroups, in case everything else here looks good for you, of course.

I've been spending much more time than what we initially planned on these issues here, and I need to focus on other things for the project I'm working on, so I would appreciate any hint on how do you see the future of these coming add-ons to CCK.

rconstantine’s picture

I'd love to see the changes here and in Markus' other thread nailed down and merged with dev ASAP. I think it would help with remaining issues in my patch as well as the other modules Markus mentioned. I realize you're busy with D7 and understand the delay and appreciate your efforts on both fronts.

drewish’s picture

markus_petrux, please grab me on irc if you've got imagefield/filefield patches.

markus_petrux’s picture

@drewish: Thanks for chiming in! :-D

I'll try to open my IRC client as soon as I have something... hmm... I may also open a feature request for filefield/imagefield to discuss collateral issues, different solutions or whatever...

markus_petrux’s picture

For the moment, I tested with filefield and it doesn't completely work with multigroups (the ahah callback is broken, still need to figure out why), but it works here, with any number of multiple values.

filefield with multiple values set to a fixed number is not affected by this patch. The new 'remove' checkbox is only used when multiple values is set to 'unlimited', and here is where we have also the 'remove' button provided by filefield, next to the upload button. The former removes the whole delta from the tableDrag, while the later empties the uploaded widget. IMO, both buttons can co-exist together as they server different purposes. Please, correct me if you feel the UI for the 'remove' checkboxes/buttons needs to be ammended in some way.

So it seems filefield/imagefield would only need some kind of patch to fix the problem with ahah callback when under the context of multigroups.

Aside, I've found that filefield doesn't clean files uploaded while playing with the edit form. I mean, you can re-upload files several times, while only one should remain per widget when the form is submitted. Also found bugs that are already reported, and noticed in the code that there are a few things pending todo, so it's hard to tell when something doesn't work because of the patch here. I would say, it's just the ahah callback issue that needs to be investigated further so it works with multigroups.

@drewish: If you could test this patch and play with filefield, maybe you could see something else that needs to be fixed, as I was not yet familiar with filefield/imagefield.

drewish’s picture

markus_petrux, that's a known issue #292151: Files are not deleted when replaced

markus_petrux’s picture

@drewish, thanks for the pointer. Since we're going to use filefield/imagefield in our project, I believe I'll have to spend some time on filefield issues as well and I hope to help a little.

Have you tested the patch here with multiple valued filefields? Do you think there's anything that needs to be changed in this patch with the new 'remove' checkbox?

As far as I can tell, there may be changes to support the multigroup, but the patch in this issue implementing 'remove' checkbox for multiple valued fields in CCK seems to be fine with filefield/imagefield for me? Could you please confirm? See my previous comment, in #89 above.

rconstantine’s picture

As I mentioned in another thread...

It seems to me that the dev version of CCK should do "whatever it wants" but give downstream modules time to get at least their dev versions up-to-date before releasing an official CCK release. This would mean that Markus' patches could go into dev first, followed by fixes to affected modules. It seems that an approach to fixing issues in filefield/imagefield has been established, so IMHO, that should be good enough for CCK to move forward. This applies to my nested fieldgroup patch as well. Of course, the CCK maintainers may have a different view of doing things and that is their prerogative. I just find it easier to work on downstream patches when I can checkout a complete version of CCK directly from CVS. Otherwise, they all seem to mash together into super-patches.

BTW, this patch seems to work just fine as far as my testing shows with 'regular' fields.

This particular patch stands alone, right? I think it can go in now and isn't very big at all. I'd like to hear what drewish has to say on the matter since I have yet to test with filefield/imagefield. I do need those modules for my current project too.

markus_petrux’s picture

The only issue with this patch I know of is that possible conflict with filefield/imagefield, but I think both 'remove' buttons can co-exist perfectly well because they both serve a different purpose which seems to be clear enough for the user, as far as I can tell from the input I got here in the office, and having got no negative comment from here either. hmm... yes, look. The 'remove' button provided here allows the user to remove an item, it desapears from the UI. The 'remove' button provided by filefield/imagefield can be used to clear the value of the item, which is not the same exact thing. Anyway... I would love to hear suggestions here, so there may be a better way to do it.

That being said. fielfield/imagefield don't work with multigroups. Well, more or less. What fails is the AHAH stuff related to the 'remove' button provided by fielfield/imagefield. I started to look at that, and I feel it can be solved with a patch to filefield, but I'm not yet sure. Unfortunately, I've been very busy this week with other things, so I had to leave this temporarily. I hope to get back to it next week.

PS: I think the problem with filefield in multigroups is related to the value callback, which is a piece of code that's executed before after_build, which is where the multigroup code uses to move fields back to their original positions in the form. Well, I said, I'm not sure... the function that's aparently broken is filefield_js(), but the real cause may be somewhere else...

rconstantine’s picture

I think the solution to the filefield issue will be best solved using the changes I've done in the nested fieldgroup patch. Once I'm sure that I have the multigroup, nested fieldgroup, and this patch all working as expected, I'll look at doing the AHAH stuff for filefield. Because of the nested patch, I was able to completely remove one js function from the cck_fieldgroup_tabs module. Perhaps we'll find similar results with filefield? We'll have to see.

Ram_doss’s picture

Hi markus_petrux ,
I am Ram From India. i attached u r patch file to the cck Module (6.x-2.1) for creating a delete option in multigroup. i did the all the changes as mentioned in patch file.noting comes up as delete option. is their anything else required. can u give u r suggestions?
Also if i click on the Add more Values it shows an error like

* warning: array_keys() [function.array-keys]: The first argument should be an array in C:\wamp\www\theme\sites\all\modules\cck\modules\content_multigroup\content_multigroup.module on line 1070.
* warning: Wrong parameter count for max() in C:\wamp\www\theme\sites\all\modules\cck\modules\content_multigroup\content_multigroup.module on line 1070.

I dont know how to rectify this problem .

Any one who faced the similair problem can help me out.

markus_petrux’s picture

@Ram_doss #95: The patch in this issue needs to be applied to the development version of CCK 6.x-2.x-dev. The error you've seen is related to something that's broken in CCK 2.1.

Ram_doss’s picture

yeah i did it in even in cck(cck-6.x-2.x-dev).in tht too an error throws like

warning: Missing argument 3 for content_permissions_field_access() in C:\wamp\www\theme\sites\all\modules\cck\modules\content_permissions\content_permissions.module on line 21.

and also noting like delete comes up......wats tht mate.....

markus_petrux’s picture

#95: Oh, I'm sorry. My comment in #96 was wrong, I misunderstood the error. Well, my guess is that the patch in #320 of the multigroup module issue was not applied correctly in your system. Please, untar the packed module in that very same comment #320 and look at line 1070 of content_multigroup.module. There's nothing about array_keys() nor max() in there.

Note this is still pure experimental, so please don't use on production as it may get broken at any time, and final implementation may differ of current approach.

#97: It seems to me an error in content_permissions is not really related to this issue.

Ram_doss’s picture

Also that patch u mentioned for #320 of the multigroup module shows only the check box with out any delete image . i too copied images and placed them in (\sites\default\files\images).if we click the add more values button it throws an alert message as
( An error Occured.
/theme/content_multigroup/js_add_more/sam/group_sample) . cant able to add more values.

Can u explain me briefly how to add the delete button in multigroup module ?

Is their any patches for (cck-6.x-2.x for stable version), regarding this multigroup module with delete option. Is It available??

markus_petrux’s picture

#99: These small icons are provided here in this issue. The latest patch is in comment #84, and there you can read "Aside from this patch, you need the 2 small icons that can be found attached to comment #75 above."

Is their any patches for (cck-6.x-2.x for stable version), regarding this multigroup module with delete option. Is It available??

Nope. This issue is a "feature request", it requires patches, and patches need to be made against the latest development version of the corresponding branch. There's a lot of time involved on all this process, and keeping these patches in sync with different releases would require a lot more work, and time, which is something that at least me, don't have. Aside, this would not help the development process because it is against the latest dev code that it should work, since this is what would be released when the time comes, it doesn't tell much if it works or not against a release that is not the latest dev code of the proper branch.

If you're trying to apply these patches for production, then I would recommend don't do that. This is not for production. It may leave your site broken at any time, and it would be a lot difficult to get support, unless you know what you're doing and any problem that you may find can be resolved by yourself.

Ram_doss’s picture

What patch is required for getting a delete button or checkbox in a multigroup .??

Is it #84 or # 320 ???can u explain me ??

(#75) tht images only i have placed still it doesnt show up ??

rconstantine’s picture

@Ram_doss - It would really help if you would read everything. There are nearly 100 posts before yours that explain everything. The patch here works together with another patch, which Markus mentioned. You need them both, and you need the images that were posted to be put exactly where instructed above! The path you mentioned placing the images is not the right one. Read the posts again. If you don't know how to apply patches correctly, then that is another problem entirely and you should consult the manual page on that topic.

rpanna’s picture

Subscribe

paulludington’s picture

Subscribe

itaine’s picture

does the patch in #84 work with the latest dev version of cck or the stable version released 2008-Nov-12?

guidot’s picture

@itaine: Please read the whole comment #84:

Here's a patch that should be applied to current CVS code from the D6 branch of CCK. It should also work with the latest nightly snapshot.
rconstantine’s picture

@guidot and itaine - nope, it may not. that was against the dev of that date. I'll be re-rolling a new patch, hopefully tomorrow. see also http://drupal.org/node/300084#comment-1254063

guidot’s picture

I'm sorry, I misunderstood the problem.

markus_petrux’s picture

@KarenS/yched: Could you please provide some input on how do you see the latest patch using the 'remove' button. Any chance that this gets in? Is there anything else that needs to be done?

In regards of filefield/imagefield... AFAICT, they work with this patch. Only problem is with multigroup, but that's a separate issue.

If this was committed, I think it doesn't break any other module, and it would make things easier for the rest of things that are going on (multigroups and nested fieldgroups). Well, these other features could start from what' been done here.

KarenS’s picture

We've both been extremely busy. We're talking about issuing a new official release to pick up the latest changes and I think we both want to wait until after that release to add in anything that has any potential of breaking anything. Then we can take a look and see how much of this we can commit.

markus_petrux’s picture

Thanks for the input, Karen. Much appreciated.

PS: AFAICT, this patch doesn't break anything that I am aware of... oh, yeah. I know... :)

rconstantine’s picture

RE #107: I was going to post an overall update on all three patches, but it seems that I've reached my 10MB file quota! I think that must be a mistake from the site upgrade. Anyway, I don't think I changed anything to this patch, just the other two. I'll post the updates once I am allowed to.

coltrane’s picture

Status: Needs review » Needs work

Patch from #84 no longer applies. I'm reviewing this and #119102: Combo field - group different fields into one, so may have a revised patch soon. I'm thankful for all the hard work thats gone into this.

$ patch -p0 < cck-196421-84.patch
patching file content.module
Hunk #1 succeeded at 490 (offset 7 lines).
Hunk #2 succeeded at 502 (offset 7 lines).
Hunk #3 succeeded at 534 (offset 7 lines).
Hunk #4 FAILED at 735.
Hunk #5 succeeded at 781 (offset 7 lines).
Hunk #6 succeeded at 904 (offset 7 lines).
Hunk #7 succeeded at 934 (offset 7 lines).
Hunk #8 succeeded at 1015 (offset 7 lines).
1 out of 8 hunks FAILED -- saving rejects to file content.module.rej
patching file includes/content.node_form.inc
patching file theme/content-edit.js
patching file theme/content-module.css

coltrane’s picture

Status: Needs work » Needs review
FileSize
11.26 KB

An easy reroll of #84

rconstantine’s picture

Feel free to reroll the multifield one as well. If you can, include the change I mentioned somewhere in that thread where I posted an update of some function - sorry I don't recall what it was called.

I think yched will check all of this out soon, so recent patches of these will help, especially since I think that these two should be applied to CCK dev before my nesting fieldgroup one.

Cheers.

markus_petrux’s picture

@rconstantine: One of the things you've mentioned somewhere on the past follow ups on these issues, I missed. It's that piece of code that fixes the wrappers for the Add more buttons. I believe you mean these changes applied to CCK recently: patch for changes to content.node_form.inc from 1.7.2.15 to 1.7.2.18. Well, I just applied those changes to my working copy of the multigroup and it's to working well. I'll post an updated version of the multigroup as soon as possible.

BTW, the issue with filefield has been moving thanks to quicksketch. It's not fully working yet, but it's been applied to their CVS project, and that's great news! :D

Edited - @coltrane #114: your patch is missing the .js file. Also, the images posted in #75 need to be included. I'll post a new patch later, with all these pieces together.

markus_petrux’s picture

FileSize
17.92 KB

Ok, here's an updated patch. It's the one that implements the 'remove' buttons. For newcomers to this issue... before this patch, CCK removes delta values for multiple valued fields when the fields are empty. After this patch, CCK removes delta values based on user request, there's a new 'remove' button in multiple value fields that allows this. and that means that you can keep empty fields in the database, if you wish. If you don't, then you can press the 'remove' button for the corresponding delta value, and CCK will remove that.

The patch also opens the the door to the multigroup module: #119102: Combo field - group different fields into one. The multigroup module can be used to group several multiple valued fields together, and CCK will keep their delta values in sync. Well, the multigroup module has its own issue...

Besides the patch here, you need to get 2 small icons that I uploaded to comment #75 above:

http://drupal.org/files/issues/button-remove.png
http://drupal.org/files/issues/button-throbber.gif

These icons need to be copied to the directory: cck/images

And that's it. Please, bump if I'm missing something here.

rconstantine’s picture

Great work Markus. Thanks for the updated patch to this and multigroups. I may continue with the all-in-one zips until yched has time to review everything. He's mentioned that the end of the month looks good. I still need to incorporate your latest changes.

coltrane’s picture

Patch in #117 applies cleanly and works correctly. All CCK tests pass as well!

@markus_petrux in content_multiple_value_form() in the default case of the switch statement you add

$deltas = array_keys(array_fill(0, $field['multiple'], 0));

may I suggest range() instead since you have $max as one less than $field['multiple'] anyways? Though I don't even know when the default is even hit.

$deltas = range(0, $max);

I'm going to write a test for this patch, hope to have it today or tomorrow.

markus_petrux’s picture

@coltrane #119: Sure, no problem. I have already applied this change to my working copy so that it comes with the next patch. Also, applied this tip to a couple of places of the multigroup module. Sweet, and thanks for the hint. Sometimes I wonder how is it possible that programs I write really work. :)

coltrane’s picture

FileSize
3.25 KB

Patch provides test case for leaving empty values. Requires simpletest 2.7.

Before the #117 patch you'll get two errors with this test case. With patch no errors of course.

The case could probably be cleaned up. I'm not using much of CCKs wrappers because they proved pretty difficult.

hass’s picture

Unsubscribe :-)

markus_petrux’s picture

Well, I needed to update my working copy with the latest changes in CVS. Now, I'm ready to update the patches here, and the multigroup.

One thing I would like to ask first to Ryan, yched and KarenS about a couple of functions added to the nested fieldgroups patch. I mean content_set_form_element() and content_get_form_element().

Since the patch here, is probably a requirement for both, the multigroup and nested fieldgroups, maybe these functions could be included in this patch? This will save the nested fieldgroups patch to touch a few things that could already be provided by the multigroup patch, and I think it will help approach the validation of these monster patches.

Hopefully some of you can see this comment. I plan to start adding changes later today, before rolling an up-to-date version of this patch and the multigroup, that would include these functions and all suggestions made in the multigroup issue.

yched’s picture

I'd rather keep content_set_form_element() and content_get_form_element() out of this patch, since they are not strictly related to the current 'remove' checkbox feature. Let's tackle things in order as much as we can.

I didn't actually apply and test the patch yet, here are a few initial remarks :

- content_set_empty() should at least remove trailing empty values ?

- -1 on adding $item['_remove'] = 0; to all field values on load. This should only be form-related, just like '_weight'.

- content_field('view') :
That change looks related to mltigroups. Could we remove it from the current patch ?
- if ($node->build_mode == NODE_BUILD_PREVIEW && content_handle('widget', 'multiple values', $field) == CONTENT_HANDLE_CORE) {
+ if ($node->build_mode == NODE_BUILD_PREVIEW) {

- UI-wise : do we really want to hide a row once it's 'removed' ? Everything you do in a form should be reversable until you actually submit the form, so ideally the user should be able to 'unremove' if he clicked by mistake. Would save the restriping ?
[edit : would also allow us to display the 'remove' checkboxes even for fields with fixed cardinality (multiple, non unlimited) ?]

- Sort of related : what would typically happen in onRemove js callbacks ?

markus_petrux’s picture

I'd rather keep content_set_form_element() and content_get_form_element() out of this patch, since they are not strictly related to the current 'remove' checkbox feature. Let's tackle things in order as much as we can.

Ok, I'll leave that for the nested fieldgroups patch then. I'm afraid it'll have to modify the multigroup anyway.

content_set_empty() should at least remove trailing empty values ?

hmm... not really as items now should be removed upon user request. If the user needs to add an empty value at end, it's up to him/her to do so.

-1 on adding $item['_remove'] = 0; to all field values on load. This should only be form-related, just like '_weight'.

I have now tried to reproduce the situation that made me add this here, but I couldn't, probably because content_set_empty() was changed after, so I guess this can be removed safely from node load.

- content_field('view') :
That change looks related to mltigroups. Could we remove it from the current patch ?
- if ($node->build_mode == NODE_BUILD_PREVIEW && content_handle('widget', 'multiple values', $field) == CONTENT_HANDLE_CORE) {
+ if ($node->build_mode == NODE_BUILD_PREVIEW) {

Not really, but also multiple value widgets. On node preview, we can sort items and remove those that were flagged for removal so that the preview matches the end result of the node edition. And the updated node edit form can correctly reflect the state of the changes. This is related to the following point as well, so it's a side effect of the way the 'remove' button has been implemented.

Edited: As a plus... this change caused no harm on my tests, and then the multigroup patch doesn't touch anything but the multigroup module code. I mean, it was a lot easier for me to track these separate issues if each one touches different files. So, I would prefer to leave this as is, please.

- UI-wise : do we really want to hide a row once it's 'removed' ? Everything you do in a form should be reversable until you actually submit the form, so ideally the user should be able to 'unremove' if he clicked by mistake. Would save the restriping ?
[edit : would also allow us to display the 'remove' checkboxes even for fields with fixed cardinality (multiple, non unlimited) ?]

hmm... not sure I agree with that. For example, the user cannot revert the state of the form after a click to 'Add more items' button, or the 'upload' or 'delete' buttons of a filefield/imagefield. Or when adding a field or a group in the 'Manage Fields' panel.

If we had to allow the user to 'undelete' a removed item on a multiple values widget, then it would be a bit more complex and possibly confusing as I have no idea on how it should be done in the UI so it causes no confusion. Also, it would add more stuff to customize for themers. On the other hand, removing the item from the UI is just that, it simply disappears and a warning is displayed that say the change does not take effect until the form is submitted, just like the reordering stuff warning.

- Sort of related : what would typically happen in onRemove js callbacks ?

I added that with the filefield in mind, thinking some kind of client side operation would be required, but now the filefield is forward compatible with multigroups and that was not really needed, so it could be removed.

In summary, I could get rid of the onRemove stuff in the javascript if you feel it's not really necessary AND $item['_remove'] = 0; from content_storage('load'). Not sure if I have to modify anything else. Please, advice or feel free to re-roll this yourself. Please, apply the patch and test, feel the sensation of being able to control when a field needs to be removed or not. :-D

rconstantine’s picture

On the other hand, removing the item from the UI is just that, it simply disappears and a warning is displayed that say the change does not take effect until the form is submitted, just like the reordering stuff warning.

IIRC, Markus is right. If one moves away from the page without saving, I think the data remains intact. yched, are you asking to add an undo button there? If the data is still intact, it seems we might be able to re-reveal each deleted row, maybe as an automated "add new row and populate it with data from the database unless we're already storing it locally" kind of thing.

Looks like you guys are well on the way to separating out the patches. I believe I mentioned somewhere that the fieldgroup patch will also patch multigroups a little, but I think that's probably best.

I do love the CONTROL of manual deletion and not emptying fields to get that result!

markus_petrux’s picture

FileSize
16.55 KB

This patch implements the 'remove' buttons for CCK multiple value fields. For newcomers to this issue... before this patch, CCK removes delta values for multiple valued fields when the fields are empty. After this patch, CCK removes delta values based on user request, there's a new 'remove' button in multiple value fields that allows this. and that means that you can keep empty fields in the database, if you wish. If you don't, then you can press the 'remove' button for the corresponding delta value, and CCK will remove that.

The patch also opens the the door to the multigroup module: #119102: Combo field - group different fields into one. The multigroup module can be used to group several multiple valued fields together, and CCK will keep their delta values in sync. Well, the multigroup module has its own issue...

Besides the patch here, you need to get 2 small icons that I uploaded to comment #75 above:

http://drupal.org/files/issues/button-remove.png
http://drupal.org/files/issues/button-throbber.gif

These icons need to be copied to the directory cck/images that you should create.

Chages since last patch in #117:

- comment #119 by coltrane: use range() to compute deltas.
- comment #124/#125 by yched/markus: a) Get rid of the onRemove callbacks in the javascript.
- comment #124/#125 by yched/markus: b) Get rid of $item['_remove'] = 0; from content_storage('load').

And that's it. Please, bump if I'm missing something here.

The attached patch requires latest CCK 6.x-2.x-dev snapshot. Do not use in production environments. No upgrade paths between these development patches.

coltrane’s picture

#127 patch applies fine and all simpletests pass. I'm sure this should get more review from yched and KarenS but it's looking very close to RTBC.

Excuse me if this was addressed in an earlier comment but why must there always be one field on the node (NULL or not) after this patch? I can see the value of being able to remove the one field that is added by default. I see in Drupal.contentRemoveButtons.onClick = function() it hides the remove button when there's only one field. With Javascript off I'm able to mark the single field for removal, but after saving I see the field with a value on the node. @markus any problem with allowing all fields to be removed?

markus_petrux’s picture

Status: Needs review » Needs work

My reasoning here was that CCK stores at least one item in the database, empty or not. Before this patch, it is the same thing. Now, even if you left all fields empty, CCK stores one value with NULLs. This patch doesn't change the way data is stored, it just affects the UI, and here I tried to reflect what really happens. Also, I do not exactly recall, but it made things easier this way in the code. If there were no items, there may be other places in CCK that would have to be touched to get this in.

Aside... there's one thing I forgot to change in the previous patch when getting rid of the onRemove thing in the javascript. It is that the 'rel' attribute of the 'remove' checkbox is not really needed. This will simplify PHP and JS code. I'll try to prepare a new patch with this stuff fixed.

coltrane’s picture

Ah, my bad, it was not clear that CCK leaves a NULL record currently. I thought the patch was changing to that. I'm fine with leaving the existing functionality. Thanks.

markus_petrux’s picture

FileSize
15.33 KB

This patch implements the 'remove' buttons for CCK multiple value fields. For newcomers to this issue... before this patch, CCK removes delta values for multiple valued fields when the fields are empty. After this patch, CCK removes delta values based on user request, there's a new 'remove' button in multiple value fields that allows this. and that means that you can keep empty fields in the database, if you wish. If you don't, then you can press the 'remove' button for the corresponding delta value, and CCK will remove that.

The patch also opens the the door to the multigroup module: #119102: Combo field - group different fields into one. The multigroup module can be used to group several multiple valued fields together, and CCK will keep their delta values in sync. Well, the multigroup module has its own issue...

Besides the patch here, you need to get 2 small icons that I uploaded to comment #75 above:

http://drupal.org/files/issues/button-remove.png
http://drupal.org/files/issues/button-throbber.gif

These icons need to be copied to the directory cck/images that you should create.

Changes since last patch in #127:

- Simplified rendering of 'remove' checkboxes.

And that's it. Please, bump if I'm missing something here.

The attached patch requires latest CCK 6.x-2.x-dev snapshot. Do not use in production environments. No upgrade paths between these development patches.

rconstantine’s picture

Status: Needs work » Reviewed & tested by the community

Looks good to me. Tested in a fresh install. Anyone else want to chime in? For now I'm setting this to reviewed and tested.

markus_petrux’s picture

FileSize
15.35 KB

Patch re-rolled with a small fix that affects the rendering of the drag'n'drop table, as described in the multigroup issue, comment #501. Thanks to andreiashu for pointing it out. ;)

@all: Please, read carefully #131 for further information on this patch, requirements, etc.

rconstantine’s picture

Can anyone else chime in and call this one good? The more of you that confirm this works as advertised, the quicker yched and karens can review this, get it into CCK and close out this issue!

andreiashu’s picture

As I commented in the #119102: Combo field - group different fields into one issue: wouldn't be nice to have some tests that come with this patch ?
This way karens and yched would be happier, right ? :)

markus_petrux’s picture

Unfortunatelly I'm not familiar with the test framework, and also lacking time now to start learning it. I would say anyone with the skills is welcome to provide a patch.

If is there anything else that needs to be changed/fixed in the main patch, I would be glad to work it out, if no one else chimes in before.

andreiashu’s picture

@markus_petrux: I think it would be very helpful if you could provide some ideas/advices on how could we test this. I'm talking about unit-testing of course. Then maybe I (or someone that knows better) could provide a patch for this.

About the patch as it is now: as far as I am concerned it fixes the problem described in this issue.

coltrane’s picture

@andreiashu et al, I provided a simpletest test case in #121: http://drupal.org/node/196421#comment-1363926

andreiashu’s picture

@coltrane @#121 great :) I knew that I saw a test somewhere, but forgot where exactly (I thought it was on the multigroup issue) :)
Now that we have the test I think someone should ping KarenS or/and yched and see if this can be committed, right ?

coltrane’s picture

@andreiashu #139, yeah, there's a test on for multigroups as well (http://drupal.org/node/119102?page=1#comment-1378418) but the test case code needs some work. Feel free to test both if you'd like, or even looking through the code can help, thanks!

andreiashu’s picture

@coltrane thanks!

Agileware’s picture

It seems that at the point of the nodeapi validate op the delete has not been done yet.
So if I have a validation error on one of the items in my multi field and I then delete it and re-save I still get the validation error because the delete functionality has not run yet.

Is fixing this in the scope of this patch or is it a different issue (or not fixable)?

I'll have a look into it myself but thought I would ask in the meantime as someone might already know.

markus_petrux’s picture

Yeah, too tired yesterday that I forgot coltrane did it in #121. Thank you! :-D

@Agileware #142: Good catch. I think it should be fixed here. I'll give it a shot. Thanks

markus_petrux’s picture

Status: Reviewed & tested by the community » Needs work

issue status... oops!

markus_petrux’s picture

Ok, the problem in #142 is a bit more complex. FAPI validation errors also fire, so we need to add something here to bypass FAPI validation as well as to prevent from sending to hook_field(validate) field items that are flagged for removal.

The later is easy as we just need to $items = content_set_empty($field, $items) before hook_field(validate), but the FAPI validation issue is a bit more complex. I need to investigate which is the best way to fix that.

markus_petrux’s picture

BTW, in regards to the simpletest case provided by coltrane in #121... if someone else can confirm it works with latest patch in #133, I will include it in the next one. And maybe I can also figure out how to extend the tests to catch up additional tricky bits that may live around here.

andreiashu’s picture

@#146: I added the test provided by coltrane. It seems to not work so smooth anymore: it fires 5 Notices - all saying: "Undefined index: format" in 'content.module' on line 1085 function 'content_storage()'. First 3 of them are fired when the test saves the node with the 3 custom fields to the database $node = $this->drupalCreateNode($edit); and the last 2 notices are fired when the test code is loading the node from the database: $node = node_load($node->nid, NULL, TRUE);.
I'm not sure how to deal with this, sorry. Any ideas what is wrong ?

Everything else seems to run fine.

markus_petrux’s picture

FileSize
21.54 KB

Ok, here we go. Patch re-rolled to fix the issue reported by Agileware in #142, and it also includes simpletest class provided by coltrane in #121 (with a minor change aimed to fix the problem reported by andreiashu in #147).

Regarding the problem reported by Agileware... this one has been tricky. I had to use #after_build callback to empty the values of items that are flagged for removal so that we don't get validation errors, in FAPI and hook_field('validate') layers. This needs testing with as much different CCK fields as possible, for example dates, optionwidgets, nodereferences, etc. I tried and seems to get fixed, but help testing would be much appreciated.

@all: Before applying this patch: Please, read carefully #131 for further information on this patch, requirements, etc.

yched’s picture

Thanks markus and all for the great work. Thanks also for the code quality, that's fairly uncommon for cck patches :-) !

I've been thinking about this, and here's where I am so far.

The patch tries to do 2 different things :
a) provide a UI to easily empty a field value,
b) change the way CCK stores values.
The two are not related. b) is a major block for multigroups as they currently stand, but is only related to multigroups, and is still controversial. a) is 'just' a UI enhancement, which happens to reveal its full power with multigroups, but is not tied to that feature.
So I'd really prefer that this patch focused on the UI side. The storage aspects that are needed for multigroups should be kept with the multigroup patch. Let's not cram several topics in the same commit, it's the usual practice and will make later maintainance and bug tracking saner.

UI-wise, 2 things bug me :
- The fact that 'remove' is not undoable - If I spent 10 minutes editing my node form and erroneously click 'remove' (or change my mind), then my 10 minutes are lost. With this approach, the non-JS version is actually more usable than the JS version, which is a paradox.
Not convinced by the answers in markus #125 :-) : of the examples you provide, only filefield is not reversable.
- The fact that there's no 'remove' if there's only one value left. This is dictated by the current 'row disappears' behavior so that we're not left with just the table header with no actual visible rows, but there's no real reason for that : if 'removing a value by emptying the input field(s)' is tedious, then it's also true for the 'last value', and will be equally tedious for multigroups.

I've been playing with the following idea :
instead of hiding the whole row, we just hide the *contents* of the row, and keep the 'remove' button visible. The row thus collapses to the height of the button and frees screen real-estate, the behavior of the button becomes 'unremove'. This can be applied to all the rows, even when there's only one left.
Thoughts ? I can try to see how this works code-wise.

Side note : +50 for tests, of course !. If we stick to the UI feature right now, coltrane's tests in #121 look like they should do the job once the 'exceptions' are fixed. They should also test 'remove all values' if the patch eventually allows this.

markus_petrux’s picture

Ugh, we just posted almost at the same time.

Well, I would appreciate testing of the patch in #148 for the newly added stuff regarding the bug reported by Agileware.

That being said, I'll try to implement the 'unremove' button. Thet other thing about a) and b)... not sure how I can really separate things here. It is all really complex and separating thing would require me to spend a bunch of time again, when if we leave this as-is, it doesn't affect anything else. This is for CCK with or without multigroups. Really, can you imagine how many hours that (I don't really have) I spent on this? This patches are increasing the cost of our project too much. I'll work on the 'unremove' thing, but not this one. And I'm happy if anyone else separates the patch to implement a) and b) separately in way that patching for a) only doesn't break things.

coltrane’s picture

@yched #149 you have a point that non-JS is different functionality than the JS version but I'm not in agreement that there should be an "unremove" option. With such an option, and is the case with non-JS version, the UI can get cluttered with removed-but not visually gone items and normal items. If someone spends 10 minutes adding values to a complex mutligroup and accidentally clicks the remove button, well, too bad, we can't hold their hand for everything.

rconstantine’s picture

@coltrane, true, but netflix is an example of removing a row (from your movie queue) and then adding it back. If the information really isn't gone, is it super hard to do?

markus_petrux’s picture

FileSize
1.24 KB
21.5 KB

Ok, here's a new version of the patch that implements the 'restore' button. Feel free to critique.

Now, there's only one button "remove.png" that I attach here. This button should be copied to cck/images, directory that you should create.

BTW; the multigroup module is now broken. Needs to be adapted to whatever we decice to do here.

coltrane’s picture

FileSize
12.96 KB

I ran quick test of #153 patch without errors. Also ran simpletest test case and there were no errors and no exceptions.

@rconstantine #152: The answer to "is it hard to do" is yes, I think. Markus can reply to the difficulty in coding it, but I think a "hard" part of doing the unremove feature is making it usable. The unremove in 153 would be confusing to use if I didn't already know what would happen. (No offense meant, Markus)

Take a look at the attachment to see what #153 patch adds. I don't think the orange unremove button clearly informs the user that clicking it again will undo their removal. And the fade out of the input field doesn't help.

I'm still not 100% in favor of an "unremove", but if it is going in what about the equivalent of setting the field to disabled, so it's greyed out? I don't have an idea for the button though ...

rconstantine’s picture

For the graphic, how about an arrow curving up to the left like in a word processor? And would it be hard to place a message where the data was? Again, looking at Netflix, they leave the row, but put a message there stating what to do to get the data back.

Thanks Markus, for persisting in this. I know how much more time you've spent on this than you wanted to or really should have.

markus_petrux’s picture

Thanks for testing so quicky. :-D

Disabled form elements are not sent to the server, so that would a new layer of complexity.

Aside from the look that can be seen in the screenshot above, the title of the icon changes from 'Remove this item' to 'Restore this item'.

Also, I tried to make it easy for themers to adapt the look of everything in there.

TBH, I don't care too much if this feature comes with a 'restore' button, as the people here at the office found it ok or without.

Back on the other suggestion by yched regarding the statement "change the way CCK stores values". Well, I've been thinking on this for a while again, and that sentence is not completely accurate. This patch does NOT change the way CCK stores values. Rather it provides a method to keep track of deltas in a way that doesn't break existing code and that's compatible with the multigroups. This means that items are not removed when empty anymore. It is the user who decides when an item needs to be deleted, so that means we need to change the way content_set_empty() works. There's no change in the way fields are stored in the database. So... I cannot see how this patch can be really splitted into 2 different things, that will need to get in anyway. Working on this was very tricky and that's what I ended up doing. Could there be better ways? Probably, but I just went for the one that seemed better to me.

Again, there is nothing written in stone, so anyone is welcome to provide alternatives. I happy as far as we get this patches in because we would prefer running a multigroup module that's supported.

@ryan: could you please post a screenshot? Also, graphics for the icons are welcome. I just made that one this afternoon, but I'm not expert on this, so that's what I was able to... :-|

rconstantine’s picture

@Markus, for icons, try http://drupal.org/node/13911 which are GPL'd. I would've thought that's where you got the ones you're using. I guess you didn't. As for what Netflix looks like, check out the attached. It appears that the items are not removed via AJAX, but are simply marked for deletion. The message is placed seemingly on top of the existing row via javascript fade-in/fade-out. Clicking undo restores the row. It looks like you are doing the same kind of thing, but without a message. I haven't looked at your code, so I'm not sure what you're doing exactly.

markus_petrux’s picture

No, I got the base 'remove' icons from views. What I did for latest patch was adding the 'restore' icons to the sprite. Same look but different colors, just to keep going. Thanks for the link to the icons issue, but I'm not really sure if there is a standard for icons, so I tried to use something known (icons from views ui). Anyone can come up at any time with a new sprite with alternative icons.

Also, thanks for the netflix screenshot. But how do we know here which text represents the item that's been just removed? It would be easier if it was something generic, and if you want, that can be altered by a separate behavior. Any idea on which text could be shown "by default"?

rconstantine’s picture

For non-multigroup fields, we should just be able to display the contents of the field since there is only one field, no? Just use the default formatter. That should also account for the case where someone has developed a multi-field field in the old cck way (and which sometimes still makes sense despite multigroups' functionality). As for multigroups, I'd say either the first field, or let the admin select a 'primary' field.

But really, a generic message could go in now, with a new issue created after this is in dev to address a more friendly message. I don't think this should be held up on a message or an icon. Yched may choose to use different icons for the release, and that should be fine. If he doesn't like the orange circle/slash, then I suppose he could choose what he wants. Am I making sense?

andreiashu’s picture

about #149, @yched: This issue started in November 29, 2007 and just now we have a workable solution. I think it is a bit unfair (for markus and all other ppl who worked on this) deciding now that we must split this patch into two or add any sort of additional features. Of course I'm not an expert on this, but couldn't this get committed as it is now and maybe solve it later ?

// Edit: by "committed as it is now and maybe solve it later" what I really wanted to say was "get committed as it is and maybe solve other features later"

Agileware’s picture

@Markus,

In regards to your patch at #148 (I'm not using the newer one because I don't really need the undo and I need it to work with the multi-field functionality)
I am using a multifield with a date field (select boxes) and a select option widget.

I am still getting the issue where deletions are not done until after my nodeapi validate. But I noticed that there is now a _remove item in the form that gets passed through the $a3 param to the validate. I can use this to make my validation skip values where the _remove value is 1.

Is this how it is supposed to work, me using the _remove to validate as opposed to the value already being removed.

I don't mind either way, I'm just checking.

Agileware’s picture

Maybe that _remove part was already there before too and I just didn't notice it.

markus_petrux’s picture

hmm... no, I think this should be transparent.

Let's see what I did is adding a FAPI after_build callback that clears the values of items flagged for removal. Well, in fact, the after_build callback deals with the values of the items in the $form structure and in $form_state, but not in the ['#post'] attribute of the elements, which is still a copy of $_POST.

Now that you mention dates... IIRC, I had to fix ['#post'] in the multigroup module because of dates. I just forgot to do it here. :(

markus_petrux’s picture

@Agileware: Could you please try these functions? I hope you can figure out where it goes, as I won't be updating the other patches now.

These functions should replace those that I added to includes/content.node_form.inc at #148

/**
 * After build callback for multiple value fields.
 *
 * When an item has been flagged for removal, we need to empty user input
 * to prevent from getting validation errors.
 */
function content_multiple_value_after_build($elements, &$form_state) {
  // Process the element only when we have user input that needs validatation.
  if (!empty($elements['#needs_validation'])) {
    // Check if the item has been flagged for removal.
    if (isset($elements['_remove']) && !empty($elements['_remove']['#value'])) {

      // Remove user input for the top level value of the element.
      if (isset($elements['#value'])) {
        // Update the value in the form element.
        $elements['#value'] = NULL;

        // Update the value in the $form_state.
        $empty = array(
          '_weight' => $elements['_weight']['#value'],
          '_remove' => 1,
        );
        foreach ($elements['#columns'] as $column) {
          $empty[$column] = NULL;
        }
        form_set_value($elements, $empty, $form_state);

        // Update the value in the #post attribute of the element.
        $post = &$elements['#post'];
        foreach ($elements['#parents'] as $name) {
          $post = &$post[$name];
        }
        $post = $empty;
      }

      // Now, recursively remove the values of children elements.
      foreach (element_children($elements) as $key) {
        if ($key != '_weight' && $key != '_remove') {
          content_multiple_value_remove_values($elements[$key], $form_state, $elements['#post']);
        }
      }
    }
  }
  return $elements;
}

/**
 * Helper function to empty values for items flagged for removal recursively.
 */
function content_multiple_value_remove_values(&$elements, &$form_state, &$post) {
  foreach (element_children($elements) as $key) {
    if (isset($elements[$key]) && $elements[$key]) {

      // Remove values for items flagged for removal.
      if (isset($elements[$key]['#value'])) {
        $elements[$key]['#value'] = NULL;
        form_set_value($elements[$key], NULL, $form_state);
        $elements[$key]['#post'] = $post;
      }

      // Recurse through all children elements.
      content_multiple_value_remove_values($elements[$key], $form_state, $post);
    }
  }
}

Once confirmed I'll update the patch in #148 with them.

Agileware’s picture

Thanks for your work Markus.

I'm still seeing the same issue with the new code.

I might be checking in the wrong place for my validation but if my form has group_two_fields, which then has field_date and field_select in it I have checked the following (where 0 represents the delta):

$form['group_two_fields'][0]['field_date']['#value']['value']
$form['group_two_fields'][0]['#post']['field_date'][0]['value']
$form['group_two_fields'][0]['field_date']['#post']['field_date'][0]['value']
$form['group_two_fields']['#post']['field_date'][0]['value']
$form['#post']['field_date'][0]['value']

All of them still have the deleted values at the time of the nodeapi validate.

I can successfully use the #removed value though and skip validation based on that.

yched’s picture

Markus : thanks for rerolling so fast, I'm currently testing. I do think the behavior with 'unremove' is much better. The icons issues can definitely be tackled later.

About _removed and validate, I'd suggest another approach :
- Prune the _removed values in content_field('validate') (there's currently an empty switch case)

    case 'validate':
      foreach ($items as $delta => $item) {
        if (isset($item['_remove']) && $item['_remove']) {
          unset($items[$delta]);
        }
      }
      break;

- Switch the order of content_validate() :

  _content_field_invoke_default('validate', $node, $form);
  _content_field_invoke('validate', $node, $form);

Seems to work on my setup.
Ideally, a cleaner way might be to make the 'content_multiple_values' form wrapper a real input element and handle _remove and _weight by a #value_callback so they stay at FAPI level, but the approach above should work, and is consistent with how '_weight' is currently handled.

About empty values and consecutive deltas : this *is* changing how CCK storage works :-), and I do stand by the fact that this should be decoupled IMO. I'll expand tomorrow (quite late here). I don't mean by this that you have to do this yourself, though, and I can measure and appreciate the amount of time you've already put into this, so I perfectly understand if you pass on this. I'll look into this within the very next days.

markus_petrux’s picture

FileSize
22.33 KB

@Agileware #165: Ahah! Now I see that your fields are using multigroups, and that's why this is not working. The multigroup module is now broken and needs to be reworked. This patch is for multiple value fields not in multigroups.

@Yves #166: So I think with the updated after_build callback there's no need to mess with the validate stuff as the items flagged for removal here are filled with NULL, keeping their _weight and _remove values. I also thought about doing content_set_empty() inside content_field itself, but checking for $op == 'validate'.

As per splitting this patch... well, I don't see why as all needs to get in, afaict. And this works with and without multigroups. So it's up to you.

Here's an updated patch. The sprites for the remove/restore buttons can be found in #153. This icon should be placed in the cck/images directory that you should create.

Agileware’s picture

OK thanks,

I can stay with my #remove based validation and it's fine.
Other than that this patch seems to work well with the multigroups one from my testing.

Thanks all for both these patches.

yched’s picture

Looking hard at the code in content_multiple_value_after_build :

- the code assumes the widget structure maps to the field columns, meaning : 'if field columns are column_a and column_b, then i'll find something like $form['field_foo']['column_a'] and $form['field_foo']['column_a'].

        foreach ($elements['#columns'] as $column) {
          $empty[$column] = NULL;
        }
        form_set_value($elements, $empty, $form_state);

This cannot be assumed. Widgets can be structured in their own way, and massage the data into the expected 'field' format at process time.
- at each level in the $form, $form['#post'] is supposed to contain the *full* $_POST, but the code overwrites it with new values just for the element.
- Isn't there a recursion bug around content_multiple_value_remove_values() ? the function is first called on the children of the top-level $element, and the function itself starts with a foreach (element_children($elements) as $key) {, so it looks like the first level of children elements is never treated.

Overall, this part of the code (the after_build) looks extremely convoluted and fragile. I really don't want to sound like I'm systematically hindering any of this, but there has to be a simpler way. The approach I proposed in #166 has one drawback : it doesn't prevent widget validation (FAPI #element_validate callbacks e.g nodereference_autocomplete_validate) to run on removed values. I'll think a little more...

markus_petrux’s picture

Please, consider these functions instead:

/**
 * After build callback for multiple value fields.
 *
 * When an item has been flagged for removal, we need to empty user input
 * to prevent from getting validation errors.
 */
function content_multiple_value_after_build($elements, &$form_state) {
  // Process the element only when we have user input that needs validatation.
  if (!empty($elements['#needs_validation'])) {
    // Check if the item has been flagged for removal.
    if (isset($elements['_remove']) && !empty($elements['_remove']['#value'])) {

      // Remove user input for the top level value of the element.
      if (isset($elements['#value'])) {
        // Update the value in the form element.
        $elements['#value'] = NULL;

        // Prevent FAPI from invoking validation callbacks for this element.
        $elements['#validated'] = TRUE;

        // Update the value in the $form_state.
        $empty = array(
          '_weight' => $elements['_weight']['#value'],
          '_remove' => 1,
        );
        form_set_value($elements, $empty, $form_state);

        // Update the value in the #post attribute of the element.
        $post = &$elements['#post'];
        foreach ($elements['#parents'] as $name) {
          $post = &$post[$name];
        }
        $post = $empty;
      }

      // Now, recursively remove the values of children elements.
      foreach (element_children($elements) as $key) {
        if ($key != '_weight' && $key != '_remove') {
          content_multiple_value_remove_values($elements[$key], $form_state, $elements['#post']);
        }
      }
    }
  }
  return $elements;
}

/**
 * Helper function to empty values for items flagged for removal recursively.
 */
function content_multiple_value_remove_values(&$elements, &$form_state, &$post) {
  foreach (element_children($elements) as $key) {
    if (isset($elements[$key]) && $elements[$key]) {

      // Remove values for items flagged for removal.
      if (isset($elements[$key]['#value'])) {
        $elements[$key]['#value'] = NULL;
        $elements[$key]['#validated'] = TRUE;
        form_set_value($elements[$key], NULL, $form_state);
        $elements[$key]['#post'] = $post;
      }

      // Recurse through all children elements.
      content_multiple_value_remove_values($elements[$key], $form_state, $post);
    }
  }
}

Fragile, no. Tricky, yes. It's just a complex data structure, not completely unknown. We should be able to do what we need with just the parts we know.

Comments on the snippet:

- content_multiple_value_after_build() deals with the first level of the widget data, content_multiple_value_remove_values() would only be invoked in cases where the widget added subelements, such us nodereference, etc.

- When $empty is computed, we don't really need to know the columns. We can get rid of that part. All we need is the weight and the remove state.

- $form['#post'] contains a copy of the $_POST, yes. But each element contain its own copy as well. What we need to do here is alter the copy of the element, and its subelements, which is what their FAPI callbacks will see. I'm using a loop to get into the proper level of the $element['#post'] structure by following references of the keys stored in $element['#parents']. This is what form_set_value() does for the $form_state['values']. But we need to alter the #post copy of the element because date fields check that. BTW, why do date fields check #post instead of $form_state, I don't know.

- Finally, I have now added $elements['#validated'] = TRUE; so that FAPI will not invoke the validation callbacks for this element. So we don't get validation errors for items that have been flagged for removal.

Agileware’s picture

I agree that at least a generic message for removed fields would be good.

Without it it isn't very easy for the user to know what is going on.

markus_petrux’s picture

If there was a message here, it would have to be something that can be easily overridden, I think. That way someone could display a personalized message for a particular widget.

Also, the method that performs this operation would ideally have to be easily overridden. Note that this code would probably be shared with the multigroup, and in the context of a multigroup there may be several cells per table row, and then the message would have to spread more than one cell, so that complicates things a little.

markus_petrux’s picture

Here's an alternative version of the file content-edit.js provided by the latest patch in #167.

In addition to hiding the form elements of the removed row, it displays a small message "Removed" that can be easily themed or even changed by overriding a method of the corresponding tableDrag object. It's simple, but I believe it's better now.

What would make things a lot more intuitive is a new set of icons for the restore state. Anyone with graphics skills to provide a new sprite with easy to understand icons?

Edited: for some reason, the comment is lost when attaching a file. So I had to edit again. Lucky me, I had a copy in the clipboard.

markus_petrux’s picture

FileSize
944 bytes

How about these icons?

I have changed the restore icons in the sprite for the 'undo' icon from TinyMCE. :-o ...better now?

The offsets in background images needs to be adjusted as follows:

.content-multiple-remove-button:hover {
  background-position: 0 -14px;
}
.content-multiple-removed-row .content-multiple-remove-button {
  background-position: 0 -28px;
}
.content-multiple-removed-row .content-multiple-remove-button:hover {
  background-position: 0 -42px;
}
andreiashu’s picture

How important is the aspect of the icons right now ? I mean couldn't we change them in a followup issue if they aren't perfect ?

markus_petrux’s picture

@andreiashu #175: Aside from these minor touches, I'm basically done here. The patch is now in the hands of yched.

I posted the .js and the icons because I think it's important that this is as much intuitive as possible.

yched’s picture

The icons and the 'removed' text are great.
Minor problem : one 'removed' text is added per element in the cell. OK for most widgets, but a noderef autocomplete widget comes with a hidden input field (which holds the autocomplete path), so you end up with two 'removed' texts.

Drupal.contentRemoveButtons and _isBusy look like they're not needed anymore ?

About content_multiple_value_after_build() :
- The explanations in #170 about $empty and #post make sense, thanks.
- I still see a recursion issue around content_multiple_value_remove_values() :
the first level of children below the original $elements is not treated, because the code chains two element_children() : one in content_multiple_value_after_build(), one in content_multiple_value_remove_values(). Thus recursion only starts at the 2nd level, first one is bypassed.
I'd say you want to remove the element_children() in content_multiple_value_after_build() and call content_multiple_value_remove_values() directly on $elements
Side note: aside from '_weight' and '_remove', you probably also want to avoid the '_error_field' element

Issues with noderef field / autocomplete widget :
- 'remove' a value / 'add another item' / 'unremove' the previously removed value : the widget comes back empty. Works ok with a simple text field or an imagefield: the widget comes back with the 'unremoved' value.
- Same behavior whith : Remove / submit with validation error (eg missing node title) / unremove : noderef not ok, textfield and imagefield ok
- Reversedly, if you do: remove / preview / unremove, noderef *is* ok, textfield and imagefield are not ok
This looks related to the recursion bug above, and/or to the fact that an autocomplete noderef widget is a nested structure (internally calls a textfield widget), but I cannot sort it out.
I do find this code terribly convoluted, and again I'd really favor going as suggested in #166, which behaves fine in the edge cases outlined above - at the cost of requiring widget validation code to take care of not to throw errors on removed values, I think we can live with that.

yched’s picture

For the record, here's the current patch (#167) updated with the changes in #170 and #174. Requires the image provided in #174.

rconstantine’s picture

Thanks for the diligence.

markus_petrux’s picture

Thank you for reviewing all these stuff, Yves! It's really, really appreciated.

I'll try to find a hole to try to post an updated patch with your suggestions, hopefully during the course of this week.

Agileware’s picture

FileSize
505 bytes

This patch can be applied after the patch at #178 and it will centre the remove icon within it's table cell.

markus_petrux’s picture

@ #181: Table cells may have varying widths, hence the button position may vary if centered. On the other hand, floated to the right it is always at the same position. There may be other ways to achieve the same effect, though.

markus_petrux’s picture

@yched:

Minor problem : one 'removed' text is added per element in the cell. OK for most widgets, but a noderef autocomplete widget comes with a hidden input field (which holds the autocomplete path), so you end up with two 'removed' texts.

Drupal.contentRemoveButtons and _isBusy look like they're not needed anymore ?

Ok, I have these 2 issues fixed. The former was a bit tricky, but it all looks cleaner. I'll show that when the rest is (or seem to be) working here.

Regarding content_multiple_value_after_build() and noderef issues... I'm now experimenting with your idea in #166 to swap the functions related to nodeapi('validate'). That part seems to work here as well, BUT... we still need to prevent from getting errors generated from FAPI validation callbacks for items flagged for removal, so I tried with the following snippet, and it seems to work ok, BUT... FAPI validation routines are also used by fields such as noderef to alter $form_state['values'], and if we disable FAPI validation this operation is not performed, which leads to break the values of items flagged for removal when doing a preview. The only way I can think at this moment to avoid this kind of issues is to really remove the items flagged from the form during the after_build processing. Not only prevent validation or nulling the values, but get rid of them from the form structure completely. Thoughts?

/**
 * After build callback for multiple value fields.
 */
function content_multiple_value_after_build($elements, &$form_state) {
  // Process the element only when needs validatation and it has been flagged for removal.
  if (!empty($elements['#needs_validation']) && isset($elements['_remove']) && !empty($elements['_remove']['#value'])) {
    content_multiple_value_after_build_recursive($elements);
  }
  return $elements;
}

/**
 * Helper function to deal with items flagged for removal recursively.
 */
function content_multiple_value_after_build_recursive(&$elements) {
  foreach (element_children($elements) as $key) {
    if (isset($elements[$key]) && $elements[$key] && !in_array($key, array('_weight', '_remove', '_error_element'))) {

      // Prevent FAPI validation for items flagged for removal.
      $elements[$key]['#validated'] = TRUE;

      // Recurse through all children elements.
      content_multiple_value_after_build_recursive($elements[$key]);
    }
  }
}
markus_petrux’s picture

Bah, here's the updated version of content-edit.js. Please, let me know how it looks now.

BTW, now that we're about to add a new .js file to CCK, does the following move worth? #421116: May we move the file content.js to js/content.admin.js?

markus_petrux’s picture

#183 / Follow up...

I have tried with the following:

/**
 * After build callback for multiple value fields.
 */
function content_multiple_value_after_build($elements, &$form_state) {
  // Process the element only when needs validatation and it has been flagged for removal.
  if (!empty($elements['#needs_validation']) && isset($elements['_remove']) && !empty($elements['_remove']['#value'])) {
    $elements = array();
  }
  return $elements;
}

And then... edit / remove a multivalue noderef / preview / restore the previously removed noderef. The value is shown as "[nid:Array]". LOL

In summary:

a) if we keep the values of items flagged for removal because some field implementations need to change the $form_state['values'] array, then we may get validation errors for removed fields, which is weird.

b) If we want to allow the user to restore a value after preview, we need to keep the value, but we can't because a).

So the only way is to get rid of the values form items flagged for removal. Caveat: user won't be able to restore these values after preview. User won't be able to restore the value of a field that was edited and changed anyway, so I'm thinking this is the only way to go.

Thoughts?

markus_petrux’s picture

Well, I'll try to work out a new patch with the latest mentioned approach, and here I'll restore the _busy thingy because if you press the remove button repeated times, you can end up with more than one 'Remove' warning.

yched’s picture

I'm fine with requiring widget validators to check for _removed before firing form errors. Meaning : a widget validate callback does its 'values massaging' thing (if any) for all values, but does the actual 'check stuff and raise errors if needed' inside an if (empty(_remove) {.
I think the most current case will be to remove a value from an existing node, so the values in place should already be valid (OK, unless field properties have changed meanwhile, but you know what I mean).
Worst case, typing an invalid value, removing it, and submitting will raise an error until the widget code is updated, but that won't affect *all* widgets, and I think we can live with that. I very much prefer that than messing too much with the form structure.

About _busy - suggestion (dunno if that's really simpler) : can't we unbind the behavior while firing the animation, and rebind it afterwards ? Not sure this is a big deal though.
Besides : I'd vote to make the anim 'fast' instead of 'slow' anyway, so this would reduce the chances of too much clicks ?

[edit : core's collapse.js uses an 'animating' property to take care of such cases, so yes, I guess we should deal with this. No dealbreaker for the initial commit, though...]

markus_petrux’s picture

I'm fine with requiring widget validators to check for _removed before firing form errors. {...} I very much prefer that than messing too much with the form structure.

Ok, so we don't need to do implement after_build, and keep the rest of the code including your idea to swap field handlers in nodeapi('vaidate')?

Sorry, I've been rounding in circles and I'm now a bit confused.

Regarding the busy flag... ok, I keep that, and when I post the new version I have now, you'll see that's really necessary, even if using 'fast' animation. Such a technique is not so uncommon.

And I presume we're reaching a point where we're near somewhere there was much rejoicing... :-D

yched’s picture

Right, let's drop after_build and clean $items where removed = TRUE in content_field('validate') (and in presave, but that part is already done through content_set_empty() )

About the rejoicing : yes, we're getting close :-). *Many* thanks for your efforts Markus. I still intend, however, to defer the 'no garbage collect of empty values / save non consecutive deltas' part. We don't *need* it until the multigroup patch lands, and I'm still pondering a way to have this behavior only affect fields in multigroups. D7 field default storage will store consecutive deltas, and multigroup / combo field in D7 will be able to use its own storage logic (see #368674: Implement hybrid storage for Fields in Core), so I'd really prefer to stay inline with what lies ahead, and do only what's strictly needed to allow D6's multigroups.
I completely understand if you don't want to spend more time on splitting the patch, though, and as I said above, I'll take care of this.

markus_petrux’s picture

FileSize
25.36 KB

Ok, here's an updated patch. It requires the icon provided in #174 that should be copied to cck/images, directory that you should create.

It includes a rewritten .js file for the manipulation of the remove buttons, and all the changes discussed in previous comments, except the changes that would be required to field modules in order to skip validation for fields flagged for removal. Reason is I tried, but it is not so easy, so let's explore this...

For example, the noderef field would need to check for '_remove' in its validation callback for the autocomplete widget, but the problem is that this flag is one level above in the $form structure, so this function does not know if the item has been flagged for removal. So... how can we do this?

Edit: One possible why to give field validation handlers the state of the '_remove' flag would require an after_build callback again, but this time we could copy the state of the '_remove' element to $element['#content-remove'] for all child elements. Agree on this?

BTW, now that we're about to add a new .js file to CCK, does the following move worth? #421116: May we move the file content.js to js/content.admin.js?

markus_petrux’s picture

Update: Even if we add the '#content-remove' attribute to the element, we can still get validation errors from FAPI: if field is required, etc.

So... I'm thinking there's no other choice than to get tricky and get to code an after_build callback that's wise enough. I'm now playing with this after_build thing:

/**
 * After build callback for multiple value fields.
 */
function content_multiple_value_after_build($elements, &$form_state) {
  // Process the element only when needs validatation and it has been flagged for removal.
  if (!empty($elements['#needs_validation']) && isset($elements['_remove']) && !empty($elements['_remove']['#value'])) {

    // Update the value in the #post attribute of the elements.
    $post = &$elements['#post'];
    foreach ($elements['#parents'] as $name) {
      $post = &$post[$name];
    }
    $post = array('_weight' => $elements['_weight']['#value'], '_remove' => 1);

    // Alter the value of this element and children recursively.
    content_multiple_value_after_build_recursive($elements, $elements['#post']);
  }
  return $elements;
}

/**
 * Helper function to deal with items flagged for removal recursively.
 */
function content_multiple_value_after_build_recursive(&$elements, $post) {
  foreach (element_children($elements) as $key) {
    if (isset($elements[$key]) && $elements[$key] && !in_array($key, array('_weight', '_remove', '_error_element'))) {
      // Recurse through all children elements.
      content_multiple_value_after_build_recursive($elements[$key], $post);
    }
  }

  // Remove values for items flagged for removal.
  if (isset($elements['#value'])) {
    $elements['#value'] = NULL;
    form_set_value($elements, NULL, $form_state);
    $elements['#post'] = $post;

    // Prevent validation errors for required fields issued by FAPI itself.
    $elements['#required'] = FALSE;
  }
}

I think this fixes the issue that were happening with my previous versions of this after_build logic (see #177).

TODO: Disabling #required is not correct. What we really need here is reset the '_remove' attribute of the element, or something of that sort... still thinking about it... hmmm... should we allow the user remove a required item? hmm... hmm... if so, then we shouldn't generate a remove button for required elements. Right? Edit: No. If we do, then the first item doesn't have button, but more items will, so what we need is allow removing all values except one. This means we cannot delegate validation of required items to FAPI, we need to do it here ourselves. yched: I'm sorry, but this means our after_build callback will get a bit more tricky, unless someone else can come with another solution that works. IMHO, this feature cannot be shipped with something that not covers all this little issues.

yched’s picture

The new JS animation behavior / 'Removed' text is really nifty !
Minor :
- could we rename ._busy to .animating ?
- this part should be moved at the beginning of the file ?

/**
 * Private namespace for local methods.
 */
Drupal.contentRemoveButtons = Drupal.contentRemoveButtons || { _isBusy: false };

- current implementation means one shared _busy state for all buttons ? There are little chances that a user is able to click several buttons while one is animating, but code-wise this looks strange. Shouldn't _busy/animating be a property of the button ? No deal breaker, can be fixed later on.

_remove / widget validators : Aw. Yes, transmitting the _removed down to children in after_build sounds fine.

#421116: May we move the file content.js to js/content.admin.js? : answered over there.

markus_petrux’s picture

Regarding the .js stuff... sweet. Though, a per button animating flag is not necessary, specially since we're using 'fast' animation. I'm unable to click a second button while one is animating, but it is still needed as it's much easier to click the same button several times.

Regarding the '_remove' stuff... I edited last part of my previous comment. I think we cannot delegate validation of #required to FAPI. Otherwise, I'm not sure what else can be done to solve the problem. The problem is that we can now get validation errors for items that have been flagged for removal. These errors may be originated by field validation handlers or by FAPI itself.

yched’s picture

Crossposted.

Actually I think the current 'required' feature is messy even without this patch : empty the first row, fill a value in the second row, drag the first row below the second row, submit: validation error, because the #required is hardcoded on the first element... It's been like that for over a year and no one complained so far, but this is messy still, and this patch will make this more visible.

As you point out, the real solution would be to handle 'required' validation outside FAPI/widget level but at field level, in content_field('validate'). In this case, no need for a complex after_build solution, right ?

markus_petrux’s picture

hmm... so for multiple value fields, we always set FAPI elements as #required = FALSE, and check this field attribute in nodeapi('validate')? Looks a nice approach. I'll try to play with this idea, and post back. Later, as I'm going to take a walk with the kids. :)

One question: is it possible that field process callbacks set the required attribute at FAPI level? I guess not, right? So it is CCK who controls this... and that make things easier, it seems.

Another question: FAPI can also issue errors related to #maxlength. What do we do with these?

yched’s picture

"is it possible that field process callbacks set the required attribute at FAPI level?" : not sure I get what you mean, but the answer is probably no :-). Field-level stuff happens after FAPI level stuff is done.

Actually, we will need to move 'required' validation out of FAPI in D7 anyway, since D7 theoretically supports validation on programmatic saves (even though no actual fieldable entity currently uses it).

Finding the right form element to assign the error to will be a little tricky, though... We refactored this to be cleaner in D7, but it's not too nice in D6 because of hook_nodeapi / FAPI limitations. It's based on the '_error_element' that widgets are kind enough to declare (probably not all contrib widgets do, though), but finding the right delta might be complicated. I guess it should be flagged on the item with the lowest weight ? Well, at worst the error will be flagged on the wrong item, but it will not be raised when it shouldn't, which is currently the biggest issue.

yched’s picture

Also : even though we want to move away from #required, we'll still want the 'required' theming on form label :-(

markus_petrux’s picture

FileSize
31.7 KB

Ok, here's an updated patch. It requires the icon provided in #174 that should be copied to cck/images, directory that you should create.

Note the new .js file is moved to cck/js, directory that you should create. Related issue: #421116: May we move the file content.js to js/content.admin.js?

Now for the heavy part... I had to use a combination of nodeapi('validate') and after_build. I hope the inline comments in the patch are descriptive enough. It's tricky, but I was unable another way to achive what we need here. And I tried a few combinations...

Also, I have no idea on how this patch can be spitted into 2 different steps. @yched, if you want to try this, then I guess there's somethins that you probably think is not really needed, or that can be implemented in a different way, but really, I can't see how. This works with and without multigroups in mind, so if it needs to get in all, then I can't where's the benefit on splitting the patch. Try if you wish... though.

markus_petrux’s picture

Status: Needs work » Needs review

Sorry, I should have changed the issue status.

markus_petrux’s picture

@yched: Trying to understand your concern about the changes in how the data is stored, I believe I finally got it. Please, correct me if I'm wrong.

Since we can now store empty items, and there may be several empty items, so we are adding more empty rows to the database than before this patch, and this affects views, etc. Right?

Ok, so we can add a 3rd argument to content_set_empty() that is FALSE by default, and it is set to TRUE in content_field('presave'). When TRUE, content_set_empty() filters items flagged for removal AND empty items, but keeping the delta values, which is something that is already supported during node load, and I think it should work with and without multigroups.

Am I missing anything else?

Edited: Instead of the above, how about dealing with this in content_storage() ?

This is the snippet of content_storage() that could deal with multiple value fields:

          // Collect records for non-empty items.
          $function = $field['module'] .'_content_is_empty';
          $records = array();
          foreach ($node->$field['field_name'] as $delta => $item) {
            if (!$function($item, $field)) {
              $record = array();
              foreach ($db_info['columns'] as $column => $attributes) {
                $record[$attributes['column']] = $item[$column];
              }
              $record['nid'] = $node->nid;
              $record['vid'] = $node->vid;
              $record['delta'] = $delta;
              $records[] = $record;
            }
          }

          // If there was no non-empty item, insert a NULL values.
          if (empty($records)) {
            $record = array();
            foreach ($db_info['columns'] as $column => $attributes) {
              $record[$attributes['column']] = NULL;
            }
            $record['nid'] = $node->nid;
            $record['vid'] = $node->vid;
            $record['delta'] = $delta;
            $records[] = $record;
          }

          // Insert the collected records for this field into database.
          foreach ($records as $record) {
            content_write_record($db_info['table'], $record);
          }

And this way we don't add more empty records to the database than what CCK already does. There's no problem with non-consecutive deltas here because node load already takes care of that.

Do this help to close the ring? :)

markus_petrux’s picture

Status: Needs review » Needs work

I tested the above mentioned change in content_storage() and seems to work like a charm here. And the more I think about it, the more a like this approach. This is independent of the rest of the management of multiple fields, and now I'm even tempted to move all this part in content_storage() to some kind of hook that can be implemented by multigroup to provide per multigroup storage. LOL

FYI: this change raised a couple of bugs in the multigroup module, that I have already fixed. a) The way $group_deltas was computed in node view and node form was not correctly collecting unique deltas, and b) default values for fields need to be provided only when creating the node, and I was getting default for delta items that were not in the database after inserting empty values.

Setting this issue to 'needs work' again until I can compile a new patch with the above mentioned change to content_storage(). Maybe later, maybe tomorrow...

markus_petrux’s picture

Status: Needs work » Needs review
FileSize
33.41 KB

And here's the updated patch.

It requires the icon provided in #174 that should be copied to cck/images, directory that you should create.

Note the new .js file is moved to cck/js, directory that you should create. Related issue: #421116: May we move the file content.js to js/content.admin.js?

Please, test and let me know if there's anything else that needs to be taken into account here.

coltrane’s picture

There are two ways to delete a field with #202. 1), empty fields are not saved so clearing a field removes it and 2), for unlimited-value fields there is the remove checkbox. CCK users are use to the first way (clearing fields) so if this issue becomes "don't reset deltas" only is the remove checkbox necessary?

The use case I noticed this was by creating two fields on a type. The first set to unlimited values and the second to a limited number (e.g. 2). The node form has remove checkbox/button for the first field but not the second.

I think this is just a bit confusing.

1), '_remove' could be added for all fields with number of values greater than 1. But, this would also require an 'Add another item' button that disappears once the limit is reached. Or, 2), '_remove' could be split off into a patch that implements the database storage of empty field values, if that's still going to happen. Or, 3), leave it as is?

So far I don't see a problem with not storing empty values as implemented in #202, in fact I like it. I haven't tested with mutligroups or my View at #416154: Synchronized deltas Views integration: Filter on equal deltas in multigroups yet.

markus_petrux’s picture

@coltrane: I just tried with a field, setting multiple values to 4, for example, and if I leave 2nd and 4th items blank, then save, when editing again, the positions and order are still there, just like the node was submitted.

When the multiple values for a field is fixed, then a remove button is not necessary, I think. And the latest patch respects the order and positions of the items, while still saving into the database the same exact number of records as before this patch, which is what I believe was yched's concern.

It would be nice if someone else could test the latest patch with as much different kind of fields as possible. For example, text, noderef, filefield, etc.

BTW, the latest multigroup module I posted is broken, as these latest patchs raised a couple of bugs. I have them fixed on my dev environment, I would wait to see how this issue evolves to post more updates to the multigroup.

coltrane’s picture

@markus_petrux You are correct about the positions and order, what I mean is that starting in #200 NULL values are not saved to the database. Deltas continue to not be reset so order stays correct from the UI. Is that not what your patch in #202 implements?

Text and integer fields hold onto delta and the edit form depicts position (empty fields between non-consecutive values).

Nodereference holds onto delta but the form doesn't depict position. Same for a date field (date 2.1). Same for filefield (I used rc1 release cause I'm getting errors with filefield stable release and CCK dev). I patched date awhile back to work with non-consecutive deltas #401326: Support empty values and multigroup but with patch in #202 I don't think it's necessary. Just have to figure out how to depict position.

Thanks Markus!

markus_petrux’s picture

a) CCK without any of the patches on this issue, does not save empty items into the database, except that it saves one value at minimum, empty if there's nothing else. In regards to deltas, always start from 0 to N, up to the number of items to store for each multiple value field.

b) CCK with patches before #202 where storing all the items not explicitly removed by the user into database. We could end up with deltas 0, 1, 2, 3, 4, 5 and 6 where 1, 3 and 5 could be NULL if they were empty in the form. ie. patches before #202 were adding records to database for all empty items. Good to keep deltas in sync, but not so good for SQL joins, etc.

c) CCK with the patch in #202 stores value as a), but keeping the deltas. As opposite to without this patch, you can get deltas 0, 2, 3 and 6 (4 non-consecutive deltas) in the database, while without this patch they were 0 to 3 (4 consecutive deltas). Empty items not stored in the database, just like without this patch.

AFAICT, #202 implements the remove button (original request) and then, in regards to the number of items stored into the database, it works just like without this patch, but keeping the deltas (the order and position in the edit form), which it was necessary for compatibility with the multigroup (see comments #3 to #10 or so, on top of this issue), and also the reason I've been working on this. LOL

You'll only notice that deltas are not consecutive if the field is configured with a fixed number of multiple values. Here you'll see how the edit form is built respecting the order AND positions of the items. However, if the multiple field is unlimited, the initial node edit form is built with only non-empty items, where you cannot see that the deltas are in fact non-consecutive. To see this "in action", you can setup a field with multiples as 10, then edit a form, leave a few non-consecutive items blank, then save, then edit. Then set multiples to unlimited, then edit the node, etc.

yched’s picture

Sorry, didn't get much time since #198.

My point in #189 was that I'd *really* prefer not to commit anything that changes how CCK stores empty values right now. CCK keeps order but not position. delta values per se are meaningless, you don't attach semantical meaning to being value 2 or 4. That's a non-minor point in CCK data model. D7 Field API works like that, and will most probably not change (at least I'm not advocating it, or planning to spend time on it, or recommending someone else does). So we're not going to change that in D6 right now. In a way, CCK is no longer contrib stuff :-)

Now, the current direction of multigroups show a need *for multigroups* to keep deltas synced. Fine, That's a different issue, and it derives from a technical point in the current approach to multigroups (fields in groups are stored as if they were standalone fields). This technical point will probably be moot in D7 (fields in groups will be able to use their own tables and storage). So let's find a way to 'fix' multigroups in D6 without changing the data model. I'm leaning towards having content_set_empty() behave differently if the field is in a group : for 'regular' (non grouped) fields, [0:value_a, 1: empty, 2:value_b] becomes [0:value_a, 1:value_b] as it currently does, for a 'grouped field', it becomes what multigroups need it to become. Maybe it's the right idea, maybe not, and if it is, the exact way to get there can be discussed.

I do think that this should happen in the multigroups patch - or in a separate, self contained patch, suits me fine as well. The main thing in this patch right now is a UI enhancement, which is nice to have with regular fields, and will reveal it's full usability power with multigroups. It carries its own set of subtleties and fine-tuning, as the above shows. Pulling too many things in the same patch makes it harder to review, harder to isolate for a D7 foreport, and is no general good practice.

Markus, I really appreciate the amount and quality and reactivity of the work you're doing here [edit: and in the rest of the CCK queue, for that matter. Be blessed for that fact alone :-)], and I'm sorry that this is probably not what you want to here. I'm hoping I was able to make my point and worries a little clearer.

yched’s picture

As per the patch itself :

- I thought that the after_build callback was needed to transfer the value of '_removed' down to where it can be read from hook_field_validate(), but it seems we're back at altering #post and #value in there ? Sorry if I missed a step. I did like where we were heading in #190 :-)

- #maxlength validation can stay on FAPI IMO. OK, will raise (rare) false positive errors, but I'd say there's only so much we can do,

markus_petrux’s picture

I don't know what to say now. This issue started moving in November, and I tried to follow your suggestions as best as I could. In regards to the "data model"... please, read comments #20 to #26 (assuming non-consecutive deltas), #57 to #65 (where some of this work was supposed to enhance Fields in D7 in regards to non-consecutive deltas), #72 (looking very nice, focus on filefield), etc. It seems to me it was accepted that we were going to have non-consecutive deltas here, which is the only change in the data model as described in #206. With the patch in #202 we are not storing more NULL values than before. It only happens that deltas may be non-consecutive. And this is necessary not only for multigroups, but also consistent for fields with fixed multiple values, visually transparent for unlimited fields, and transparent to field implementations and views integration. There have been a lot issues here, and I feel we've been moving forward, when it now seems we are now back on something that was already discussed. Who would have spent all this time just for a UI enhancement? I have the feeling that we all were dealing against something a bit more complex.

@#208 - Regarding after_build: I tried to post here what lead me to implement it back, and also tried to document it in code comments as well.

I'm interested in multigroups, and not only me I guess, but a lot of sites are probably waiting for such a feature. If multigroups are going to exist in CCK, then what's done here is ahead of that, so it doesn't seem wise to spend time splitting something now that needs to be joined later. Aside, I'm unable to see what can be taken out from this patch. Please, see the whole picture. Of course, there may be other ways to implement it all, there's always more than one way to do it, I just ended up here. Honestly, I believe I can't do more. Now, the ball is in your court, so please take this issue over.

RainbowArray’s picture

I've mostly been a silent observer here: if I knew how to apply patches and such, I could help with testing... something I have to learn.

Anyhow, I have a client project that I began last fall, found about this series of patches early this year, and I've been checking every day since then. I really need the multigroup features for the site, but had to launch the site at the beginning of March. Because multigroup wasn't ready, I am going to have to go back and redo a couple dozen pages (in order to make them easier for the client to edit) once multigroups is available, and I'm going to be doing this on my own time.

I have another client project coming up that's also going to require multigroups, and I really don't want to have to go through the same thing.

My first client wants to be able to edit their site and create new pages, and that's going to be difficult until multigroups is out. When I explain to them the reason for the delay, they have got to be frustrated with Drupal and the process for updating modules. I have switched all my development to Drupal now, but it's getting frustrating to keep telling the client, "Just a few more weeks," and having to keep extend that goal.

I have so much respect for all the work Markus (and rconstantine) have done, and I really appreciate that yched is cracking down to look at this. I just want to emphasize that there are people out here who have real business needs that need to be met posthaste through the implementation of this, nested fieldgroups and multigroups.

I don't want to be somebody just harping on the sidelines, so if somebody could point me in the direction of some resources where I can learn how to apply patches and such, then I can try to at least help with testing.

Once again, thanks for all you're doing.

coltrane’s picture

@mdrummond The Drupal.org Handbooks contain a wealth of information, and a lot of answers are in the forums as well. For applying a patch (such as #202) check out http://drupal.org/patch/apply and for further support http://drupal.org/support. If you have more questions I recommend searching the forums and asking there or hopping on #drupal-support on IRC (see http://drupal.org/irc). Glad to hear you vocalize your need for multigroups, I need the functionality as well!

markus_petrux’s picture

FYI: The patch in #202 contains a bug that I would be happy to update here if necessary.

The bug: required attribute in form element should only be disabled when the widget manages its own multiple values, AND (which is the bug) the field is actually configured for multiple values.

ndeschildre’s picture

Hello there!

I've been following the content_multigroup module evolution for a few months, and after the release of the snapshot included in CCK 2.1 (or earlier?), it seems to be a little stuck, due to some implementation considerations (delta handling with CCK and multigroup, if I am understanding right).

Since we really need that for our upcoming website, I'm going to take another lead. A maybe less "architecturally clean" approach, but something that will work. The basic idea is that I will be storing relation between the CCK deltas and "real" deltas in a separate table, and make the necessary adjustements.

I'll base myself on the content_multigroup module available in the CCK module, and I'm expecting to make a first release next week or so. I'll keep you updated :)

markus_petrux’s picture

@ndeschildre #213: Please, wait a little. I hope CCK maintainers will included these patches in the package sooner than later. If that was not the case, I already mentioned somewhere that we will be running our site with unofficial multigroup requiring the patch in this issue, even if it is not committed to CVS, in which case I would be maintaining an unofficial version of the patch as well. If that ever happens, then I would create a contrib module. So that I believe you would be duplicating efforts here.

I would suggest what we do, which is wait a little to see how this evolves. Meanwhile, you can test the patches and provide feedback so that we all know it works or not for you too.

ndeschildre’s picture

@markus_petrux: Ok, I'll be waiting with you :)

markus_petrux’s picture

To be honest, this is a complex situation. CCK maintainers seem to not have the time for these patches. It seems we're reaching a dead-end.

I have no idea what yched asked me to do with the patch. I have no idea on how it can be splitted without breaking what it does. I have no idea on how to approach any other way od doing what it needs to do, what's ok or what's wrong. And also, I spent so much time investigating here that I cannot spend more. So I'm on a dead-end. The last thing I wish is reach a situation like the one I mentioned on my previous comment, but... what else can we do? :( Forking and/or unofficial packages is not a good solution for anyone. Let's find a way that is confortable enough for everyone. But I don't know how. :(

If someone knows how to proceed from here in order to help CCK maintainers accept the patch in this issue, then please chime in!

yched’s picture

Forking is definitely the worst option, and would be a severe failure (to blame on CCK maintainers). The situation here is indeed complex, and I'm frankly torn on this patch. I have no intention to let it rot, though.

I've started to work on splitting the patch in two, as I mentioned before - but got diverted by D7 stuff again. Will come back to it ASAP. If it turns out it canot be done, I'll humbly apologize and we'll see from there (wouldn't be a great sign about the complexity of the patch and of the current CCK code, though).

rconstantine’s picture

I'm just wondering... how many lines actually change? A quick scan of the last patch shows that the bottom 40% of the code is brand new. Of the top 60%, about 50% is context code that doesn't change. The remaining changes include comment changes. What's left are the actual changes, which don't seem like a lot, volume-wise. Perhaps I'm mistaken?

ccshannon’s picture

subscribe.

Big Up to the remove button. May it smite many a nodereference.

markus_petrux’s picture

@yched #217: Thanks for the feedback. It's much appreciated, really. However, if you have the time, I would also appreciate if you could describe where's the exact problem with non-consecutive deltas, as I believe this is the only difference in how CCK stores data with my patch. I guess this is the only way to support multigroups, also for D7... or is it going to be the same thing as multigroups in Fields in D7 but not using non-consecutive deltas? ...well, let's say fields in multigroups here were stored on a per multigroup table. In that case, the UI would still need to deal with non consecutive deltas. So, really, I have no idea on how else can be implemented this feature. If you could describe the problem, and how it could be approached, then maybe I could do it, if you don't have the time.

Ah, BTW, please, let me know if your wish me to upload my latest version of the patch, as I fixed something as noted in #212 above.

hlykos’s picture

subscribe

leo.ruffini’s picture

Hi, thanks for the big effort you are putting on this.

I tried to apply the patch but got an error message. Don't know if it is my fault or if it is that something is going wrong when applying the patch to the last dev version.

This is what I got:

starlab@skat:/var/www/internal/sites/all/modules/cck$ sudo patch -p0 < cck-196421-202.patch
patching file content.module
Hunk #1 succeeded at 266 (offset 6 lines).
Hunk #2 succeeded at 477 (offset 6 lines).
Hunk #3 succeeded at 505 (offset 6 lines).
Hunk #4 succeeded at 517 (offset 6 lines).
Hunk #5 succeeded at 549 (offset 6 lines).
Hunk #6 succeeded at 706 (offset 6 lines).
Hunk #7 succeeded at 744 (offset 6 lines).
Hunk #8 FAILED at 752.
Hunk #9 succeeded at 798 (offset 6 lines).
Hunk #10 succeeded at 910 (offset 6 lines).
Hunk #11 succeeded at 921 (offset 6 lines).
Hunk #12 succeeded at 951 (offset 6 lines).
Hunk #13 succeeded at 1032 (offset 6 lines).
Hunk #14 succeeded at 1096 (offset 6 lines).
1 out of 14 hunks FAILED -- saving rejects to file content.module.rej
patching file includes/content.node_form.inc
patching file js/content.node_form.js
patching file tests/content.crud.test
patching file theme/content-module.css

eliosh’s picture

sorry, can't find any other way to....
subscribe :-)

RainbowArray’s picture

Any progress on your end, yched? We have a number of people with clients who really need this functionality, so any effort you can put into solving this would be greatly appreciated.

smithn.nc’s picture

Subscribing, as I am interested in the progression here in relation to http://drupal.org/node/119102 (the patch for the unfinished multigroup module.) It would help about three of my projects immeasurably.

eliosh’s picture

Dear CCK mantainers,
please find the time to incorporate and let this patches evolves, because more and more of us would like to use on production sites.

Thanks to everyone who helped with this.

servantleader’s picture

Please don’t let this work go to waste. I very much need this capability for many of the projects I am working on. If this patch is not accepted, I see no other alternative than to create a separate contrib. module. Though development would be slower than it could be, it would be faster than it is going now.

Thanks for all the work that has gone into this.

markus_petrux’s picture

@yched #217:

a) In case you missed it, I posted a note in #220.

b) Another approach: The current patch does not break anything related to CCK2 for Drupal 6, and a lot of work has been done to ensure it works with almost all CCK fields. Many authors have been involved in changing something here and there to ensure their CCK field works with this patch and with the multigroup module (for example, filefield and imagefield). So the only potential problem that would remain are possible issues when migrating from CCK2 to Fields in D7. Yes, this patch introduces non-consecutive deltas in fields, but this does not break anything in CCK2, AFAICT. So, may we work to accept what's being done, and then, when required, focus on the issues that may arise when working on the migration path from CCK2 to Fields in D7?

If you don't have the time to support the multigroup module, then it could be implemented as a separate module, and I would be happy to work on that one. But we need this patch in CCK2, otherwise we would have to keep maintaining an unsupported patch that affects the core of CCK, and that increases the cost significantly for those that need these features. I guess there's a lot of sites in need for this.

Forking is not an option, yes. But, that's what is going to happen for those that need to implement this while it's unofficial. Everyone that needs this, is forking CCK for him/herself. It's the same thing. But then, you're on your own, when CCK gets a new patch somewhere else. You need to apply that manually, unless someone keeps a separate copy of CCK with all unofficial stuff in place. And that's a fork. If we don't want to do that. I don't. Then, what else can we do? We need something that works, but since it is not accepted, we're required to increase the cost of our projects significantly.

May we find a point where everyone is comfortable enough?

Edited: Here's an idea that just came to mind: Maybe we could create a new branch in CCK repository, and that becomes CCK3, that could only exist as a dev release. This patch and the multigroup, and the nested fieldgroups stuff could be worked on that branch. All commits to CCK could be applied to CCK2 and CCK3. Than would make maintenance for those that need these features a lot easier. Also, since Drupal collects project usage stats per release, then we all could be aware of how much these features are used around.

eliosh’s picture

I think that markus analysis of the problem is clean like the fresh water.
My 2€c: i'm using CCK dev of one month ago, but i'm scared to update it, because i don't know if it will work after patching. And the site i'm working on will be alive next Sunday...
So, his idea in #228 is a great idea, IMHO.

I'm not so fluent in english, i've so many ideas i'm thinking about, like problems that could be generated by branching cck, but now i'm unable to write them down.

I'll try via gedit one word at a time.

Thanks.

digi24’s picture

Thanks for this work, I need this module too.

I have applied the patch from #202 and and added the .png. However the remove icons do not show up (my group consists of an imagefield and a textfield).

If I move the

  drupal_add_js(drupal_get_path('module', 'content') .'/js/content.node_form.js');

call eg. into content_init() the icons show up and work as expected.

UPDATE
The content-edit.js was missing. After adding the file everything works.

Trunkhorn’s picture

subscribe

leo.ruffini’s picture

Hi, I really would like to continue testing this, but applying the 202 patch to the last cck dev version keeps giving me errors (see comment 222). Is it because the patch doesn't work with the last dev versions anymore? How can this be solved if we cannot get prior dev versions?
Thanks,
Leo
BTW don't let this rot. It is really useful!

RainbowArray’s picture

I hate to just be a whiner, but we're all waiting on yched here. It would be really nice to get a status update on what's happening. I'm sure there's a number of problems out there that need solving (and that dreadful thing called real life), but we have a whole lot of people out here who are waiting on this that need the combo field functionality for projects for clients.

I've been touting Drupal to lots of people recently, but I have to admit that this is making me lose faith in the whole "bottom-up" approach to Drupal. Some people did some great work in creating this new functionality, and now it's just hanging in the air.

I have one client in particular that I really need this functionality for, and I've been telling him since, oh, February, that this feature would be available in just a few weeks, as it has always seemed this was just on the horizon.

The delay is making me look bad, and quite frankly, it's making Drupal look bad. If someone were to ask me about what I think of Drupal, I'd have to be quite honest in saying that this is an example of where the Drupal development model hasn't worked very well.

I'm hoping this turns around, and I can take it all back.

If there's something we can do to help move this along, please let us know what we can do.

tombigel’s picture

I second with @mdrummond - This issue and http://drupal.org/node/119102 are totally bloated and are impossible to track.
I am too waiting for months for both issues to be resolved, and I was sure it is "weeks away" for 4 months now...

It will be a bliss to have a sum of the progress and problems, best in a new clean issue so people that don't have the time or nerves to read 800+ posts might be able to help.

Tom B.

RainbowArray’s picture

What are our options if yched or others do not have time or desire to continue work on integrating this into cck? Can non-sequential deltas be handled within a contributed multigroups module, or will that break other things?

I'm really getting sick of telling my client that I'll be improving his ability to update his site "once drupal is upgraded" with this functionality. I had to do so again today.

It would be nice to even see some sort of status update from yched. Is this something he can look at in a week? A month? Is there some sort of hang-up? Is there something we can do to help?

I'm also interested to know if multigroup functionality is being built into d7: I definitely hope so, and hopefully with the ability to allow for multivalue fields to be included in a multigroup. I know that's a functionality that seems out of reach perhaps for d6, but it seems like now would be the time to make that work for d7, doing whatever needs to be done with delta handling to make that work.

Tick tock.

yched’s picture

The past few weeks have clearly shown that I have too little time for D6 work.
Markus has accepted to become a CCK co-maintainer, and the multigroup / nested groups patches will be committed in a separate, "experimental" CVS branch, so that the remaining bits can be polished and merged back in the main CCK branch. Meanwhile, all CCK commits and fixes will happen in both branches.

There is currently no work under way for multigroups in D7. There will need to be at some point, though, since we've agreed that nothing can be committed to the official CCK D6 without at least a clear path to D7, to avoid feature regression.

Multivalue fields in a multigroup: won't happen, if you want my personal opinion. This is just not within the CCK data model, or the current approach to multigroups.

rconstantine’s picture

@Yched - I know what it's like for RL to squeeze Drupal time away. Thanks for all you do anyway. And thanks for making Markus a co-maintainer. I agree with him that large patches aren't necessarily bad things in and of themselves. I could understand your trepidation, but really, the bottom line in my opinion is that if the "wheels don't fall off" after the patch goes in, it doesn't really matter how complicated it was. And by now, it's got to be pretty well tested. Hopefully, this will all go in soon and we can close these long threads - and get to my nested fieldgroups patch!!!

So whatever you and Markus (and karens I suppose) work out as co-maintainers, I appreciate your willingness to give the users what they want and to come up with a way to do that.

Has Markus already become a co-maintainer and is the experimental branch created yet? Will that branch show up on the project page, or will it be in the 'view all releases' area only?

markus_petrux’s picture

@Ryan: yep! :)

The basic idea is that yched and KarenS are working to prepare a new stable release of CCK 2, that will become 6.x-2.3. Hopefully that will happen soon. Then, they will create a new "experimental" branch (probably something like Views 6.x-3.x-dev), which is where the monster patches will live. Ideally, both CCK 2.3 and the new "experimental" branch will share the same version of database schema, meaning we will probably introduce the 'parent_group' column in the fieldgroups table (you know what it is ;-)

When this is ready, I'll probably commit this patch (the remove button) and the multigroup. Then, we probably need to brainstorm a little bit more if the current code is fine with D7, etc. And then, the plan is to include the nested fieldgroups patch. So, we'll have to work together until we get something as stable and fine with D7 as possible.

In addition to the monster patches, the experimental branch will be in sync with all the changes that affect CCK 2.x. Even though, switching from one branch to another won't be recommended because even when the field/type tables may not get affected, the field/group settings will for sure, and probably we won't create a path to rollback from the experimental branch to the stable one.

I'm personally excited and grateful for this new scenario, and I'll do my best to help now, and when the time to introduce these features to D7 comes. I avail this opportunity to thank Yves and Karen for all they have done and their confidence in me.

Well ...and they all lived happily ever after. :-D

mki’s picture

Great news! First of all, I would like to thank you very much for all you efforts.

Will you provide upgrade path between every steps in experimental branch, right to the stable branch? In other words: can we use experimental branch on almost-production environment?

I am asking becase this is very long-waiting feature, so I would like to use it and test it as soon as possible.

markus_petrux’s picture

It's hard to tell. But I would say the goal is not use the experimental branch to add everything in the world, but to try to provide a nest where these monster patches may mature, in sync with CCK2, and a reasonable good starting point to head them to D7. Ideally, we should reach a stable experimental branch as soon as possible.

Personally, I think it was an incredible effort to get to what CCK2 is today, and that took a bit more time since D6 was released. The life cycle of Drupal itself has been impacted by the time it took to release a stable version of CCK2 and, of course, Views. As I see it, there was a point where it was really necessary to release CCK for D6, and it was not possible to include the multigroup (and probably many other things that may still live in the issues queue). But now we almost have it, so if we can... then we should go for it. But, sooner or later will be the time to leave new features for Fields in D7 and beyond...

This is my personal point of view.

RainbowArray’s picture

Thank you, thank you, thank you to all involved!

Maybe this is early to ask this, but any rough estimate on what sort of timeframe we might be looking at until the new "monster patch" branch will be forward-compatible enough to use on production sites? Are we talking a few weeks? A couple months?

I'll be glad to learn how to do patches and such if I can be of help in the testing.

Thanks again!

Roulion’s picture

that's a really good news.

I had to use the rconstantine tar ball with multigroup (i only use this with 2 basic text fields) and i hope upgrading to a 6.3-x branch will occur easily...

Thanks everyone for your work...

rconstantine’s picture

@Roulion, I expect that if the database tables remain intact from my tarball to the experimental branch that any implementation code changes will not be visible to the user.

@Markus - Yippee!

@everybody - I haven't tested the changes since my tarball, but I've been using my tarball in production for a few months without problems - at least none I know about. So I expect that the experimental version will be pretty stable. That will be a judgment call for yourself, but I'm sure I'll upgrade (and update the nested fieldgroups patch) ASAP. Although the patches are huge, they have been pretty clean throughout this process. Markus and I have each cleaned up each others' code in places, though admittedly, Markus has done a lot more work than I have altogether. I hope that I won't have to change too much to the nested fieldgroups patch. But either way, I'm glad to see we're moving forward again.

markus_petrux’s picture

Status: Needs review » Fixed

I'm proud to say the code discussed in this issue has been committed to CVS, the 6.x-3.x branch. :-D

And for the moment, I'm marking this issue as fixed. We have yet to discuss a bit more about it, to see if there's anything that needs to be revised, rewritten, etc. We may re-open this issue or create new ones to deal with further changes. I would prefer the later as this issue may reach 300 follow ups easily, and there will be an annoying pagination after that.

There will be a 6.x-3.x-dev package soon, but please, consider it totally experimental. Don't use on production sites until this branch can be considered something stable, which is something I hope it will happen, well, maybe one of these days, heh. Also, please note there will be no backward path to rollback from 6.x-3.x down to 6.x-2.x.

Thank you very much to all those who have helped in one way or another! :-D

rconstantine’s picture

Cool. Thanks Markus. We should open new issues going forward. We should see if we can get an admin to lock this thread just in case.

Status: Fixed » Closed (fixed)

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

petermilad’s picture

Hello,
Could anyone tell me which .patch file I have to apply with what icons (throbber and remove images). how about .js files ?
I'm kinda lost here don't know what files to use.
Thanks

petermilad’s picture

Hello again,

It seems that the patch is out of date!
can anyone please help in this

Regards,
Peter

jghyde’s picture

Version: 6.x-2.x-dev » 6.x-2.9
FileSize
17.53 KB

I rolled #190 against 6.x-2.9 in case anyone is needing it.

rogueturnip’s picture

Has anyone looked at rolling this over to Drupal 7 field api?

owntheweb’s picture

Also looking for this in D7. :)

KarenS’s picture

As noted in #244, this code is already in CCK, so no patch is needed. We don't control the D7 code and this is not the place for issues against D7 core.

jonathan_hunt’s picture

@petermilad FYI, I just applied this to CCK 6.x-2.9 successfully. Use the image from (rename it to remove.png), the javascript from the patch #202, and the updated patch from #249. Note that #202 refers to js/content.node_form.js whereas #249 refers to theme/content-edit.js (I've used the latter).

purabdk’s picture

This patch is nice. It solves the problem of saving data in database with delta value. But edit form does shows the deleted form.

for solving full issue. You need to use following code.
--- trunk/sites/all/modules/cck/content.module (original)
+++ trunk/sites/all/modules/cck/content.module
@@ -796,7 +796,7 @@
'#single' => $single,
'items' => array(),
);
-
+
// Fill-in items.
foreach (array_keys($items) as $weight => $delta) {
$element['items'][$delta] = array(
@@ -848,8 +848,8 @@

// The location of the field's rendered output depends on whether the
// field is in a fieldgroup or not.
- $wrappers = content_get_nested_elements($node->content, $field['field_name']);
- foreach ($wrappers as $wrapper) {
+ $wrappers = content_get_nested_elements($node->content, $field['field_name']);
+ foreach ($wrappers as $wrapper) {
$element = $wrapper['field'];
// '#single' is not set if the field is hidden or inaccessible.
if (isset($element['#single'])) {
@@ -917,7 +917,8 @@
$function = $field['module'] .'_content_is_empty';
foreach ((array) $items as $delta => $item) {
if (empty($item['_remove'])) {
- $filtered[] = ($function($item, $field) ? $empty : $item);
+ // added $delta for array to unabling the multigroup form edit functionality by ssg
+ $filtered[$delta] = ($function($item, $field) ? $empty : $item);
}
}

@@ -980,7 +981,7 @@
}

$type_name = $node->type;
- $type = content_types($type_name);
+ $type = content_types($type_name);

switch ($op) {
case 'load':
@@ -1044,7 +1045,7 @@
return $additions;

case 'insert':
- case 'update':
+ case 'update':
foreach ($type['tables'] as $table) {
$schema = drupal_get_schema($table);
$record = array();
@@ -2407,6 +2408,7 @@
* doesn't get called when the theme overrides the template. Bug in theme layer ?
*/
function template_preprocess_content_field(&$variables) {
+
$element = $variables['element'];
$field = content_fields($element['#field_name'], $element['#node']->type);

@@ -2422,14 +2424,14 @@
$variables['items'][$delta]['view'] = isset($element['items'][$delta]['#children']) ? $element['items'][$delta]['#children'] : '';
}
}
- else {
+ else {
// Multiple values formatter.
// We display the 'all items' output as $items[0], as if it was the
// output of a single valued field.
// Raw values are still exposed for all items.
foreach (element_children($element['items']) as $delta) {
- $variables['items'][$delta] = $element['items'][$delta]['#item'];
- }
+ $variables['items'][$delta] = $element['items'][$delta]['#item'];
+ }
$variables['items'][0]['view'] = $element['items']['#children'];
}

@@ -2438,7 +2440,7 @@

$field_empty = TRUE;

- foreach ($variables['items'] as $delta => $item) {
+ foreach ($variables['items'] as $delta => $item) {
if (!isset($item['view']) || (empty($item['view']) && (string)$item['view'] !== '0')) {
$variables['items'][$delta]['empty'] = TRUE;
}

Modified: trunk/sites/all/modules/cck/modules/content_multigroup/content_multigroup.node_form.inc
==============================================================================
--- trunk/sites/all/modules/cck/modules/content_multigroup/content_multigroup.node_form.inc (original)
+++ trunk/sites/all/modules/cck/modules/content_multigroup/content_multigroup.node_form.inc Tue Apr 3 16:53:38 2012
@@ -28,11 +28,11 @@
}
}
}
-
+
// Quit if there are no field in the form for this group.
if (empty($group_fields)) {
return;
- }
+ }

switch ($group_multiple) {
case 0:
@@ -40,15 +40,26 @@
$max_delta = 0;
break;

- case 1:
+ case 1:
// Compute unique deltas from all deltas used by fields in this multigroup.
$group_deltas = array();
$max_delta = -1;
foreach (array_keys($group_fields) as $field_name) {
- if (!empty($node->$field_name) && is_array($node->$field_name)) {
+ if (!empty($node->$field_name) && is_array($node->$field_name)) {
foreach (array_keys($node->$field_name) as $delta) {
- $group_deltas[$delta] = $delta;
+ // commented following line for unabling the multigroup form edit functionality by ssg
+ // $group_deltas[$delta] = $delta;
}
+ // added following lines for unabling the multigroup form edit functionality by ssg - start here
+ foreach ($node->$field_name as $key => $value) {
+ if($value[value]!=''){
+ $group_deltas1[$key] = $key;
+ echo "Key: $key; Value: $value[value]
\n";
+ }
+ }
+ $group_deltas= $group_deltas1;
+ // added following lines for unabling the multigroup form edit functionality by ssg - end here
+
sort($group_deltas);
$max_delta = max($max_delta, max($group_deltas));
}
@@ -56,7 +67,7 @@
$current_item_count = isset($form_state['item_count'][$group_name]) ? $form_state['item_count'][$group_name] : max(1, count($group_deltas));
while (count($group_deltas) < $current_item_count) {
$max_delta++;
- $group_deltas[] = $max_delta;
+ $group_deltas[] = $max_delta;
}
break;

@@ -267,7 +278,7 @@

// Reset the form '#node' back to its original value.
$form['#node'] = $node;
-
+
return $element;
}

danisha’s picture

none of the patch from above is applying..

I just needed a extra remove button for multiple fields.. can anyone help me with that.. I just wasted my half a day applying these patches.

Please help.

amardhumal’s picture

Issue summary: View changes

After Applying patch #254. I got issue in edit the node. (Some delta value get deleted if I deleted middle delta value)
Then I apply following patch and issue get resolved

---
trunk/sites/all/modules/cck/modules/content_multigroup/content_multigroup.node_form.inc Thu Feb 27 19:56:06 2014 (r1223)
+++
trunk/sites/all/modules/cck/modules/content_multigroup/content_multigroup.node_form.inc Mon Mar 3 13:02:40 2014 (r1224)
@@ -43,23 +43,21 @@
case 1:
// Compute unique deltas from all deltas used by fields in this
multigroup.
$group_deltas = array();
- $max_delta = -1;
+ $max_delta = -1;
foreach (array_keys($group_fields) as $field_name) {
if (!empty($node->$field_name) && is_array($node->$field_name)) {

- foreach (array_keys($node->$field_name) as $delta) {
+ foreach (array_keys($node->$field_name) as $delta) {
// commented following line for unabling the multigroup form
edit functionality by ssg
// $group_deltas[$delta] = $delta;
}
// added following lines for unabling the multigroup form edit
functionality by ssg - start here
- foreach ($node->$field_name as $key => $value) {
- if($value[value]!=''){
- $group_deltas1[$key] = $key;
- }
+ foreach ($node->$field_name as $key => $value) {
+ $group_deltas1[$key] = $key;
}
- $group_deltas= $group_deltas1;
+ $group_deltas= $group_deltas1;
// added following lines for unabling the multigroup form edit
functionality by ssg - end here
-
- sort($group_deltas);
+
+ //sort($group_deltas);
$max_delta = max($max_delta, max($group_deltas));
}
}

satishsaner’s picture

Nice One Amar. Thanks

makbul_khan8’s picture

Source : http://www.intheloftstudios.com/blog/remove-tabledrag-field-drupal-using...
Remove the tabledrag from a field in Drupal using your theme

/**
* Implements hook_preprocess_HOOK().
*/
function THEMENAME_preprocess_field_multiple_value_form(&$vars) {
if (isset($vars['table']['#tabledrag'])) {
unset($vars['table']['#tabledrag']);
array_pop($vars['table']['#header']);
foreach ($vars['table']['#rows'] as &$row) {
foreach (array_keys($row['data']) as $key) {
if (array_intersect($row['data'][$key]['class'], [
'field-multiple-drag',
'delta-order',
])) {
unset($row['data'][$key]);
}
}
}
}
}