PHP7.4 and empty entity_keys for taxonomy term result in notices

@arx-e Have you got any solution for this issue?

@arx-e

Though I have not done a detailed testing, however issue was resolved once changes were made directly to common.inc file. I wish if this can be committed into the main branch of Drupal core.

Hey @arx-e.
> It seems that whenever a taxonomy term is rendered in a custom view mode it comes with empty entity_keys
The same thing for me too.

Thanks for the patch. The code looks good to me.
I've tested on Drupal 7.78 under PHP 7.4.10, the patch applied without problems.

Let's push it further!

Hi!

Just tested the patch in #7 against 7.80 and it applied OK.

Fixed my problem the patch #10, thanks for your work

Hi,

Patch #10 works fine. Could you please merge this to Drupal 7.80.

Thanks for the patch.

However, I'd prefer not to see the ternaries wrapped in more ternaries.

How about something simple like this patch?

Also, this should ideally have tests. They could be very basic.

RTBC + 1

Prefer patch 14 after comparing with the MR.

Thanks

Confirming latest patch in #14 still applies cleanly and resolves the issue on PHP 7.4.
I'm not sure of the status of the MR. Are all the commits showing here just rebases from 7.x?

I'm not sure of the status of the MR. Are all the commits showing here just rebases from 7.x?

...yes, looks like it.

I'd still like to see some (very basic) tests for this.

Patch at #14 works for me (albeit the line numbers have changed now).

Acquia is updating all hosted D7 sites to PHP 7.4 on October 4, 2021. Any chance this could get committed before that happens? I'd love to not have a core patch!

I updated the MR with the latest Drupal core updates and tested them.

Confirming latest MR in #25 applies cleanly and resolves the issue on PHP 7.4.

+1 from me for RTBC.

While I was experimenting with a possible test to this issue I found out that the problem is a little bit different as it is presented in the issue summary.

I was able to simulate the notice (somehow playing with display_suite settings) and it was not caused by empty entity keys, but the whole empty $info variable. See longer decription below:

------------

The if condition in the issue summary does not only cover this one possibility, it could also be a case where the whole $info variable is empty:

if (!empty($info['entity keys'])) {
  ....
} else {
  ....
}

This code will reach the else block if entity keys are empty OR if the $info variable is empty. The variable will be empty everytime you call entity_get_info() with a name of non-existing entity.

I was not able to simulate the problem with empty entity keys and while digging into it, it seems like that the function entity_get_info() always set the content of entity keys:

      $entity_info = module_invoke_all('entity_info');
      // Merge in default values.
      foreach ($entity_info as $name => $data) {
        $entity_info[$name] += array(
          'fieldable' => FALSE,
          'controller class' => 'DrupalDefaultEntityController',
          'static cache' => TRUE,
          'field cache' => TRUE,
          'load hook' => $name . '_load',
          'bundles' => array(),
          'view modes' => array(),
          'entity keys' => array(),
          'translation' => array(),
        );
        $entity_info[$name]['entity keys'] += array(
          'revision' => '',
          'bundle' => '',
        );
        ...

So even in case that the returned entity has empty entity keys, this function will set at least the array with empty revision and empty bundle. Only way how this could be empty would be if the drupal_alter() called afterwards will unset it and that is unlikely to happen...

// Let other modules alter the entity info.
drupal_alter('entity_info', $entity_info);

-----------

So my testing revealed that the display suite is (in some circumstances) calling the entity_get_info() with the $entity_type = 'term'. This will cause the $info variable to be NULL, because the correct entity type for taxonomy terms is 'taxonomy_term'.

    // Move any preprocess fields to render container.
    // Inconsistency in taxonomy term naming.
    $entity_type = $vars['elements']['#entity_type'];
    if ($vars['elements']['#entity_type'] == 'taxonomy_term') {
      $entity_type = 'term';
    }

    // Get entity id and bundle
    $id = NULL;
    $bundle = $vars['elements']['#bundle'];
    $entity = isset($vars[$entity_type]) ? $vars[$entity_type] : (isset($vars['elements']['#' . $entity_type]) ? $vars['elements']['#' . $entity_type] : NULL);
    list($id,, $bundle) = entity_extract_ids($entity_type, $entity);

There is a clear Display Suite bug - they are changing the $entity_type variable for the entity retrieval from the $vars array, but also passing the changed variable to the entity_extract_ids() without setting it back.

------------

But anyway, I think it would be good to sanitize this also on the Drupal core side, because this situation is not handled correctly and the NULL value is used as an expected array later (without check what entity_get_info() returned). This can cause issues in PHP 7.4+.

function entity_extract_ids($entity_type, $entity) {
  $info = entity_get_info($entity_type);
  // Objects being created might not have id/vid yet.
  $id = isset($entity->{$info['entity keys']['id']}) ? $entity->{$info['entity keys']['id']} : NULL;

So if we check the $info variable, it will create a better code (from my point of view). See my patch proposal. What do you think?

I think this looks good, thank you!

Is there an issue filed for the problem in Display Suite described in #28?

RTBC again, let's get this in!

@mcdruid Yes, it seems like it is: #3196015: Degradation: Re-apply the fix for entity_extract_ids in ds_entity_variables

Thank you everybody!