So this could be a "works as designed" situation, but I think solving this would be an improvement to the way that DS Extras finds field templates/functions. I think there this is the same issue mentioned at #1265920-24: The theme's field.tpl.php should be used unless field display settings exist when "Default" option is selected.

I'm using DS Extras to specify a default field template and I'm choosing "Minimal". So all my Display Suite fields are going to use the Minimal field template. For one specific field, field_event_dates, I choose "Drupal Default" as the field template in the Manage Display UI.

When ds_extras_preprocess_field() gets called, I would think that the theme_hook_suggestions should end up like:

field__date
field__field_event_dates
field__event
field__field_event_dates__event

These are the "Drupal default" suggestions before ds_extras_preprocess_field() is called.

Instead, this field ends up switching to the minimal field templates, as specified in the ft-default variable (via the DS Extras configuration UI).

I think the correct way that ds_extras_preprocess_field() should handle this situation is to just leave the theme_hook_suggestions alone since I am specifying "Drupal default" as the field template rather than specifying "The configured DS Extras default field template". To put it another way, if you select a default field template (Minimal, Full Reset), there is no other way of getting back to the Drupal default field template for a specific field.

CommentFileSizeAuthor
#1 ds-wrong-hook_theme_suggestions-1885480-1.patch1.2 KBjonmcl

Comments

jonmcl’s picture

jonmcl commented
StatusFileSize
new ds-wrong-hook_theme_suggestions-1885480-1.patch1.2 KB

A rather simple patch attached that possibly solves this issue. Feedback needed and welcome.

swentel’s picture

swentel commented
Status: Active » Needs review

Hmm this makes very much sense indeed, setting to needs review so I can let it test next week.

jyve’s picture

jyve commented
Status: Needs review » Reviewed & tested by the community

Patch tested, and makes perfect sense!

swentel’s picture

swentel commented
Version: 7.x-2.x-dev » 8.x-2.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

committed and pushed on 7.x-2.x and 7.x-1.x, thanks!
Moving to 8.x-2.x.

swentel’s picture

swentel commented
Status: Patch (to be ported) » Closed (fixed)

And committed and pushed to 8.x-2.x branch as well.

donquixote’s picture

donquixote commented 
+  elseif (!isset($config['func']) || empty($config['func'])) {

It is enough to just check if (empty($config['func'])). empty() implies !isset(), and will not break on undefined variables.

donquixote’s picture

donquixote commented

Also, if we test empty() there, we should also test empty() in the first condition, like so:

  // Determine the field template. In case it's something different
  // than theme_field, we'll add that function as a suggestion.
-  if (isset($config['func']) && $config['func'] != 'theme_field') {
+  if (!empty($config['func']) && $config['func'] != 'theme_field') {

If $config['func'] has any value that PHP considers empty, we don't want to build functions with it. Right?
Or we only need isset() in both cases.

Furthermore:
The existence of the first if () tells us that the only relevant case to exclude down in this elseif () block is where $config['func'] === 'theme_field'.

donquixote’s picture

donquixote commented

And a different problem:

In the second block with $field_instance_info['ds_extras_field_template'], we also don't want to overwrite $config['func'] === 'theme_field', do we? So the applied patch / commit is incomplete.

Fixing all this also allows to simplify the code a lot.
Maybe I will open a new issue for this.

donquixote’s picture

donquixote commented

And in the second block (condition broken to multiple lines):

  // Check if we have a default field template on instance level.
  elseif (isset($field_instance_info['ds_extras_field_template'])
    && !empty($field_instance_info['ds_extras_field_template'])
    && $field_instance_info['ds_extras_field_template'] != 'theme_field'
  ) {
    [..]
  }
  // Use Display Suite Extras Default theming function.
  elseif (!isset($config['func']) || empty($config['func'])) {

In the case of $field_instance_info['ds_extras_field_template'] === 'theme_field', it should not fall through to the next condition, but rather just stop altogether.

donquixote’s picture

donquixote commented
Related issues: +#2767615: Refactor and fix ds_extras_preprocess_field()

Follow-up: #2767615: Refactor and fix ds_extras_preprocess_field()