Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#8 | 3177545-8.patch | 1.29 KB | alexpott |
#8 | 4-8-interdiff.txt | 883 bytes | alexpott |
Comments
Comment #2
alexpottComment #3
alexpottComment #4
andypostI 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?
Comment #5
alexpott@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.
Comment #6
andypostFiled child issue for that #3177548-2: Prepare strict type for FieldStorageConfig::setCardinality()
Comment #8
alexpottFixing #4 - at this point
$definition['cardinality']
has to be set.Comment #9
andypostCurious why it can't be 0 (zero) in getter
Comment #10
alexpottA field with a cardinality makes no sense.
Comment #11
amateescu CreditAttribution: amateescu as a volunteer commentedThe patch from #8 looks good to me :)
Comment #12
andypostThe same code lives in
\Drupal\Core\Field\BaseFieldDefinition::getCardinality()
but returns 1 by defaultProbably it missing test coverage and should be updated too
Tests using
\Drupal\Core\Field\BaseFieldDefinition::createFromFieldStorageDefinition()
so NULL is passed only for base fieldsComment #13
alexpott@andypost you have an interesting definition of "same". The code in HEAD in \Drupal\Core\Field\BaseFieldDefinition::getCardinality
vs
\Drupal\field\Entity\FieldStorageConfig::getCardinality()
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.
Comment #14
andypostI 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
Comment #17
catchCommitted 4cd7df6 and pushed to 9.2.x. Thanks!