Problem/Motivation

\Drupal\field\Entity\FieldStorageConfig::getCardinality() does $enforced_cardinality < 1 without checking types. PHP 8.0 handles integer comparison a bit differently. This causes a failing test.

Steps to reproduce

See https://www.drupal.org/pift-ci-job/1855374 and https://3v4l.org/KV2LI

Proposed resolution

Cast to int

Remaining tasks

None

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
949 bytes
alexpott’s picture

andypost’s picture

I think better to type-hint early (cos it used more then once in changed line)

Looks configuration schema can't prevent getting weird data as cardinality (sensitive definition) maybe throw warning better here?

alexpott’s picture

@andypost config schema can prevent this but it can't prevent \Drupal\Core\Field\BaseFieldDefinition::setCardinality() being called with random stuff... once we add a primitive typehint to that then this code can go away.

andypost’s picture

Status: Needs review » Needs work

The last submitted patch, 4: 3177545-4.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
883 bytes
1.29 KB

Fixing #4 - at this point $definition['cardinality'] has to be set.

andypost’s picture

Curious why it can't be 0 (zero) in getter

alexpott’s picture

A field with a cardinality makes no sense.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

The patch from #8 looks good to me :)

andypost’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/field/src/Entity/FieldStorageConfig.php
@@ -632,11 +632,11 @@ public function getCardinality() {
-    $enforced_cardinality = isset($definition['cardinality']) ? $definition['cardinality'] : NULL;
+    $enforced_cardinality = isset($definition['cardinality']) ? (int) $definition['cardinality'] : NULL;

The same code lives in \Drupal\Core\Field\BaseFieldDefinition::getCardinality() but returns 1 by default
Probably it missing test coverage and should be updated too

Tests using \Drupal\Core\Field\BaseFieldDefinition::createFromFieldStorageDefinition() so NULL is passed only for base fields

alexpott’s picture

@andypost you have an interesting definition of "same". The code in HEAD in \Drupal\Core\Field\BaseFieldDefinition::getCardinality

  public function getCardinality() {
    // @todo: Allow to control this.
    return isset($this->definition['cardinality']) ? $this->definition['cardinality'] : 1;
  }

vs

\Drupal\field\Entity\FieldStorageConfig::getCardinality()

  /**
   * {@inheritdoc}
   */
  public function getCardinality() {
    /** @var \Drupal\Core\Field\FieldTypePluginManager $field_type_manager */
    $field_type_manager = \Drupal::service('plugin.manager.field.field_type');
    $definition = $field_type_manager->getDefinition($this->getType());
    $enforced_cardinality = isset($definition['cardinality']) ? $definition['cardinality'] : NULL;

    // Enforced cardinality is a positive integer or -1.
    if ($enforced_cardinality !== NULL && $enforced_cardinality < 1 && $enforced_cardinality !== FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED) {
      throw new FieldException("Invalid enforced cardinality '$enforced_cardinality'. Allowed values: a positive integer or -1.");
    }

    return $enforced_cardinality ?: $this->cardinality;
  }

They are different implementations of the same interface. They are not the same code. Base fields don't have to deal with enforce cardinality from field types. There is nothing to update.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

I mean that default value is null in configurable fields but 1 for base fields, will check it again and will file follow up but surely it should not block this fix

  • catch committed 4cd7df6 on 9.2.x
    Issue #3177545 by alexpott, andypost: \Drupal\field\Entity\...

  • catch committed 8b72ea4 on 9.1.x
    Issue #3177545 by alexpott, andypost: \Drupal\field\Entity\...
catch’s picture

Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 4cd7df6 and pushed to 9.2.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.