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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Issue summary: View changes
yched’s picture

Status: Active » Needs review
FileSize
47.68 KB

#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 :

entity_create('field_instance_config', array(
  'field' => $field,
  'bundle' => 'article',
);

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.

yched’s picture

Actually, posting the "relevant changes" part in a separate patch for easier review :-)

Status: Needs review » Needs work

The last submitted patch, 2: FIC_inject_field-2277941-2.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
9.11 KB
42.35 KB

- 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.

yched’s picture

FileSize
6.53 KB

Forgot the interdiff

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Ooh, 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.

yched’s picture

Issue summary: View changes

@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.

yched’s picture

alexpott’s picture

Title: Allow injecting an arbitrary FieldConfig when buidling a FieldInstanceConfig » Allow injecting an arbitrary FieldConfig when building a FieldInstanceConfig
Status: Reviewed & tested by the community » Fixed

  • Commit 22fc66d on 8.x by alexpott:
    Issue #2277941 by yched: Allow injecting an arbitrary FieldConfig when...
tim.plunkett’s picture

Priority: Normal » Critical
Status: Fixed » Active
Issue tags: +Missing change record

Anyone 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)

yched’s picture

@tim.plunkett: you don't *need* to, the "old" syntax with a field_name stays valid.

tim.plunkett’s picture

Hmm. @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.

yched’s picture

Odd. 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)

tim.plunkett’s picture

Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Missing change record

Fair enough. Thanks @yched!

Status: Fixed » Closed (fixed)

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