Create a new ConfigEntity type for storing bundle-specific customizations of base fields

Thanks @eff, great summary.

In order to store stuff like c_t's "translation_sync", both the new FieldDefinitionOverride and the existing FieldInstanceConfig will need to support entry for "3rd party settings", with roughly the same constraints and API needs than what is being done in #2005434: Let 3rd party modules store extra configuration in EntityDisplay :

- internally keyed by module name so that we can have config schemas (and cleanup on 3rd party module uninstall)

- with dedicated API getter and setter next to getSettings() / setSettings()

I guess this "extensibility by 3rd party settings" feature would be supported by the base class shared by FieldDefinitionOverride and FieldInstanceConfig ?

Also, in the hangout with @plach @fago @xjm @alexpott last saturday, it was agreed that FieldDefinitionOverride config would not show up as edit forms in Field UI for base fields - not fully convinced we won't want to go there at some point, but that's the current hypothesis.

I am not sure we have decided whether those "3rd party settings" would be shown as form snippets in the existing edit forms for FieldInstanceConfig. In which case we also need to add a hook to generate those form snippets (again, as #2005434: Let 3rd party modules store extra configuration in EntityDisplay is currently doing)

I think we want the extending modules to be able to provide forms too. Eg. currently field translatability settings are displayed on the field form too. (I'm not sure the triggering translation_sync one is, that is for fields that have component values some of which may be synched).

Here is a very basic start with a base class and a FieldDefinitionOverride class. Not entirely sure how to untangle the maze of interfaces as they apply to these, I did not componentize the interfaces (yet?).

Looking at how #2005434: Let 3rd party modules store extra configuration in EntityDisplay implements third party settings, it is based on PluginSettingsInterface/PluginSettingsBase which participate in widget/entity display settings. This does not participate in field settings that I can see. So don't know if it would be viable to refactor based on that vs. writing our own similar API.

@Gabor : yeah, PluginSettingsInterface/PluginSettingsBase was added in the first steps of "convert Field API pluggable types to Plugins, starting with Widgets and Formatters", as a way to mutualize the handling of settings and their default values.

It was not leveraged for "Field type" plugins (FieldItem clases), because those have a split notions of "field-level settings" and "instance-level settings".

I might be wrong, but I'm not sure there's lots to gain by mutualizing the methods that handle "3rd party settings". Could be worth a dedicated interface though ?

class FieldInstanceConfig extends FieldConfigBase

+++ b/core/modules/field/src/Entity/FieldConfigBase.php
@@ -0,0 +1,84 @@
+class FieldConfigBase extends ConfigEntityBase {

+++ b/core/modules/field/src/Entity/FieldInstanceConfig.php
@@ -34,46 +33,7 @@
+class FieldInstanceConfig extends FieldConfigBase implements FieldInstanceConfigInterface {

So, sure, from the POV of Core/Field/FieldDefinition[Interface], FieldConfigBase is the right name.

But then for configurable fields we have
- FieldConfig
- FieldInstanceConfig
- it's the latter that extends FieldConfigBase, not the former :-)

Not saying we should name that new base class differently, just that it raises the nomenclature WTF...

Just to stop sounding like a broken record, I went ahead and opened #2287727: Rename FieldConfig to FieldStorageConfig :-). Will need a core committer green light for D8...

All righto then. Here is a changeset with an initial API formed after the API in #2005434: Let 3rd party modules store extra configuration in EntityDisplay. Note the methods are not "thirdParty..." because this entity is *designed* to store third party settings only. I assume we'll proxy this object from the field instance where we'll need "thirdParty" method equivalents to most of these and create the object when at least one option is set and delete it if !hasSettings() (which is the reason I introduced that method).

Also added initial config schema coverage for the settings structure with possibility for the modules to provide their relevant schema snippets by module name.

Feedback please if you have time :)

7: 2283977-7.patch queued for re-testing.

With proxy code now for CRUD of the field definition overrides from field instances. I think we want to make these a trait and share it with whatever API is used to manage the base fields, no? :) Not sure how best to do that, but this definitely needs feedback at this point.

I don't think FieldInstanceConfig can even be identical to FieldDefinitionOverride? It was not clear to me if you envisioned FieldDefinitionOverride to be a kind of instance config object for base fields (so default value, label, etc. could be overridden for them? But also a third party settings container for both base fields and configurable fields? However then the FieldDefinitionOverride base properties would not apply to configurable fields, only the 3rd party settings?

I think both FieldInstanceConfig and FieldDefinitionOverride need to support third party settings but we should leave that to #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration. After all modules like field permissions and field validation need somewhere to store their stuff. The third party system should be generic but for configurable fields it should store it's settings in field.instance.* and for base fields it should store them in field.override.*.

Also I think the base field override should be in core/Drupal/lib/Field

Let's start over from #4 then. Having the field_uuid on this does not seem to make sense then because AFAIS base fields don't have such a value. I so far have a hard time seeing what FieldDefinition properties this should contain that overlap with FieldInstanceConfig. For one they store their values entirely differently, eg. FieldDefinition has $definition['label'] while FieldInstanceConfig has $label. So not sure how much of an overlap there is between the two that we want to rely on.

Also looked at where merging of the two data sets would happen, EntityManager::buildBaseFieldDefinitions() has a colorful set of sources already with the original class, hook_entity_base_field_info() and hook_entity_base_field_info_alter(). I *think* we would merge in values somehow there after the existing sources but it is not clear to me what would provide the management of these objects for base fields and ensure their presence when there are overrides and their disappearance when there are none.

Also moving the FieldConfigBase and FieldDefinitionOverride to where @alexpott suggested. I thought config entities are to be provided by modules and not from lib, but he says this will be fine. Since I don't have code to exercise this because I don't understand what to even put into them (see #14), it will most likely be green either way and not proving anything.

Helps to use the right case.

I'm currently working on this.

Here's a first cut at a patch... lots to do. But I've moved label overriding from node types to the field definition overrides successfully.

I have to pop out now but I will come back to the issue later to add notes on things - just posting now in case of the proverbial bus.

1. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -312,3 +312,45 @@ condition.plugin:
   +core.base_field_override.*.*.*:
   +  type: field_config_base
   +  label: 'Field override'
   
   +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -107,6 +107,8 @@ public function installDefaultConfig($type, $name) {
   +    // Core can provide configuration.
   +    $enabled_extensions[] = 'core';
   

   So why is it so important to go all through these hoops to define this vs. just providing from field module? Is the use case of no customizable fields at all on the site but using base fields overrides is a viable thing? If there are no configurable fields, I would assume people coded their base fields the way they wanted and not going to override them?

   Looks to me lots of hoops for a theoretical use case.

2. +++ b/core/lib/Drupal/Core/Field/Entity/FieldDefinitionOverride.php
   @@ -0,0 +1,197 @@
   +   * In most cases, Field instance entities are created via
   +   * entity_create('field_instance_config', $values), where $values is the same
   +   * parameter as in this constructor.
   ...
   +   *   An array of field instance properties, keyed by property name. The field
   +   *   this is an instance of can be specified either with:
   ...
   +    // Allow either an injected FieldConfig object, or a field_name and
   +    // entity_type.
   

   Not a field instance / FieldConfig though?

3. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -0,0 +1,432 @@
   +  /**
   +   * The UUID of the field attached to the bundle by this instance.
   +   *
   +   * @var string
   +   */
   +  public $field_uuid;
   

   Base fields don't have UUID though? Will override have a 'field UUID'?

4. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -0,0 +1,432 @@
   +  /**
   +   * Flag indicating whether the bundle name can be renamed or not.
   +   *
   +   * @var bool
   +   */
   +  protected $bundle_rename_allowed = FALSE;
   

   How is this possible on a field definition override? Some other properties are similarly confusing to me as to why/how they apply to overrides.

5. You removed 'Title' as the default value of the title. Is this now coming from the base field definition? It is not evident from the patch.

Regarding #22.3 / public $field_uuid :
Just a note that #2282627: Remove field_uuid from field instance config records removes it from FieldInstanceConfig - and is RTBC ;-)

So discussed this a bit with @plach in IRC he suggested some renames of things. The patch attached adds:

• BundleFieldDefinitionInterface An interface combining FieldDefinitionInterface and ConfigEntityInterface which adds setters.
• FieldDefinitionOverride A new configuration entity will the same schema as FieldInstanceConfig to store base field overrides.
• FieldConfigBase A base class for common functionality for FieldInstanceConfig and FieldDefinitionOverride.

We cannot much progress on removing Node::bundleFieldDefinitions until FieldDefinitionInterface::getDefaultValue does not require an entity since we need to get the default values for status, promote and sticky on the NodeTypeForm.

Other than that I think this patch is ready for some architectural / naming review. The patch works and is used to change the title field label so testing is as simple as going to the node type edit from and changing the title label.

This patch also makes it possible to change the title description :) So changing 'The title of this node, always treated as non-markup plain text.' for forum posts to 'The subject of this forum post, always treated as non-markup plain text.'is now very simple.

I'm not a huge fan of FieldConfigBase as a name especially since FieldConfig does not extend it - this directly relates to #2287727: Rename FieldConfig to FieldStorageConfig

I'm going to continue on an change content_translation to use the bundle field definitions instead of hook_entity_bundle_field_info_alter

1. @timplunkett wants core to be able to provide config entities too - we're not alone and this will happen :)
2. Fixed
3. Fixed - field_uuid was not part of field_config_base config schema type anyway :)
4. Moved back to FieldInstanceConfig - but this brings up a good point we need to support bundle renaming.
5. Yes it was always set on the base field definition

Re: $bundle_rename_allowed
This was added just to allow field.module's hook_entity_bundle_rename() to update the 'bundle' stored in field.instance.* config records by doing a save() - which otherwise forbids changing the 'bundle' of an existing FieldInstanceConfig.

That's something I never really agreed with : changing the value of the 'bundle' stored in a config record in reaction to the bundle name being renamed is strictly internal housekeeping, and shouldn't result in an API save with hooks firing IMO. The FieldInstance is not being changed, we just keep it in line with some external changes. In D7 we made a direct UPDATE request in the {field_instance_config} table.

The last patch broke hard because of a renamed property... $originalFieldDefinition => $baseFieldDefinition... fixed and converted content_translation to use bundle field definitions :) it works!

Patch generation fail :(

Yes, I hit the Core.* thing while working on a prototype of #2292733: [meta] Introduce Display objects, let's spin that out to #2294177: Allow Drupal\Core to provide [config] entity types

@effulgentsia's review

1. Tim's created an issue - I'm not a huge fan since it's hard to test without actually have a use in core
2. Yeah naming is hard :)
3. I think it should be the full bundle field definition - keeps things simple
4. Well this should be inline with the naming
5. Any better ideas? A field definition object is not bundle aware. We need to make it bundle aware, override the values and save it.

@alexpott

A field definition object is not bundle aware. We need to make it bundle aware, override the values and save it.

I don't get this either. A FieldDefinition *is* bundle aware, it's FieldStorageDefinitions that are not. So I don't get why we'd need FieldDefinitionInterface::getBundleFieldDefinition($bundle) ?

How is a field definition's for base fields bundle aware?

+++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
@@ -76,6 +76,15 @@ public function getName();
   /**
+   * Gets the bundle the field is defined for.
+   *
+   * @return string|null
+   *   The bundle the field is defined for, or NULL if it is a base field; i.e.,
+   *   it is not bundle-specific.
+   */
+  public function getBundle();

+++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php
@@ -297,4 +297,11 @@ public function getProvider();
    */

#2144263: Decouple entity field storage from configurable fields adds this method to FieldDefinitionInterface.

EntityManager::buildBundleFieldDefinitions() attaches field definition objects to bundles - but all bundles share the same object instance unless something clones and overrides.

Right, sorry. Fields exposed in EntityType::baseFieldDefinitions() and not explicitly overrriden in bundleFieldDefinitions() or (with the patch) in a ConfigEntity, are not tied to a bundle - and god I hate this. That's what #2289391: Reconsider whether FieldDefinition should implement FieldStorageDefinitionInterface is about.

Trying to articulate thoughts about the direction this is taking, adding the notion of BundleFieldDefinitionInterface to the set of *Definitions we already have, but running out of time atm.
Sorry for the FUD, but I'm frankly worried about the complexity of the system we're ending up with, I'm having a hard time figuring it out myself :-(

1. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -329,3 +329,45 @@ display_variant.plugin:
   +field_config_base:
   

   As discussed in IRC, I'd rename this to bundle_field_definition_base.

2. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -329,3 +329,45 @@ display_variant.plugin:
   +core.base_field_override.*.*.*:
   

   Shouldn't this be core.bundle_field_definition.*.*.*?

3. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinitionInterface.php
   @@ -0,0 +1,42 @@
   + * Defines an interface for overriding field definitions.
   

   Not sure I'd explicitly mention overriding here. This will be implemented also by FieldInstanceConfig.

4. +++ b/core/lib/Drupal/Core/Field/Entity/BundleFieldDefinition.php
   @@ -0,0 +1,202 @@
   +use Drupal\field\FieldException;
   

   Mmh, we need a core version of this (or just move this to \Drupal\Core\Field)

5. +++ b/core/lib/Drupal/Core/Field/Entity/BundleFieldDefinition.php
   @@ -0,0 +1,202 @@
   +  /**
   +   * The original field definition.
   +   *
   +   * @var \Drupal\Core\Field\FieldDefinition
   +   */
   +  protected $baseFieldDefinition;
   ...
   +    return $this->baseFieldDefinition;
   

   This should be available through $this->getFieldStorageDefinition(), why do we need another class member?

6. +++ b/core/lib/Drupal/Core/Field/Entity/BundleFieldDefinition.php
   @@ -0,0 +1,202 @@
   +        throw new FieldException('Attempt to create an field override on a configurable field.');
   

   "an field"? Is that a typo or is it correct? I thought the "an" form was used only with initial vowels...

7. +++ b/core/lib/Drupal/Core/Field/Entity/BundleFieldDefinition.php
   @@ -0,0 +1,202 @@
   +    if (!isset($this->baseFieldDefinition)) {
   +      $fields = \Drupal::entityManager()->getBaseFieldDefinitions($this->entity_type);
   +      $this->baseFieldDefinition = $fields[$this->getName()];
   +    }
   +    return $this->baseFieldDefinition;
   

   As above this could use $this->getFieldStorageDefinition(). An explicit method wrapping it looks good to me though.

8. +++ b/core/lib/Drupal/Core/Field/Entity/BundleFieldDefinition.php
   @@ -0,0 +1,202 @@
   +  public function setBaseFieldDefinition(FieldDefinition $field_definition) {
   +    $this->baseFieldDefinition = $field_definition;
   +    return $this;
   +  }
   

   Why do we need this? Again, cannot we rely on $this->getFieldStorageDefinition()?

9. +++ b/core/lib/Drupal/Core/Field/Entity/BundleFieldDefinition.php
   @@ -0,0 +1,202 @@
   +    $properties[] = 'baseFieldDefinition';
   

   Won't this be a problem when we restore it? It will no longer be a reference to the original wrapped base field definition. Can't it just be null and retrieved again through $this->getFieldStorageDefinition()?

10. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
    @@ -0,0 +1,426 @@
    +abstract class FieldConfigBase extends ConfigEntityBase implements BundleFieldDefinitionInterface {
    

    As discussed in IRC I'd rename FieldConfigBase to BundleFieldDefinitionBase.

11. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
    @@ -0,0 +1,426 @@
    +  public $field_name;
    

    Here and below, shouldn't we be using camel case for class members?

12. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
    @@ -0,0 +1,426 @@
    +  public function targetBundle() {
    

    Maybe not in this issue but we should really rename this to getTargetBundleId() for consistency.

13. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
    @@ -0,0 +1,426 @@
    +    // Manage dependencies.
    +    $bundle_entity_type_id = \Drupal::entityManager()->getDefinition($this->entity_type)->getBundleEntityType();
    +    if ($bundle_entity_type_id != 'bundle') {
    +      // If the target entity type uses entities to manage its bundles then
    +      // depend on the bundle entity.
    +      $bundle_entity = \Drupal::entityManager()->getStorage($bundle_entity_type_id)->load($this->bundle);
    +      $this->addDependency('entity', $bundle_entity->getConfigDependencyName());
    +    }
    +    return $this->dependencies;
    

    Maybe I'm not reading it right, but doesn't this code belong to the parent class?

14. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
    @@ -0,0 +1,426 @@
    +    \Drupal::entityManager()->clearCachedFieldDefinitions();
    

    Here and below: we can use $this->entityManager().

15. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
    @@ -0,0 +1,426 @@
    +
    

    Bogus blank line

16. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -600,4 +601,12 @@ public function getFieldStorageDefinition() {
    +  public function getBundleFieldDefinition($bundle) {
    +    $override = BundleFieldDefinition::loadByName($this->getTargetEntityTypeId(), $bundle, $this->getName());
    +    if (!$override) {
    +      $override = BundleFieldDefinition::createFromFieldDataDefinition($this, $bundle);
    +    }
    +    return $override;
    +  }
    +
     }
    

    I am not sure about this: if we need to explicitly instantiate bundle field definitions (and ideally from a DX pov I'd really wish we hadn't to), I'd put this method on the EntityManager. Two reasons:

    • The EM is our main field definition factory, so centralizing the code there makes sense.
    • If I want to instantiate a bundle field definition for a third field flavor not implemented in core (say a bunny field), having to instantiate it through FieldDefinition would not look right.

17. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -305,20 +304,15 @@ function content_translation_form_language_content_settings_submit(array $form,
    +          // If the field definition is a base field get the bundle field
    +          // definition for it.
    +          if ($definition instanceof \Drupal\Core\Field\FieldDefinition) {
    +            $definition = $definition->getBundleFieldDefinition($bundle);
    +          }
    

    This is exactly the kind of DX wtf I'd like to avoid:

    • either I would always ask for a bundle field definition to the EM, which in turn would figure out internally whether it already instantiated a bundle field definition for that field
    • or I would like EMI::getFieldDefinitions() to always return bundle field definitions (although I realize this might have a cost in terms of memory usage)

18. +++ b/core/modules/forum/config/install/core.base_field_override.node.forum.title.yml
    @@ -0,0 +1,17 @@
    +label: Subject
    

    I'd tend to agree with @eff: is defining the whole config file strictly required? Would it be so hard to provide just the overrides, at least in the default config?

@yched:

Trying to articulate thoughts about the direction this is taking, adding the notion of BundleFieldDefinitionInterface to the set of *Definitions we already have, but running out of time atm.
Sorry for the FUD, but I'm frankly worried about the complexity of the system we're ending up with, I'm having a hard time figuring it out myself :-(

IMHO we are not adding new concepts, we are just giving explicit names to concepts that are already existing implicitly in our current system. Maybe this is not making things easier, but it's definitely improving consistency.

1. Fixed
2. Hmmm... this is the configuration object name - I kinda like having the work override in here.
3. It does in the paragraph below - which I've tidied up and added some @see's
4. Well core using this exception was not introduced by this patch. Can we move this to a followup?
5. The original base field definition is not the Field storage storage definition. We need it so that we can connect all the dots - plus this is not available publicly so what's the issue?
6. Fixed
7. No it couldn't - in order to load a base field override using entity_load() and get the field storage definition we need to get the original base field definition. We need to be able to do this to instantiate these entities for config import and install.
8. This is just a helper method when we're creating these things from an override or loading them when we create all the field definitions - although perhaps we should not do that as this means we'll being caching duplicates of the base field objects... hmmm.
9. Yes indeed. So I've removed the sleep and setter. But we still need the getter for things like BundleFieldDefinition::getDisplayOptions() since this comes from FDI and not FSDI and it not stored in the config entity.
10. Fixed
11. This comes from FieldInstanceConfig and is used in the config file - I vote for leaving as is.
12. Yep not this issue - followup material.
13. No - this in from FieldInstanceConfig and puts dependencies into the config entity files for the bundle.
14. Fixed
15. Fixed
16. Will address later
17. Will address with 16 later
18. Yes the whole file is necessary - partial entities are not just a headache they are wrong.

The interdiff is a pseudo interdiff due to the reroll.

However, with the patch as-is, for
any bundle where the label is customized, then every other aspect of the
title field gets disconnected from what is code-defined, so future code
changes don't carry over to it.

Yep, that's an issue IMO. Just because one value is overriden in config means that all other values also get "masked" and disconnected from the code definition.

In practice, we'll want to use overrides for what currently lies in Node::bundleFieldDefinitions() - i.e by-node-type overrides of title, status, promote, sticky. Meaning, de-facto all node types will be disconnected from their code definition from the get go, and never get back. Thus, any update in the code field definition on any property will never be actually reflected without an update function.

I too thought we were going for shallow overrides ?

So sounds like the shallow override problem is a good reason to return back to where I started this issue to have a generic override array container and only include keys that are necessary vs. a full on entity with properties where you need to specify all of the properties.

But here's the thing - if we did

change Node::baseFieldDefinitions() to change the description, required, or default value of the title field

We'd be putting quite an onus on existing sites to be retested / retranslated. I think changing any of these would be madness.

Also moving away from the common base between (what are currently know as) field instances and the overrides means that we have yet another thing for people to learn. In my mind there is a certain elegance to the current solution that is completely lost if we do shallow overrides.

Thus, any update in the code field definition on any property will never be actually reflected without an update function.

I think this is fine as a limitation. We already have that issue for default configuration. This is only being discussed because base fields aren't defined in default configuration - but that makes them the exception.

Rerolled - and a few cleanups.

I have no strong opinion about this: the DX is a bit concerning but if there are architectural reasons behind this choice, let's go on with it. Anyway, I think @yched should have the final word on this.

About RTBC, I didn't look at the code yet, but per alex's comment I think we are still missing a couple of items from my review.

Yeah we need to address points 16 and 17 from #36.

Finally addressed 16,17 from @plach's review in #35.

I agree this it is a nicer dx just to have a getBundleFieldDefinition() method on the entity manager. We can't make getFieldDefinitions() always return saveable bundle definitions since this will totally break @Berdir's elegant caching in getFieldDefinitions() with the way it combines the entity_bundle_field_definitions and entity_base_field_definitions cache objects for each entity type.

Patch needed a simple reroll so the interdiff is a pseudo interdiff again.

Looks great to me! I guess now we definitely need at least @yched to have a look to this.

Just a couple of minor notes:

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -914,4 +918,18 @@ public function loadEntityByUuid($entity_type_id, $uuid) {
   +  public function getBundleFieldDefinition($entity_type, $bundle, $field_name) {
   +    $definitions = $this->getFieldDefinitions($entity_type, $bundle);
   +    if (!isset($definitions[$field_name])) {
   +      throw new FieldException(String::format('Field %name or entity type %type does not exist', array('%name' => $field_name, '%type' => $entity_type)));
   +    }
   +    if ($definitions[$field_name] instanceof BundleFieldDefinitionInterface) {
   +      return $definitions[$field_name];
   +    }
   +    return BundleFieldDefinition::createFromFieldDataDefinition($definitions[$field_name], $bundle);
   +  }
   

   Yay!

   Perhaps we could rename ::createFromFieldDataDefinition() to ::createFromFieldDefinitionData? Unless I am missing something that would sound more understandable to my ears.

   Also: should we update the EM field definition static cache?

2. +++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
   @@ -374,4 +374,27 @@ public function getFormModeOptions($entity_type_id, $include_disabled = FALSE);
   +   * @param string $field_name
   +   *   The name of the field to get the bundle definition of.
   +   *
   +   *
   

   Surplus empty line

@alexpott:

The original base field definition is not the Field storage storage definition.

Why not? Unless something changes in #2289391: Reconsider whether FieldDefinition should implement FieldStorageDefinitionInterface, a base field definition is a field storage definition. Or would they be referencing different object instances?

The original base field definition is not the Field storage storage definition

was meant to be... "The original base field definition is not JUST the Field storage storage definition" we can't use getFieldStorageDefinition because we are relying on method on the FieldDefinitionInterface not FieldStorageDefinitionInterface.

Let's go with createFromFieldDefinition since this corresponds nicely to FieldDefinition::createFromFieldStorageDefinition

Will try to get back to this soon.

Meanwhile, posted some related thoughts in #2289391-7: Reconsider whether FieldDefinition should implement FieldStorageDefinitionInterface.

#1498662: Refactor comment entity properties to multilingual added:

+    $settings = content_translation_get_config('comment', 'comment_article', 'fields');
+    $this->assertFalse(isset($settings['comment_body']), 'Configurable fields are not saved to content_translation.settings.');
+    $this->assertTrue(isset($settings['subject']), 'Base fields are saved to content_translation.settings.');

This patch makes those test assertions obsolete.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -903,4 +918,18 @@ public function loadEntityByUuid($entity_type_id, $uuid) {
+    return BundleFieldDefinition::createFromFieldDefinition($definitions[$field_name], $bundle);

I am still wondering whether we should update the EM static cache so that subsequent calls of getFieldDefinitions() return the bundle field definition.
(I think so :)

re #54 - I don't think this is needed - it looks kinda ugly https://gist.github.com/alexpott/644c1146af8afac8bd76 plus it would be different from FieldInstanceConfig. Actually what we do is share the postSave - which sorts this out for us.

  /**
   * {@inheritdoc}
   */
  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
    // Clear the cache.
    $this->entityManager()->clearCachedFieldDefinitions();

    // Invalidate the render cache for all affected entities.
    $entity_type = $this->getFieldStorageDefinition()->getTargetEntityTypeId();
    if ($this->entityManager()->hasController($entity_type, 'view_builder')) {
      $this->entityManager()->getViewBuilder($entity_type)->resetCache();
    }
  }

In BundleFieldDefinitionBase

Rerolled cause #2294177: Allow Drupal\Core to provide [config] entity types landed.

Looks great! could be useful for comment field to change "required" flag for subject base field

Removed some uses and updated summary.

Warning: I didn't read most of the issue and only partially followed some discussions.

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -465,8 +468,20 @@ protected function buildBundleFieldDefinitions($entity_type_id, $bundle, array $
   +      // @todo possible performance optimisation to do a loadMulitple() here
   +      //   outside of the foreach.
   +      $bundle_field_definition = $this->getStorage('bundle_field_definition')->load($entity_type->id() . '.' . $bundle . '.' . $definition->getName());
   

   for configurable fields/instances, we use an entity query with a STARTS_WITH condition on the id. That's not optimized yet, but there's an issue to do that.

2. +++ b/core/lib/Drupal/Core/Field/Entity/BundleFieldDefinition.php
   @@ -0,0 +1,179 @@
   + *   id = "bundle_field_definition",
   + *   label = @Translation("Field definition override"),
   + *   config_prefix = "base_field_override",
   

   So far, I've only seen config_prefix used to get rid of an unnecessary module prefix. This is a pretty different name, which seems a bit weird?

Overall, this looks pretty nice. Two things I'm worried about, a) the naming, which has been discussed at length already. b) The amount of config entities that we might end up. This just converts the title label but I assume we'd want to update sticky/promote/status too, so we'd end up with 4 additional config entities per node type, just to control some default values and the label.

Oh, and as soon as you make them translatable, you end up with more overrides. Ownership is also interesting, the entity type is owned by core, but multiple modules might mess with the same override. And while that's kind of clear in an alter hook, it's less obvious if you load, change and save a config entity.

I agree that completely overriding makes sense, otherwise we have to load the storage definition + bundle/field definition + the overrides right now. However, we at least need a documented strategy on how to update base fields. Yes, you might not want overridden values to be replaced but the problem is that we don't know which values where. Maybe the module author adds an additional setting or make it required, but that change will not affect any site that stored an override just to change the label. And we don't know what was overidden and what not.

Rerolled and convert the node workflow options.

#59:

1. The problem is that the entity query system depends on entity manager and this is the entity manager - we could inject entity.query.config is people feel that is correct.
2. Happy for suggests and given the number of renames around Entity Field API I'd rather build consensus before committing this patch.

I tried to convert Comment::bundleFieldDefinitions() but this was extremely problematic since the setting being altered here is on the storage level and not the instance level.

Fixed some of the tests.

We have a problem with the boolean default value schema

field.boolean.value:
  type: sequence
  label: 'Default value'
  sequence:
    - type: mapping
      label: 'Default'
      mapping:
        value:
          type: integer
          label: 'Value'

Configurable fields are always ready for multiple cardinality whereas these base fields have a cardinality of 1 and the default value is a bool. Not sure what to do here - ideas anyone?

Well of course we can do this.

Re #65: That's a convoluted fix IMO. Plus, we still hope to be able to have base fields with multiple values at some point, then they can specify default values as multiple too.

setDefaultValue() is inherently about multiple values. It accepts specifying a default value as either a litteral or an array of default values keyed by delta - mostly because it just passes the result to $entity->field->setValue(), that is flexible / sloppy as to what it accepts.

That does provide nicer DX when defining a base field in code, but that's only a shorthand. Internally, and especially if those things can now be put in CMI, we should always store the same shape - an array of multiple values keyed by delta.
That is, if setDefaultValue() receives $litteral, it should translate it to array(array([first property for the field type] => $litteral), which is basically what FiieldItemList->setValue() does.

Other than that, I'm slowly going through the patch :-)

Also : the fix in #65 would force every new/contrib/cutsom field type to provide two different schema entries for "default_value" (when used in a configurable field / when used in a base field override), which would suck :-)

Another approach that standardise default value storage regardless of cardinality. Also rerolled for #2293773: Field allowed values use dots in key names - not allowed in config

I've attached a couple of interdiffs so you can see that #65 is fully reverted and it is easy to see how it's been fixed.

This should fix some of the test fails.

Gotta love tests where the test runner and the site under test get different containers. Use of $this->container->get('module_handler')->install() and then making requests to the site is very dodgy.

1. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinitionBase.php
   @@ -0,0 +1,441 @@
   +      // Convert to the multi value format to support fields with a cardinality
   +      // greater than 1.
   +      $value = array(
   +        array('value' => $value),
   +      );
   

   Fine, but we cannot hardcode the 'value' property, this would only be valid for a couple field types.
   It should be the name of the first property for the field type.
   This can be obtained through $this->getFieldStorageDefinition()->getPropertyNames()[0].

2. +++ b/core/modules/node/src/NodeTypeForm.php
   @@ -17,23 +19,53 @@
   +      // Create a node with a fake bundle using the type's UUID so that we can
   +      // get the default values for workflow settings.
   +      $node = entity_create('node', array('type' => $type->uuid()));
   ...
   +      // Create a node to get the current values for workflow settings fields.
   +      $node = entity_create('node', array('type' => $type->id()));
   

   Ouch. but yeah, i have nothing better to propose :-/.

Re; #73

1. Fixed - used getMainPropertyName() instead and throwing an exception when we really wouldn't know what to do.
2. Indeed

Hmm - I'm wary of getMainPropertyName(), it's semantics is kind of shaky, and it's not defined for every field type :-/.

We're doing something symmetric to FieldItemBase::setValue() here, and what is done there is :

    if (isset($values) && !is_array($values)) {
      $keys = array_keys($this->definition->getPropertyDefinitions());
      $values = array($keys[0] => $values);
    }

which is equivalent to getPropertyNames()[0] (the getPropertyNames() method didn't exist when this code was written)

Ok... weird to have getMainPropertyName() then... but here's the change to use getPropertyNames()

Still mulling on terminology a bit, meanwhile, a couple down-to-earth code remarks :

1. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
   @@ -170,20 +170,25 @@ public function getDefaultValue(ContentEntityInterface $entity);
   -  public function setTranslatable($translatable);
   ...
   +  public function setDefaultValue($value);
   

   So we remove setTranslatable() and add setDefaultValue() - not sure why in both cases :-) ?

   Also, the phpdoc of FieldDefinition::setDefaultValue() should be switched to {@inheritdoc}.

2. +++ b/core/modules/content_translation/content_translation.module
   @@ -843,11 +813,6 @@ function content_translation_save_settings($settings) {
   -        // Store whether fields are translatable or not.
   -        if (!empty($bundle_settings['fields'])) {
   -          content_translation_set_config($entity_type, $bundle, 'fields', $bundle_settings['fields']);
   -        }
   -
   

   then the phpdoc content_translation_save_settings() should be updated, it still mentions a 'field' key (BTW, it also mentions a 'settings' entry that doesn't seem used at all ?)

   Plus:
   - So content_translation_set_config() is never used to save 'fields' anymore - does it mean some config schema somewhere should be updated/removed ?
   - content_translation_set_config() is actually only ever called to save 'enabled' - means there could be some simplification in this area ?

   Overall, looks like there could be some refactoring / simplification in this area as a followup

3. +++ b/core/modules/node/src/NodeTypeForm.php
   @@ -17,23 +19,53 @@
   +    $bundle_of = $type->getEntityType()->getBundleOf();
   ...
   +      $fields = $this->entityManager->getBaseFieldDefinitions($bundle_of);
   

   Hmm, sure, but this is NodeTypeForm, so why not just directly 'node' ?

4. +++ b/core/profiles/standard/config/install/node.type.article.yml
   @@ -2,14 +2,10 @@ type: article
   -      status: true
   -      promote: true
   -      sticky: false
   

   Apparently there remains some of these in a couple node.type.*.yml files present in core.

5. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
   @@ -759,7 +759,18 @@ protected function setUpEntityWithFieldDefinition($custom_invoke_all = FALSE, $f
   +    $override_entity_class = get_class($entity);
   +    $override_entity_type->expects($this->any())
   +       ->method('getClass')
   +       ->will($this->returnValue($override_entity_class));
   

   Minor : matter of taste, but I'd think the code would be more readable if $override_entity_class var was inlined (same thing lower in the file)

   + I might be missing something but this looks like we only add "setup" stuff but don't actually use it to test more things ?

Also :

FieldInstanceConfigInterface should extend BundleFieldDefinitionInterface ?
And then, what's left in FICI that's not in BFDI ? Do we really need two interfaces here ?

[edited : fixed wrong class name]

Removing failing parts of \Drupal\migrate_drupal\Tests\d6\MigrateNodeTypeTest so we can concentrate on #2289391-17: Reconsider whether FieldDefinition should implement FieldStorageDefinitionInterface for now.

Renamed:

• BundleFieldDefinitionInterface => FieldConfigInterface
• BundleFieldDefinition => BaseFieldOverride
• BundleFieldDefinitionBase => FieldConfigBase

Field[Instance]Config and BaseFieldOverride both extend FieldConfigBase which implements FieldConfigInterface.
FieldConfigInterface extends ConfigEntityInterface, FieldDefinitionInterface.

There is still a FieldInstanceConfigInstance but once we implement bundle renames (which might be complex) all of it's functionality might be rolled in FieldConfigInterface.

Let me know if these changes accord with #2289391-17: Reconsider whether FieldDefinition should implement FieldStorageDefinitionInterface

#2289391: Reconsider whether FieldDefinition should implement FieldStorageDefinitionInterface is proposing to rename FieldConfigInterface to FieldConfigurationInterface. If that's approved IMO we should do it here.

"FieldConfigInterface / FieldConfigurationInterface" : might be my bad in IRC :-/. I think FieldConfigInterface would be best.

Personally I'd prefer the full version as we don't have FieldDefInterface, but I won't fight for this :)

Realised that #2289391-17: Reconsider whether FieldDefinition should implement FieldStorageDefinitionInterface wanted BaseFieldOverride to be BaseFieldBundleOverride and it to have a config prefix of 'field'. Patch fixes that and renames EntityManager::getBundleFieldDefinition() to EntityManager::getConfigurableField()

Tidied up a few things FieldInstanceConfigInterface and made bundle renames work. I could not think of anywhere other than the system module to put the implementation of hook_entity_bundle_rename() - if we don't like this the only other option is to add this to field_entity_bundle_rename().

This additional functionality has only been manually tested - need to add tests for this and most of the functionality in BaseFieldBundleOverride and the changes to EntityManager.

Whilst coding this I was lead to wonder if we'd to implement functionality that if you save a BaseFieldBundleOverride config entity and this changes it to make the original BaseField and therefore obsoletes the override should we actually do a delete in BaseFieldBundleOverride::postSave()? Obviously it is super odd calling save on something and it being deleted so maybe this thought can be ignored.

+++ b/core/modules/node/src/NodeTypeForm.php
@@ -18,23 +20,54 @@
     if ($this->operation == 'add') {
       $form['#title'] = String::checkPlain($this->t('Add content type'));
+      $fields = $this->entityManager->getBaseFieldDefinitions($bundle_of);
+      // Create a node with a fake bundle using the type's UUID so that we can
+      // get the default values for workflow settings.
+      $node = entity_create('node', array('type' => $type->uuid()));
     }
     elseif ($this->operation == 'edit') {
       $form['#title'] = $this->t('Edit %label content type', array('%label' => $type->label()));
+      $fields = $this->entityManager->getFieldDefinitions($bundle_of, $type->id());
+      // Create a node to get the current values for workflow settings fields.
+      $node = entity_create('node', array('type' => $type->id()));

This statement been an if/elseif implies that the operation could be something other than add or edit but $node is used below to build $workflow_options regardless. If the operation wasn't add or edit it would throw an error. Same goes for $fields.

Rerolled and added a test and for renaming.

So calling the base field override files core.field.* seems a bit odd - isn't core.base_field_override.* better since it explains better what the file is doing and what will happen when it is removed - i.e. the field is not going to go away, the override is.

Apart from that I think the all that remains is to decide on the appropriate level of test coverage - I think all of the functionality is implicitly covered by content_translation tests, the existing Field[Instance]Config tests and the changed NodeTypeRenameConfigImportTest.

@benjy re #89

 * @ConfigEntityType(
 *   id = "node_type",
 *   label = @Translation("Content type"),
 *   controllers = {
 *     "access" = "Drupal\node\NodeTypeAccessController",
 *     "form" = {
 *       "add" = "Drupal\node\NodeTypeForm",
 *       "edit" = "Drupal\node\NodeTypeForm",
 *       "delete" = "Drupal\node\Form\NodeTypeDeleteConfirm"
 *     },
 *     "list_builder" = "Drupal\node\NodeTypeListBuilder",
 *   },

I'm not sure this form really could be used for anything else looking at the NodeType annotation. Otherwise what will the $form['#title'] be?

Yeah agreed, maybe we should make it simply an if/else.

@alexpott:

- core.field.* / core.base_field_override.*:
Yeah, you're probably right. Longer CMI names, but consistency with the entity_type_id reduces confusion.

- auto-deleting overrides when the overriden values equal the underlying code values:
That's an interesting idea, but I suspect there could be tricky aspects (right, "save() turns into delete()" might be surprising, not sure whether it's a real issue).
Plus, we still intend to have a 'third_party_settings' in there, right ? For those, we don't have any knowledge of 'default values', so there's no way to decide that "the current override values are equal to defaults and can be removed".
We should keep that for a followup IMO.

Not sure whether #77 has been adressed ?

Also :

- The more I think of it, the more I'm leaning towards :
FDI::getConfig($bundle) returns FCI
rather than
EM::getConfigurableField($entity_type, $bundle, $field_name)

I.e : if you're one of the very rare code that needs a guaranteed FCI because you're going to write to it, you can get it from the FDI the "generic EM API" gives you, but we don't clutter EM itself with this additional notion.
(additionally, the current EM::getConfigurableField() is a bit off, because it's the only one to "target a single field", while the others - EM::getFieldDefinitions(), EM::getBaseFieldDefinitions() are all plural, currently without single-field variants)

- See thoughts in #2289391-21: Reconsider whether FieldDefinition should implement FieldStorageDefinitionInterface about the impact of the existence of FCIs on EntityType::bundleFieldDefinitions()

Last "also" for the day : The optimization done in #2313875: Preserve the 'field_type' within FieldInstanceConfig will most likely impact this patch - a similar optim for BaseFieldOverride would probably make sense ?

Addressed #77 and renamed core.field.* to core.base_field_override.* - NOTE this is still not exactly inline with the name - which would be core.base_field_bundle_override but I think it is long enough.

re #77

1. I think we've agreed that FieldDefinitionInterface should not have setters but FieldDefinition can. So I've create setDefaultValue on FieldConfigInterface
2. Fixed PHPDoc to only mention what is used. Yep lots of simplification possible here - especially after #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration
3. Fixed
4. Fixed book, forum and a couple of test node.types
5. Fixed and yep this is just there to test the EntityManager - it's hard to actually use PHPUnit to mock everything that would be necessary here because of the get_class's.

re #93 I've implemented getConfig in this latest patch. Personally I think

    // Override a core base field.
    $fields = \Drupal::entityManager()->getFieldDefinitions($content_type->getEntityType()->getBundleOf(), $content_type->id());
    $fields['status']->getConfig($content_type->id())->setDefaultValue(FALSE)->save();

Isn't great since you are passing the bundle into getFieldDefinitions and you have to pass the bundle into getConfig as well. As getConfig needs a better name.

re #94 shall we address this here on in a follow-up or that issue? Since we can probably implement most of this on FieldConfigBase?

I wrote the three new migrations and a test to fix the migrate issues. Put the patch in a follow-up. #2315167: Create migrations for status/promote/sticky

Brought issue summary in line with latest patch and #2289391-17: Reconsider whether FieldDefinition should implement FieldStorageDefinitionInterface

1. +++ b/core/lib/Drupal/Core/Field/Entity/BaseFieldBundleOverride.php
   @@ -0,0 +1,196 @@
   +    return $this->getBaseFieldDefinition()->getFieldStorageDefinition();
   

   No biggie, but $this->getBaseFieldDefinition() would also work, wouldn't it ?

2. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -0,0 +1,464 @@
   +  public function __sleep() {
   +    // Only serialize necessary properties, excluding those that can be
   +    // recalculated.
   +    $properties = get_object_vars($this);
   +    unset($properties['fieldStorage'], $properties['itemDefinition'], $properties['bundle_rename_allowed']);
   +    return array_keys($properties);
   +  }
   ...
   +  public function __wakeup() {
   +    // Run the values from self::toArray() through __construct().
   +    $values = array_intersect_key($this->toArray(), get_object_vars($this));
   +    $this->__construct($values, $this->entityTypeId);
   +  }
   

   - #2313053: Field storage and instance call toArray() in __wakeup() is very slow, remove it? removed __wakeup() from FieldInstanceConfig.
   - the __sleep() code is now specific to FIC, won't work for BFBOverride - those have a 'baseFieldDefinition' property rather than 'fieldStorage', and no 'itemDefinition'.

   That's exactly why I wish we had #1977206: Default serialization of ConfigEntities - currently stalled on toArray() not working for fresh config entities ;-)

re #98:

1. To keep us honest - in case we ever separate FieldDefinition into its storage and instance components. This ensures we are returning a FieldStorageInterface object.
2. Fixed and improved BaseFieldBundleOverride's __sleep implementation too

Rerolled.

Fixed EntityManagerTest - wow that test has a lot of mocking.

1. I'm going to wait for more field back on this - I think I'd prefer everything to be base_field_override since for me core.field_override could be thought to override field.field.*
2. Fixed
3. Open to ideas here - what the current condition is checking is that the provided field implements both FSDI and FDI. Actually the only important thing is that it implements FDI. We could ensure that there is no bundle by using getBundle() since we should only override things that can not be configured on the bundle level. Which is what I've implemented here. Yes we're going to get rid of getBundle() but we need some replacement
4. Fixed
5. Yeah this is a remanent of when setters where going to be added to the FDI
6. Added @todos and created #2318187: FieldDefinitionInterfacegetDefaultValue() requires an entity

I also removed fullstops from the exception messages in BaseFieldBundleOverride

Talked some more with @effulgentsia and he realised that we did not need the ability to pass a field object into the constructor since with have createFromFieldDefinition(). Less code++

Also removed remnants of the old name Bundle field definitions

No clear opinion yet, and happy to let others chime in, but if the entity type was field_override, then the class name should be FieldOverride as well (rather than BaseFieldBundleOverride) for the same reasons ?

Great work, thanks Alex. This looks close to be ready to me as well.

- auto-deleting overrides when the overriden values equal the underlying code values:

I do not think we should do this. There is a difference between a value overriding the default although the value is the same and having no override - when the default changes one will follow and the other one not. I do think that you only expect your configuration to follow defaults when your override is *deleted*, but not when it coincidentally matches defaults.

 *   id = "base_field_bundle_override",
 *   label = @Translation("Base field override"),

Usually label and id match. I actually think base_field_override describes it better anyway, base_field_bundle_override sounds confusing to me as it sounds like an override of a "base_field_bundle", whatever that should be. That the override is stored on bundle level does not have to be reflected in the name imo.

- core.field.* / core.base_field_override.*:
Yeah, you're probably right. Longer CMI names, but consistency with the entity_type_id reduces confusion.

Agreed . I do like core.base_field_override - it tells quite clearly what it really does.

Here's my thinking on why people thinking that core.field_override might override more than only base fields is ok, maybe even desirable.

Implementation wise, it's overriding base fields only now - so having config that implies something that is not happening seems confusing to me.

And there's no reason to discourage contrib from swapping out that implementation. And therefore, I think we should name the config entity type and YAML files with only the conceptual bit (that it's for overriding fields), and not with the implementation-detail bit (that in core, by default, we're only using those config entities for base fields).

If contrib swaps out that implementation, it can easily move to a fitting config file as well imho. Also I cannot think of good use case for doing so right now - thus, without use-cases I don't think this is something we should prepare for here. However, I'd be interested in possible use-cases for this?

Personally I'd prefer the full version as we don't have FieldDefInterface, but I won't fight for this :)

I'd prefer FieldConfiguration instead of FieldConfig as well as it works out nicely besides FieldDefinition. I do see that CMI goes with "config" all over the place, so I'm ok with sticking to FieldConfig also.

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -475,8 +478,20 @@ protected function buildBundleFieldDefinitions($entity_type_id, $bundle, array $
   +      // @todo possible performance optimisation to do a loadMulitple() here
   +      //   outside of the foreach.
   

   Yep, we really should do this. See how field_entity_bundle_field_info() does it.

2. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -0,0 +1,453 @@
   +
   

   One new line too much.

3. +++ b/core/lib/Drupal/Core/Field/FieldConfigInterface.php
   @@ -0,0 +1,76 @@
   +
   

   Another NL too much. ;)

4. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
   @@ -170,20 +170,21 @@ public function getDefaultValue(ContentEntityInterface $entity);
   +  /**
   +   * Gets an object that can be saved in configuration.
   +   *
   

   I think this needs more documentation on when you want a config object and what you'd use it for.

   Also it should explain that it will create a new config object in case there is non yet.

5. +++ b/core/modules/node/src/NodeTypeForm.php
   @@ -26,15 +54,21 @@ public function form(array $form, FormStateInterface $form_state) {
   +      // Create a node to get the current values for workflow settings fields.
   +      $node = entity_create('node', array('type' => $type->id()));
   

   This should use the manager, especially as it is already there.

6. +++ b/core/modules/system/system.module
   @@ -1715,3 +1715,17 @@ function system_path_insert() {
   +function system_entity_bundle_rename($entity_type, $bundle_old, $bundle_new) {
   +  $overrides = entity_load_multiple_by_properties('base_field_bundle_override', array('entity_type' => $entity_type, 'bundle' => $bundle_old));
   +  foreach ($overrides as $override) {
   

   As the bundle overrides are handled directly by the entity system, I think it's the job of the entity system to take care of the changes as well? I.e. move it to entity_invoke_bundle_hook()?

@fago #109:

1. Fixed - injected config entity query factory into the entity manager - we can not inject entity.query since it depends on the entity.manager
2. Fixed
3. Fixed
4. Fixed
5. Fixed
6. Moved this to the entity manager by doing:
   

     // Notify the entity manager.
     if (method_exists($entity_manager, $method)) {
       $entity_manager->$method($entity_type, $bundle, $bundle_new);
     }
   

   in entity_invoke_bundle_hook() - putting actual business logic in that function looked really bad.

Other considerations

I've gone for base field override - since that is exactly what is occurring. The class, ID and config_prefix now all match.

The only thing left to do is to add tests for the new code

    $base_field_override_ids = $this->configQueryFactory->get($this->getDefinition('base_field_override'), 'AND')
      ->condition('id', $entity_type->id() . '.' . $bundle . '.', 'STARTS_WITH')
      ->execute();
    if ($base_field_override_ids) {
      $base_field_overrides = $this->getStorage('base_field_override')->loadMultiple($base_field_override_ids);
      foreach ($base_field_overrides as $base_field_override) {
        /** @var \Drupal\Core\Field\Entity\BaseFieldOverride $base_field_override */
        $field_name = $base_field_override->getName();
        if ($base_field_definitions[$field_name]) {
          $bundle_field_definitions[$field_name] = $base_field_override;
        }
        else {
          throw new \LogicException(String::format('The @field_name base field override has no corresponding field to override on @bundle bundle on @type entity type', array('@field_name' => $field_name, '@type' => $entity_type->getLabel(), '@bundle' => $bundle)));
        }
      }
    }

Since this now potentially loads overrides for base fields that do not exist - we should be testing that the expected exception is thrown.

Whilst writing a test for the exception I realised that throwing an exception is wrong since at this point your site is not broken but it would be unrecoverable. Added a test to ensure that overriding a non existing base field does not cause the field definition to come into existence.

Do our standards require this to be shorter to fit on 1 line?

summaries can be over 80 characters if necessary.

Thanks, changes are great. Here a couple of remarks, no biggies though:

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -174,6 +177,7 @@
   +    $this->configQueryFactory = $config_query_factory;
   

   This misses a defined property.

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -479,18 +483,25 @@
   +    if ($base_field_override_ids) {
   +      $base_field_overrides = $this->getStorage('base_field_override')->loadMultiple($base_field_override_ids);
   

   Great to see that implemented already - thanks!

3. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
   @@ -180,6 +180,12 @@
   +   * properties per bundle use this method to create an base field bundle
   

   *a* base field

4. +++ b/core/includes/entity.inc
   @@ -67,6 +67,12 @@ function entity_invoke_bundle_hook($hook, $entity_type, $bundle, $bundle_new = N
   +  // Notify the entity manager.
   +  if (method_exists($entity_manager, $method)) {
   +    $entity_manager->$method($entity_type, $bundle, $bundle_new);
   

   We probably should move the whole function to the manager? I'd suggest we deal with that in its own issue.

5. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -950,2 +961,26 @@
   +  public function onBundleRename($entity_type_id, $bundle_old, $bundle_new) {
   

   Shouldn't that be part of the interface? But that can be dealt with in the same issue.

This is looking great to me as well! Just a couple of minor remarks that can be addressed post-commit or if we happen to reroll this.

1. +++ b/core/lib/Drupal/Core/Field/Entity/BaseFieldOverride.php
   @@ -0,0 +1,193 @@
   +        throw new FieldException("Cannot change the bundle of an existing base field bundle override");
   

   We could specify the involved bundle names (plus trailing dot).

2. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -0,0 +1,452 @@
   +  protected $bundle_rename_allowed = FALSE;
   

   This should be camel case.

Re #113

1. Fixed
2. Fixed
3. Fixed

Re #116

1. Fixed
2. No problem :)
3. Fixed
4. Agreed
5. Agreed

Re #117

1. Exceptions should not have a fullstop I've changed the exception to include the entity type, bundle and field name.
2. Fixed

Rerolled since I committed #2315237: Rename FieldDefinition to BaseFieldDefinition

Slightly borked reroll :(

1. @@ -67,6 +67,12 @@ function entity_invoke_bundle_hook($hook, $entity_type, $bundle, $bundle_new = N
   

   Agreed with earlier comments that entity_invoke_bundle_hook() should be moved to a method on the EM.
   Opened #2319171: Move entity_invoke_bundle_hook() to EntityManager.

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -475,8 +486,25 @@ protected function buildBundleFieldDefinitions($entity_type_id, $bundle, array $
   +        // Ensure that the override is for an existing base field.
   +        if (isset($base_field_definitions[$field_name])) {
   

   Minor: the EFQ could be built with "id IN [list of known base fields]" rather than with "STARTS WITH" + filter unknown base fields ?

3. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -475,8 +486,25 @@ protected function buildBundleFieldDefinitions($entity_type_id, $bundle, array $
   +    // Allow the entity class to override the base fields. Base fields which
   +    // have already been overridden using BaseFieldOverride take priority.
   +    $bundle_field_definitions = array_merge($class::bundleFieldDefinitions($entity_type, $bundle, $base_field_definitions), $bundle_field_definitions);
   

   The order is a bit anti-intuitive here. The code order is generally "build stuff incrementally on top of the previous step", but here we discard results of that step if the previous step says something about the field.

   We could start by calling $class::bundleFieldDefinitions(), and then collect overrides on top of that ?

4. +++ b/core/lib/Drupal/Core/Field/Entity/BaseFieldOverride.php
   @@ -0,0 +1,191 @@
   + * @ConfigEntityType(
   + *   id = "base_field_override",
   + *   label = @Translation("Base field override"),
   + *   config_prefix = "base_field_override",
   + *   entity_keys = {
   + *     "id" = "id",
   + *     "label" = "label"
   + *   }
   + * )
   

   There is a dedicated FICStorage, but no dedicated BFOStorage class.
   But at least some of FICStorage would make sense for BFOStorage too :
   mapFromStorageRecords(), mapToStorageRecord() ?
   loadByProperties() ?

   (note : we might take the opportunity to update the comment about import order and alphabetical order at the beginning of FieldInstanceConfigStorage, which seems quite stale now)

5. +++ b/core/lib/Drupal/Core/Field/Entity/BaseFieldOverride.php
   @@ -0,0 +1,191 @@
   +    // 'bundle' is required in either case.
   +    if (empty($values['bundle'])) {
   +      throw new FieldException(String::format('Attempt to create a base field bundle override of field @field_name without a bundle', array('@field_name' => $values['field_name'])));
   +    }
   

   Comment is useless now, there are no "either case" anymore here :-)

6. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -0,0 +1,451 @@
   +abstract class FieldConfigBase extends ConfigEntityBase implements FieldConfigInterface {
   

   Maybe the method order could be enhanced a bit ? The class looks like a bag of methods without much structure.

7. +++ b/core/lib/Drupal/Core/Field/FieldConfigInterface.php
   @@ -0,0 +1,75 @@
   +interface FieldConfigInterface extends FieldDefinitionInterface, ConfigEntityInterface {
   

   Nitpick : could we reorder methods so that setters are grouped together (allowBundleRename() & targetBundle() sit in the middle of two setters)

8. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
   @@ -170,20 +170,27 @@ public function getDefaultValue(ContentEntityInterface $entity);
   +   * properties per bundle use this method to create a base field bundle
   +   * override that can be saved in configuration.
   

   "a base field bundle override" is something we moved away from in the last couple interations. maybe just "to create an override that can be saved in configuration" ?

9. --- /dev/null
   +++ b/core/modules/book/config/install/core.base_field_override.node.book.promote.yml
   
   --- /dev/null
   +++ b/core/modules/forum/config/install/core.base_field_override.node.forum.promote.yml
   
   --- /dev/null
   +++ b/core/modules/forum/config/install/core.base_field_override.node.forum.title.yml
   
   --- /dev/null
   +++ b/core/profiles/standard/config/install/core.base_field_override.node.page.promote.yml
   

   So, yeah, "full override" means it's not too clear why we add those - i.e. what's the diff with the original code definition.
   Maybe we could add a comment ? Would be lost once this gets merged in the active store, but still worthwhile to document in the yaml in config/install ?

   Also, we'll have to remind to update those if we later change the code definition - but nothing in the code definition warns that core defines an override somewhere...
   --> From now on, each time we adjust a base field definition, we need to check for the existence of overrides...

   All in all, still not sure that "full override" is the right approach, but that can be re-discussed / adjusted in a followup if need be.

10. +++ b/core/modules/book/config/install/node.type.book.yml
    @@ -2,15 +2,10 @@ type: book
    -title_label: Title
    

    Greping 'title_label' in the codebase brings quite a few results still, looks like at least some of them are stale now ?

11. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -306,20 +305,11 @@ function content_translation_form_language_content_settings_submit(array $form,
    +          $definition = $fields[$field_name]->getConfig($bundle);
    

    Minor: $field_config rather than $definition ?

12. +++ b/core/modules/content_translation/src/Tests/ContentTranslationSettingsTest.php
    @@ -125,10 +122,10 @@ function testSettingsUI() {
    +    $bundle_definition = BaseFieldOverride::loadByName('entity_test_mul', 'entity_test_mul', 'name');
    

    Minor: $field_override rather than $bundle_definition ?

13. +++ b/core/modules/field/src/Entity/FieldInstanceConfig.php
    @@ -361,34 +174,12 @@ public function calculateDependencies() {
         // Manage dependencies.
         $this->addDependency('entity', $this->getFieldStorageDefinition()->getConfigDependencyName());
    

    Now that part of the logic is delegated to the parent, "Manage dependencies" is a bit dumb for a comment :-)
    --> "Mark the field_storage_config as a a dependency" ?

    (the similar inline comment in the parent implementation could similarly be detailed a bit)

14. +++ b/core/modules/forum/src/Tests/ForumTest.php
    --- a/core/modules/migrate_drupal/src/Tests/d6/MigrateNodeTypeTest.php
    +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateNodeTypeTest.php
    

    Lots of @todos in there - what's the process for those ?

15. Most / all of FIC::preSave() would also apply for baseFieldOverride ?
16. The beginning of FIC::postDelete() too :
    \Drupal::entityManager()->clearCachedFieldDefinitions();
    Call EM->getStorage()->onFieldDefinition*Update*() ? (rather than onFieldDefinitionDelete...)

Also, @alexpott, @plach, @Gabor:

+++ b/core/modules/content_translation/content_translation.module
@@ -844,11 +810,6 @@ function content_translation_save_settings($settings) {
         // Store whether fields have translation enabled or not.
         [+ some FIC-specific case about 'translation_sync']

Lost track a bit - IIRC, 'translation_sync' was the main reason we went with "all fields can be configured". So in which issue do we generalize this snippet to all fields ? In the issue where we add "third party settings" ?

(also, not the fault of this patch, but the comment is quite stale / misleading, this is about 'translation_sync', not about "field translation enabled or not")

Last : see #2289391-21: Reconsider whether FieldDefinition should implement FieldStorageDefinitionInterface - IMO the existence of BaseFieldOverride means EntityType::bundleFieldDefinitions(), hook_entity_bundle_field_info[_alter]() should be made to return FCIs rather than clones of BFDs ? And thus, renamed to bundleFieldConfig() ?

Might be best deferred to a followup ?

Re #122:

1. Thanks for opening the follow up
2. Nice idea - we can completely remove the config entity query dependency :)
3. Fixed - less complexity - nice!
4. Fixed - created another base class that implements mapFromStorageRecords and mapToStorageRecords - I don't think loadByProperties is needed since this is all about including deleted fields.
5. Fixed
6. Let's do this is a followup
7. Let's do this in the same followup as #6
8. Fixed
9. Added a comment to each override file.
10. I think all the title_label in core is okay - either will be addressed by the migrate followup or is a correct usage of the form element on NodeTypeForm
11. Fixed
12. Fixed
13. Fixed
14. There is a followup and patch already - the migrate team are awesome.
15. Fixed (i think)
16. Fixed (i think)

Re: #123
Updated comment. The translation_sync stuff will be handled in the postponed beta blocker - #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration

Re #124
Yep let's handle that in a followup.

Fixed up the post delete.

Thanks @alexpott.

6. 7. (method order) : those are entirely new files, so pushing a clean method order to a followup feels a bit artificial :-p
But fine, opened #2319787: Reorder methods in FieldConfigBase & FieldConfigInterface

10. title_label : indeed, sorry, my bad.

About #124: opened #2319821: Make EntityType::bundleFieldDefinitions(), hook_entity_bundle_field_info[_alter]() return FieldConfigInterface objects

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -485,8 +488,22 @@ protected function buildBundleFieldDefinitions($entity_type_id, $bundle, array $
   +    // Ensure that the override is for an existing base field.
   +    $base_field_override_ids = array_keys($base_field_definitions);
   +    array_walk($base_field_override_ids, function(&$field_name) use ($entity_type_id, $bundle) {
   +      $field_name = $entity_type_id . '.' . $bundle . '.' . $field_name;
   +    });
   

   - Comment is outdated now.
   - The comment above about "overrides take priority" could be moved to this place here where we do load the overrides ?

   + matter of taste: in-place turning an array of field names into an array of config ids is a tad cryptic and makes a var name that can't be accurate for both. Could be two separate arrays with an array_map() ?

2. +++ b/core/lib/Drupal/Core/Field/BaseFieldOverrideStorage.php
   @@ -0,0 +1,60 @@
   +  /**
   +   * The field type plugin manager.
   +   *
   +   * @var \Drupal\Core\Field\FieldTypePluginManagerInterface
   +   */
   +  protected $fieldTypeManager;
   +
   

   Already present in FCStorageBase --> no need to be re-defined here ?
   (same for FICStorage)

   Also means BFOStorage::__construct() could move to FCStorageBase::__construct() ?

   (at this point, there's not much left in BFOStorage, but it's probably best to still keep an empty FCStorageBase class rather than directly use FCStorageBase as the storage handler for base_field_override ?)

3. +++ b/core/lib/Drupal/Core/Field/Entity/BaseFieldOverride.php
   @@ -0,0 +1,220 @@
   +  public function preSave(EntityStorageInterface $storage) {
   ...
   +
   +  }
   

   Leftover empty line.

4. +++ b/core/lib/Drupal/Core/Field/Entity/BaseFieldOverride.php
   @@ -0,0 +1,220 @@
   +    if ($this->isNew()) {
   +      $this->original = $this->getBaseFieldDefinition();
   +    }
   

   Why do we need this ? IIRC $this->original should be automatically populated for us upstream, and there's nothing similar in FIC ?

Re #129:

1. By creating a list of possible IDs we are still ensuring the overrides are only for existing base fields. Fixed comments and used array_map (much nicer)
2. Removed $fieldTypeManager - I think I prefer to keep the constructor on the concrete object so that it aligns nicely with the createInstance()
3. Fixed
4. We need this because when create a new entity we don't have an original property and we call out to onFieldDefinitionUpdate at the bottom of preSave passing the updated or new override and the original override or base field definition. An FIC when it is new does not need an original.

And now for the patch :)

Update fixes some unused variables and incorrect PHPDoc blocks.

Thanks @alexpott !

Down to the very last nitpicks, after that, I swear I'll shut up :-)

- re #130-1 :

What I meant about the comments in EM::buildBundleFieldDefinitions() is that the code currently reads :

// Allow the entity class to override the base fields.
[call ET::bundleFieldDefinitions()]

// Ensure that overrides are for existing base fields.
[load the BFOs from config]

Those two comments blocks talk about "overrides", but it's not the same for the two code blocks. That second comments looks like the code below is about checking something about the "overrides" included in the 1st code block.

The comment that's missing for the overall method to make sense is that this second code block "loads the overrides stored in configuration". The fact that we ensure only those for known base fields are loaded is only a nice-to-have side note, but misses the point if on its own :-)

- re #130-4 :

Oh - right. If isNew(), we place a value in $this->original so that we can use $this->original later on. A bit hackish IMO.
I'd suggest populating a $previous_definition var in both branches of the if (isNew()) {..} else {...}, and then using $previous_definition in the common branch ?

+++ b/core/lib/Drupal/Core/Field/Entity/BaseFieldOverride.php
@@ -0,0 +1,223 @@
+    // Set the default instance settings.
+    $this->settings += \Drupal::service('plugin.manager.field.field_type')->getDefaultInstanceSettings($this->getType());
+    parent::preSave($storage);
+    // Notify the entity storage.
+    $this->entityManager()->getStorage($this->getTargetEntityTypeId())->onFieldDefinitionUpdate($this, $this->original);

Do we really have to call the parent specifically between two operations here ? It's easily missed there.
Could we move this at the beginning of the method ?

(side note : apparently FIC::preSave() doesn't bother with calling the parent at all :-/ - opened #2320253: FIC/FSC::preSave() do not call the parent implementation)

re #133 hopefully this addresses both points.

The docs in #135 are a big improvement.

Can we get an explicit followup for #136 so we can call this done? :)

#136 questions me a bit, not sure whether there's an actual problem here, but fine with looking into this in a followup.

RTBC +1, was about to do it myself :-). Thanks a lot @alexpott!

Woot!

Assigning to catch for committer review based on previous discussion with effulgentsia and alexpott. :)

Thanks, updates look good to me also. +1 on the added docs.

+    // Load base field overrides from configuration. These take precedence over
+    // base field overrides returned above.
+    $base_field_override_ids = array_map(function($field_name) use ($entity_type_id, $bundle) {
+      return $entity_type_id . '.' . $bundle . '.' . $field_name;
+    }, array_keys($base_field_definitions));

hm, so when I have non-configuration driven entity type and provide some bundle field definitions, the configuration entities which might be added in a general manner,e.g. by content translation module, will undo my bundle field definition changes? That's a problem as the bundle field definition is probably there for a reason -> setting to needs work for this :-/

I think we have to improve BaseFieldOverride to fall-back to a general field definition object, which then can be the base field defintiion - or if existing - the bundle field defintion. That means, we'd have to respect that when creating BaseFieldOverride in getConfig() and inject existing bundle field definitions when loading overrides when building bundle field definitions in the EM. Thoughts?

+label: Promote
+description: 'A boolean indicating whether the node should be displayed on the front page.'
+required: false
+translatable: true
+default_value:

Also, I'm not sure it's feasible to duplicate the information of base fields here? Wouldn't it be better to only keep overridden values?
What if the entity type providing module wants to change a description of a base field later on, it would not take any effect once content translation module has started adding overrides for a bundle? But that doesn't seem to be intuitive to me as the entity type providing module cannot know whether another module has started doing overrides or not. Thus, imho we have to keep overridden properties only?

@fago: on the overrides/code change discussion and why it was discarded see #39 to #45 (and maybe onwards a bit). Was raised a month ago :)

I know that discussions touched this topic on overrides/code change discussion already, but the current patch does not implement either approach in a reasonable way: There are code based bundle definitions and config based definitions, but when there is no safe way to use code-based definitions (see #143) there is no point in having them any more. They either need to be usable or should go away in the first place.

ok, gave this some more thought:

- I agree the general override issue of code base-fields in #143 is addressed, I mean it's not nice, but by thinking through multiple scenarious I was not able to come up with a really problematic issue. So it seems there is no point to re-iterating on the shallow override discussion - sry for that.

- But what we've not discussed yet imo, is how this works with code-based overrides:

+    // Allow the entity class to provide bundle fields and bundle-specific
+    // overrides of base fields.
     $bundle_field_definitions = $class::bundleFieldDefinitions($entity_type, $bundle, $base_field_definitions);

There is no way of using this, without changes being dropped by content translation (once enabled). So we should either drop support for it, or fix it?

The approach for fixing it would be:

I think we have to improve BaseFieldOverride to fall-back to a general field definition object, which then can be the base field defintiion - or if existing - the bundle field defintion. That means, we'd have to respect that when creating BaseFieldOverride in getConfig() and inject existing bundle field definitions when loading overrides when building bundle field definitions in the EM. Thoughts?

Seems doable. It has the same update problem as code based field definitions, but that's fine - it's just the same.

When we'd go with the removal option, I see only this problem:

+  /**
+   * {@inheritdoc}
+   */
+  public function isDisplayConfigurable($context) {
+    return $this->getBaseFieldDefinition()->isDisplayConfigurable($context);
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getDisplayOptions($display_context) {
+    return $this->getBaseFieldDefinition()->getDisplayOptions($display_context);

It seems to me that this would be a very likely use-case for overridding, e.g. change the default widget per bundle. But it's not possible right now with the config based approach, so when we'd remove the code-based override approach we'd have to make that possible I think.

On removal vs fix: Personally I'd tend to prefer the fixing way as it seems a bit arbitrary if we'd support code-based fields for base fields only and for bundle fields we only do config based ones.

#2319821: Make EntityType::bundleFieldDefinitions(), hook_entity_bundle_field_info[_alter]() return FieldConfigInterface objects touches the whole area as well, not sure how yched's idea would work in regard to saving them or not. However returning config from there does not seem to solve the problem of whatever you return there might be ignored?

ok, looking at the code again I actually think it does already work (good work!). It's just that

  public static function createFromBaseFieldDefinition(BaseFieldDefinition $base_field_definition, $bundle) {
    $values = $base_field_definition->toArray();
    $values['bundle'] = $bundle;
    $values['baseFieldDefinition'] = $base_field_definition;
    return \Drupal::entityManager()->getStorage('base_field_override')->create($values);
  }

is kind of confusingly named as it might be called from a base field override already. But we can improve its name + add test coverage for it in a follow-up. Thus, setting back to RTBC - sry for the noise :-)

@fago:

BFO::createFromBaseFieldDefinition() is kind of confusingly named as it might be called from a base field override already

No, because its param is typehinted as BaseFieldDefinition ?

Just a note I probably won't be able to review this in depth until Monday/Tuesday. The follow-ups look OK.

No, because its param is typehinted as BaseFieldDefinition ?

That's not a problem right now, as bundle overrise are (confusingly) based upon the same class right now. If we'd change the class it means we'd have to add a general createFromFieldDefinition().

I've done a small test case to proof current code is actually working right now, setting back to needs review for that part.

My opinion is we should change the name from core.base_field_override.*.yml to core.field_override.*.yml, so that contrib could use the exact same entity type for overriding non-base fields.

Implementation wise, it's overriding base fields only now - so having config that implies something that is not happening seems confusing to me.

So, actually the implemenation could be easily improved to handle all fields by using getStorageFieldDefinition() instead of getBaseFieldDefinitions() to fetch the storage field. Given that, I'd actually agree with effulgentsia that the config (+ entity type) should be core.field_override.*.yml then. I don't think the possible confusion around overriding field instances with config is not an issue - technically, you could do it, but there is no point in overriding a config field with config, so that's why we don't ;-)

Howsowever, I think we should defer further discussion around that to #2321443: Check if there's sufficient test coverage for base fields with simultaneous code-based and config-based overrides and re-evaluate it depending on what we want to do with code based bundle fields.

@fago nice extra text coverage - I'd been meaning to add a test for that. I'm comfortable with rtbc'ing the patch based on the changes in #152.

Problems with bot, not the patch.

Yet more "too many connections" fails on testbot - the tests pass locally just fine.

OK sorry it took me a while to get to this.

This whole issue feels like we're running into the limitations of trying to keep base fields and configurable fields separate. Trying to resolve the limitations of configurable fields (schema location, lack of reliability with the locked status etc.) might end up working out better in the long run, although hard to tell without a lot of work.

However if we're going to stick with the split, then the patch here (with the follow-ups) is fine I think. So, committed/pushed to 8.0.x, thanks!

@_nod: are you sure ? This issue has no JS implications...

it touches a js file so I'd like to have the tag. however insignificant it may seem. In this case there is an Id selector that should be replaced (in follow-up) by a data attribute. Problem is with drupal_HTML_id causing our fat ajax requests. tagging help me see what ppl run into (and why) so I can have a feel for refactoring priorities.

@_nod: oh, ok. Note that the patch merely changed the id used in an existing selector, as a result of a form structure change (this targets an id auto-generated by Form API on form elements).