infinite recursion with extra fields

This doesn't prevent other things to run twice...
I'm not sure I really like the idea.

Hmmm

I ran into a different version of this problem.

One cycle of the infinite recursion on 7.x-2.6:

713: view->preview() (Array, 2 elements)
712: views_content_views_panes_content_type_render() (Array, 2 elements)
711: ctools_content_render() (Array, 2 elements)
710: ds_render_ctools_field() (Array, 2 elements)
709: ds_get_field_value() (Array, 2 elements)
708: ds_field_attach_view_alter() (Array, 2 elements)
707: drupal_alter() (Array, 2 elements)
706: field_attach_view() (Array, 2 elements)
705: DrupalCommerceEntityController->buildContent() (Array, 2 elements)
704: CommerceProductEntityController->buildContent() (Array, 2 elements)
703: entity_build_content() (Array, 2 elements)
702: DrupalCommerceEntityController->view() (Array, 2 elements)
701: entity_view() (Array, 2 elements)
700: entity_views_plugin_row_entity_view->pre_render() (Array, 2 elements)
699: views_plugin_style->pre_render() (Array, 2 elements)
698: view->render() (Array, 2 elements)
697: views_plugin_display->preview() (Array, 2 elements)
696: view->preview() (Array, 2 elements)

This is a dynamic field called "related products", which is filled with a views_content views display pane.
The "related products" should only show up when the main product is in view mode "full". The related products should then be displayed in view mode "grid item".
The "grid item" view mode does not have the "related products" field in any visible region, but ds_field_attach_view_alter() still calls ds_get_field_value() for every related product, and every related to the related product, etc.
So, the recursion unfolds before DS can determine that it doesn't need to show the dynamic field.

My suggestion would be to only evaluate the fields that are actually displayed on the page.

I think the problem is in ds_get_field_settings().
This function attempts to filter out fields that are not visible in the current view mode.
But it fails to do so if the current view mode contains no dynamic fields at all. In this case, it returns the dynamic fields of the default view mode instead, which is wrong.

Attached is a patch which should fix this.
It also cleans up the clustered ternary operator in the end, but this is not really required to fix this.

Oh, I am wrong.
We need to look into the layout settings, to see if the view mode has a custom layout.. Even if this is not configured with Display suite.

Actually we just need one change.

   // Add Display Suite display fields.
   $fields = ds_get_fields($entity_type);
-  $field_values = ds_get_field_settings($entity_type, $bundle, $layout['view_mode']);
+  $field_values = ds_get_field_settings($entity_type, $bundle, $layout['view_mode'], FALSE);

The rest in the patch is cosmetics.

This changes the behaviour to NOT perform a fallback... That's like a big change, this probably will break a lot of things.

Hmm and what happens when you render the exact same entity twice on the same page

This changes the behaviour to NOT perform a fallback... That's like a big change, this probably will break a lot of things.

What would be a case where the fallback is intended and legitimate?

It does make sense if the entire view mode has no dedicated configuration, and uses the default configuration instead.
In this case, ds_get_layout() will already return the default layout.
And then, ds_get_field_settings($entity_type, $bundle, $layout['view_mode']) will already get the field settings for the default layout.
But, if ds_get_layout() returns a dedicated layout for the given view mode, we also need the dedicated field settings, even if this is empty.

This said, I am not sure if this is really the same issue as reported by djdevin.
Maybe we should continue in #2653034: View mode without fields inherits from default view mode..

This changes the behaviour to NOT perform a fallback... That's like a big change, this probably will break a lot of things.

On second thought:
I am convinced that the current behavior is wrong. And likely has measurable performance implications on existing sites.

However, there might be code out there that relies on this kind of misbehavior.

What can we do? An opt-in fix? A major version bump? Or just have people fix their code?

We are still using the patch from #8.

I still think the logic of my previous comments is correct.

For each view mode, one of the following cases applies:

1. It is already the "default" view mode. Boring.
2. The view mode does NOT have custom configuration.
   -> use settings from "default" for everything.
3. The view mode DOES have custom configuration (layout, fields), and it DOES have entries in "ds_field_settings".
4. The view mode DOES have custom configuration (layout, fields), but it does NOT have entries in "ds_field_settings".
   This is the problematic case.

The "ds_field_settings" contains settings for display suite custom fields, and nothing else.
This means, the last case occurs any time a view mode does not use ds custom fields.

In this case, ds will load the custom field settings for "default" view mode, even though the current view mode has custom configuration for other things like layout and regular fields.
This usually has no visible effects. The ds custom field settings for "default" are loaded and processed, but not displayed. This means silent waste of resources, but nothing else.
However it can cause a recursion, depending what custom fields are configured for default view mode.

With the patch from #8, ds will not load the custom field settings from "default".
So, would this cause any problems?
Possibly for case 2 above?

2. The view mode does NOT have custom configuration.
-> use settings from "default" for everything.

In this case, $layout['view_mode'] in ds_field_attach_view_alter() will already be 'default', because ds_get_layout() already applies the default.

So I do not expect any negative effect or BC break.

(uppercase for emphasis, no shouting intended)

Btw a workaround is to clear all custom field settings from default view mode, and activate the "full" view mode instead.
On this project I had not done this because I was not aware that commerce_product has a "full" view mode. It is labeled "Admin display". We are using it in combination with commerce_product_page.

So one can avoid this kind of problem.
But this does not change that the current behavior is incorrect.

A brief label for this problem would be:

ds custom field settings from "default" leak into other view modes

Btw a workaround is to clear all custom field settings from default view mode, and activate the "full" view mode instead.

On the site I am currently working on, we have a number of paragraphs and taxonomy_term bundles with ds custom field settings in "default" view mode. I think this is quite common, and would be a lot of effort to avoid.
So we will continue to use the patch from #8.