Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Status: Active » Needs review
FileSize
607 bytes
Dave Reid’s picture

Peacog’s picture

This works for me, after reverting the patch from #2336985: ctools_entity_field_content_type_render() unnecessarily alters field deltas. Thanks.

MacMladen’s picture

I noticed the problem when using Display suite and trying to create a dynamic field that will be skipping some values.

When skipping value is greater than zero, it would actually skip one more, so:

  • value 0 does not skip anything — expected
  • value 1 skips two instead of one
  • value 2 skips three instead of two
  • ...etc

While those related issues suggested removing TRUE from array_slice, this one solved the issue for my case (using image multiple value field and creating dynamic one from that one).

This issue is actually a critical one and I would check Reviewed and tested by community, however, I'm not sure how to test it beyond what I already did.

@Dave: if there is anything I can do to help/test, please say so and lets check other issues if this is a solution to the related issues.

joelpittet’s picture

Issue tags: +Needs tests

@MacMladen you could write a simpletest to prove the delta values are incorrect and that this patch fixes it.

MacMladen’s picture

Digging more into it, some other problems surface.

First of all both Panels and Display suite suffer from the same problem and that is Ctools simply because they rely on its service for dealing with fields with multiple values.

Next, I was wrong in #4, it actually skips double the number, so for a skip value of 1 it skips two, for skip value of 2 it skips four, etc.

But even that is not the whole story as there are options to skip the first (to offset), then to display at most and then to display in reverse order.

The main suspects in this case are fields, ctools and panels/display suite. Being dependent on ctools I suggest that both panels and display suite should be cleared of guilt so it is either fields or ctools.

I've made testing with everything standard and then applying Dave patch with following outcome:

Possible
Std Drupal/Panel Dave patch
Skip 3 skip 6 instead of 3
Use 2
Reverse not
Skip 3 Use 2
Skip 3 Reverse skip last 3, not reverse
Use 2 Reverse
Skip 3 Use 2 Reverse

It seems to me that this patch solves the issue.

However, while changing from array_merge to + (array union without renumber) I was not sure about the second line ksort as I removed that and still my 7 cases were rendered the same.

For it is only the question if it is bug in field as suggested or it is a bug in ctools that just do not handle well what it gets from field.

Can anyone elaborate on this? Dave?

dev.patrick’s picture

Status: Needs review » Reviewed & tested by the community

The supplied patch#1 seems to resolving the issue. Its the problem with multi-valued field where only single value being rendered leaving others empty.

As @MacMladen concluded that problem lies with Ctools only. Here https://www.drupal.org/node/2456327

stefan.r’s picture

Assigned: Unassigned » David_Rothstein

Assigning to David for further review

Fabianx’s picture

Assigned: David_Rothstein » Unassigned
Priority: Normal » Major
Status: Reviewed & tested by the community » Needs work

I am happy to consider that a major bug as it seems to be breaking things.

If one can create a scenario where this breaks with core alone, we could also consider to make it critical.

But this really needs tests, because + is also not without its problems especially if there are just numeric keys and we could end up with the opposite problem under certain circumstances.

dev.patrick’s picture

Were anyone able to replicate the issue? Issue patch#1 is working in my case.

bibliophileaxe’s picture

Will this patch be applied to the core module?

AnaSwin’s picture

+1 for the patch, I have an issue with deltas and multi value field collection in panel and this patch work for me!
I hope that patch will be applied to the next core version :)
Thanks!

phuvm389’s picture

We have the same issue with multivalued field using panelizer and this patch works perfect for us!

In our case, there are 6 items in the field. We tried several test cases with/without Dave's patch.

Here are the results:
drupal_multiple_value_field

Dave's patch solves all cases, We hope that patch will be merged to the core and released on next reversion.