Problem/Motivation
\Drupal\Core\Field\RequiredFieldStorageDefinitionInterface and its only method ::isStorageRequired() have been introduced in
#2390495: Support marking field storage definitions as required →
.
BaseFieldDefinition implements that interface and method and also provides its own setter method ::setStorageRequired().
The method implementation \Drupal\Core\Field\BaseFieldDefinition::isStorageRequired() however falls back to \Drupal\Core\Field\BaseFieldDefinition::isRequired() if the setter has not been used.
This means that defining a field like this:
$field = BaseFieldDefinition::create('string')
->setRequired(TRUE);
will result automatically into a field, whose storage is required and the following will return TRUE even without it being explicitly set:
$field->isStorageRequired();
However ::isStorageRequired() is documented like this:
* If a field storage is required, NOT NULL constraints will be added
* automatically for the required properties of a field type.
Core is not doing that yet automatically, but there is an issue to implement that, which is in an almost finished state -
#2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field →
.
So now we have 2 methods for whether a field is required - isRequired and isStorageRequired, where the first is used for form inputs and validation purposes, while the second is used to flag the storage of the field in a shared table as required - for example by adding the NOT NULL constraint to the schema as described in the method's documentation.
Please note that we even already have an implementation in core, which is checking whether a field is storage required and if so a NOT NULL constraint is being added. From \Drupal\comment\CommentStorageSchema::getSharedTableFieldSchema():
case 'entity_type':
case 'field_name':
assert($storage_definition instanceof RequiredFieldStorageDefinitionInterface);
if ($storage_definition->isStorageRequired()) {
// The 'entity_type' and 'field_name' are required so they also need
// to be marked as NOT NULL.
$schema['fields'][$field_name]['not null'] = TRUE;
}
break;
Unfortunately there are 4 problems with ::isStorageRequired() falling back to ::isRequired():
- If we clone a required base field and return it as non-required from an implementation of
hook_entity_bundle_field_info() for a specific bundle, then we'll still not be able to save an entity for that bundle without providing a value for the overridden field as the storage might already be created with a NOT NULL constraint for that field.
- It is impossible to make the base field non-required after it was installed even if we haven't explicitly requested the
NOT NULL constraint.
- We cannot yet properly update
\Drupal\Core\Field\BaseFieldDefinition::createFromFieldStorageDefinition() and let it inherit the storage_required flag, because it is wrong to set the flag if it wasn't set on the passed field storage definition, but TRUE is returned because of the fallback to ::isRequired().
- This issue is also a blocker for
#3014760: [PP-2] Create an abstract composite field definition/storage definition and update BaseFieldDefinition to use it →
.
The proposal there is that the field storage definition becomes a property on the base field definition in order to achieve a clean separation between a field storage and a field definition similar to configurable fields. As such it is not possible for the storage definition to call its parent - the field definition from within ::isStorageRequired() and access ::isRequired().
As the use cases and meaning of a required and storage required are different a check for storage required should not fall back to required.
It is worth mentioning that regarding bundle fields only FieldConfig is aware of the required property and FieldStorageConfig does not know anything about it.
Only FieldConfigInterface extends the FieldDefinitionInterface where ::isRequired() is defined. The FieldStorageDefinitionInterface does not extend FieldDefinitionInterface.
This does not match BaseFieldDefinition where everything belongs together, but only on bundle fields we have a clear separation between what is a field definition setting and what is a field storage setting.
Currently there is no workaround for allowing a field on a specific bundle to not be storage required, if a field has gotten a NOT NULL constraint as documented just because it has been simply flagged as required but not as storage required. Core might not be adding the NOT NULL constraint automatically yet, but has documented that it should be added if the field is storage required and doing that in custom or contrib code is therefore perfectly valid and according to the core's documentation. Considering the 4 different problems this is not to be confused with simply being a documentation issue.
Steps to reproduce
The test from the attached patch in
#2
🐛
BaseFieldDefinition::isStorageRequired wrongly falls back to ::isRequired
Needs work
- \Drupal\KernelTests\Core\Entity\EntityFieldTest::testRequiredChangeOnBundleOverride() is showing an example of how core behaves with and without automatic NOT NULL constraint based on the output of ::isStorageRequired().
There we create a required base field and two bundles, where for the second bundle we return an altered base field definition by flagging the field as non-required.
This represents a perfectly valid use case as we might want something to be optional on one bundle, but required on another.
Then we create and save an entity for the bundle where the field is not required.
First we do that with current behaviour where NOT NULL constraints are not yet added automatically based on the output of ::isStorageRequired(). We assert that the entity is saved properly.
Then we recreate the entity tables by using a different storage schema handler, which adds the NOT NULL constraint to storage required fields. As one can see from the test results we cannot anymore save an entity for the bundle without providing a value for a field which isn't required on that bundle, but is required on the first bundle.
Proposed resolution
As the use cases and meaning of a required and storage required are different a check for storage required should not fall back to required.
Therefore we need to remove the fallback to ::isRequired() from within ::isStorageRequired().
If we want a field storage to be required, then we should explicitly request that by calling ::setStorageRequired(TRUE).
Remaining tasks
Review.
User interface changes
None.
API changes
BaseFieldDefinition::isStorageRequired() does not fall back to isRequired anymore.
If we want a field storage to be required, then we should explicitly request that by calling ::setStorageRequired(TRUE).
Data model changes
None.
Release notes snippet