Been willing to do that for ages, made possible by the patch in #2211723: FieldInstance::__construct() loads all field_config entities.
Problem/Motivation
Currently FieldInstanceConfig pulls its FieldConfig from either :
- the config storage (or rather the EntityManager FieldStorageDefinition registry after #2211723: FieldInstance::__construct() loads all field_config entities)
- the "state" list of deleted fields, to cater for the case of field purge.
Consequences :
- This makes it impossible to build a working FieldInstanceConfig for an arbitrary runtime FieldConfig object that doesn't actually exist in config. Annoying restriction for the UI creation flow, and for a couple other edge cases.
- It sucks to have FieldInstanceConfig be aware of the "state() store of deleted fields".
- More generically, we know injection is good :-)
Proposed resolution
Allow optionally injecting a 'field' => FieldConfig $field
in place of 'entity_type' + 'field_name' string to FieldInstanceConfig::__construct($values) (and thus FieldInstanceConfig::create() / entity_create('field_instance_config')).
In other words, you can specify the FieldConfig of an instance :
- either by passing the arbitrary FieldConfig object directly
- or by refering to it by its entity_type + field_name (like in existing entity_create('field_instance_config') calls, or when loading from a config record), in which case it is understood as "the official field definition currently existing in config and cached in EntityManager's registry".
Benefits :
- Easier DX when programmatically creating fields and instances (including lots of tests) - see code sample below.
- Instead of currently having FieldInstanceConfig know how to pull deleted fields, the knowledge about "the state() store of deleted fields" is moved to FieldInstanceConfigStorage::loadByProperties(), which already knows about 'the state() store of deleted instances". When building a runtime "instance of a deleted field" (typically during field purge), the method injects the "deleted field".
User interface changes
None, but should help with #1963340: Change field UI so that adding a field is a separate task.
API changes
None, just an API addition :
$field_foo = entity_create('field_config', array(...));
entity_create('field_instance_config', array(
'field_name' => 'field_foo',
'entity_type' => 'node',
'bundle' => 'article',
);
can alternatively be written :
$field_foo = entity_create('field_config', array(...));
entity_create('field_instance_config', array(
'field' => $field,
'bundle' => 'article',
);
Patch coming up after #2211723: FieldInstance::__construct() loads all field_config entities lands.
Comment | File | Size | Author |
---|---|---|---|
#6 | interdiff.txt | 6.53 KB | yched |
#5 | FIC_inject_field-2277941-5.patch | 42.35 KB | yched |
#5 | FIC_inject_field-2277941-5-to_review.do_not_test.patch | 9.11 KB | yched |
#3 | FIC_inject_field-2277941-3-to_review.do_not_test.patch | 9.11 KB | yched |
#2 | FIC_inject_field-2277941-2.patch | 47.68 KB | yched |
Comments
Comment #1
yched CreditAttribution: yched commentedComment #2
yched CreditAttribution: yched commented#2211723: FieldInstance::__construct() loads all field_config entities got in, so here's a patch.
Touches a lot of test files, but the relevant changes are just :
- FieldInstanceConfig :
Allows injecting a FieldConfigInterface object in $values['field'] in the __construct() (and thus in entity_create('field_instance_config'))
If not present (e.g when instantiating from a field.instance.* config record), the getField() method only knows how to fetch [entity_type, field_name] from EM's registry.
- FieldInstanceConfigStorage :
loadByProperties() builds FieldInstanceConfig objects for deleted fields by reading the FieldConfig objects from state() and injecting them in the FieldInstanceConfig it creates
(as an aside, replaces silly code like $this->entityManager->getStorage($this->entityTypeId) by, er... $this :-p - and does the same in FieldConfigStorage)
+ Updates a bunch of existing tests to use the simpler :
when possible, i.e when the FieldConfig is present in an available variable - I did not refactor the tests that currently do create('field_config')->save() directly without storing it in a variable.
Comment #3
yched CreditAttribution: yched commentedActually, posting the "relevant changes" part in a separate patch for easier review :-)
Comment #5
yched CreditAttribution: yched commented- Fixed a couple errors in the test changes
- Reverted Drupal\field\Tests\FieldInstanceCrudTest & Drupal\field\Tests\FormTest, for the kind of stuff they do, defining their $instances with an 'entity_type' + 'field_name' is actually easier.
The actual (non-test) code changes are the same as in #3, uploading both patches for clarity.
Comment #6
yched CreditAttribution: yched commentedForgot the interdiff
Comment #7
swentel CreditAttribution: swentel commentedOoh, yes. This could also help us in #1963340: Change field UI so that adding a field is a separate task big time.
Sweet cleanup, can't see anything that bothers me, so RTBC from me.
Comment #8
yched CreditAttribution: yched commented@swentel : right, forgot that in the issue summary. Only being able to construct a FieldInstanceConfig if its FieldConfig already exists in config was a big and long standing limitation for the "create new field / instance" UI flow.
Restructured the issue summary a bit, and mentioned the Field UI benefits.
Comment #9
yched CreditAttribution: yched commentedNext step when that one is in : #2282627: Remove field_uuid from field instance config records
Comment #10
alexpottComment #12
tim.plunkettAnyone creating a field instance in a test needs to account for the
'field_name' => $name
to'field' => $field
switch.This needs a change notice (it came up in IRC today)
Comment #13
yched CreditAttribution: yched commented@tim.plunkett: you don't *need* to, the "old" syntax with a field_name stays valid.
Comment #14
tim.plunkettHmm. @RoySegall had some issues with creating a field instance manually during a test. I think it was FieldInstanceConfig::calculateDependencies, couldn't load $bundle_entity or call getConfigDependencyName() on it.
Switching to 'field' from 'field_name' fixed the problem.
Comment #15
yched CreditAttribution: yched commentedOdd. Then it's a bug on its own that would deserve its own issue.
Could you close that one back ? (my phone browser won't let me)
Comment #16
tim.plunkettFair enough. Thanks @yched!