Contrib.social

Missing $display check in entity_extra_field_form_alter (edge case)

Open on Drupal.org →
Created on 30 November 2023, 12 months ago
Updated 14 August 2024, 3 months ago

Problem/Motivation

The following error is emitted when trying to open the Product add form with Commerce installed and no Stores configured:

TypeError: entity_extra_field_display(): Argument #4 ($display) must be of type Drupal\Core\Entity\Display\EntityDisplayInterface, null given, called in /var/www/html/web/modules/contrib/entity_extra_field/entity_extra_field.module on line 78 in entity_extra_field_display() (line 111 of modules/contrib/entity_extra_field/entity_extra_field.module).

The problem here is that while$form_object->getFormDisplay($form_state); is expected to return EntityFormDisplayInterface, there's an edge case with Commerce; the entity form may not be fully initialized if there are not Stores configured. Instead, it just displays the warning message (and doesn't call parent buildForm where the form display initialization should be called from).

From \Drupal\commerce_product\Form\ProductForm:

  public function buildForm(array $form, FormStateInterface $form_state) {
    // Skip building the form if there are no available stores.
    $store_query = $this->entityTypeManager->getStorage('commerce_store')->getQuery()->accessCheck(TRUE);
    if ($store_query->count()->execute() == 0) {
      $link = Link::createFromRoute('Add a new store.', 'entity.commerce_store.add_page');
      $form['warning'] = [
        '#markup' => $this->t("Products can't be created until a store has been added. @link", ['@link' => $link->toString()]),
      ];
      return $form;
    }

    return parent::buildForm($form, $form_state);
  }

As a workaround, the Store should be created before opening the form.

Steps to reproduce

1) Install entity_extra_field and commerce.
2) Do NOT create Store.
3) Open /product/add/default.

Expected: the page opens, there's a warning message instead of the form.
Actual: error message / WSOD.

Proposed resolution

As a workaround for Commerce forms behavior, check if $display is not null in entity_extra_field_form_alter.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

🐛 Bug report
Status

Needs work

Version

2.1

Component

Code

Created by

🇺🇦Ukraine abramm Lutsk

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @abramm
  • Merge request !10Issue #3405137: Missing $display check in entity_extra_field_form_alter (edge case) → (Open) created by abramm
  • Issue was unassigned.
  • Status changed to Needs review 12 months ago
  • Comment 12 months ago →
    🇺🇦Ukraine abramm Lutsk

    Here's the MR fixing this issue (and patch if anyone wants to apply it).

  • Comment 8 months ago →
    🇩🇪Germany luenemann Südbaden, Germany

    It's also possible to just change the Signature of entity_extra_field_display to allow for null. The method can handle that.

    function entity_extra_field_display(
  string $type,
  array &$build,
  EntityInterface $entity,
  ?EntityDisplayInterface $display
): void {
  • First commit to issue fork.
  • Status changed to Needs work 3 months ago
  • Comment 3 months ago →
    🇩🇪Germany Anybody Porta Westfalica

    @luenemann where is your code from #4 from?

    The current code in 2.0.x is:

    /**
 * Entity extra field display.
 *
 * @param string $type
 *   The display type, (e.g. view, form).
 * @param array $build
 *   An array of elements to attach the extra field.
 * @param \Drupal\Core\Entity\EntityInterface $entity
 *   An entity instance.
 * @param \Drupal\Core\Entity\Display\EntityDisplayInterface $display
 *   An entity display instance.
 *
 * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException
 * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
 * @throws \Drupal\Component\Plugin\Exception\PluginException
 */
function entity_extra_field_display(
  string $type,
  array &$build,
  EntityInterface $entity,
  EntityDisplayInterface $display
): void {

    So maybe that should be changed instead?

  • Merge request !24Draft: Make ?EntityDisplayInterface $display optional → (Open) created by Anybody
  • Comment 3 months ago →
    🇩🇪Germany Grevil
  • Comment 3 months ago →
    🇩🇪Germany Grevil

    2.1.x is the latest branch. Let's target the MR on 2.1.x. Also, which MR is the preferable? Let's hide the other one.

  • Comment 3 months ago →
    🇩🇪Germany luenemann Südbaden, Germany

    @Anybody the code in #4 is my suggested change. Thank you for turning it into a MR.

    @grevil I am not sure which change is preferable.

    By looking at this again I thinks MR !24 won't work. If there are extra_fields for that entity bundle it will probably fail.

    I am going to hide MR !24.

    Still Needs work for target change on MR !10 to 2.1.x (@abramm)

  • Comment 3 months ago →
    🇩🇪Germany luenemann Südbaden, Germany

    luenemann → changed the visibility of the branch 3405137-missing-display-check to hidden.

  • Comment 3 months ago →
    🇩🇪Germany luenemann Südbaden, Germany

    luenemann → changed the visibility of the branch 3405137-missing-display-check to active.

  • Comment 3 months ago →
    🇩🇪Germany luenemann Südbaden, Germany

    luenemann → changed the visibility of the branch 3405137-make-parameter-optional to hidden.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024