Allow setting custom storage on FieldStorageConfig

Works for me. I guess you're working on the tests ? :)

One thing that I've been wondering is how field purging will behave when a such a field is deleted, we'll definitely have to check that.

Yeah, I'm assuming that as well, also, you can't uninstall a module until the fields it owns are gone, so your alter runs, so yeah, we're probably fine :)

I'm personally fine with this approach, however I vaguely remember discussing it with @yched, so it would better to have his signoff. I know also @Berdir had ideas about this, so it would be better to gather some more feedback.

+++ b/core/modules/field/src/Entity/FieldStorageConfig.php
@@ -455,7 +462,21 @@ public function getSchema() {
+   *   Pass FALSE if the storage takes care of storing the field,
+   *   TRUE otherwise.

80 chars

+++ b/core/modules/field/src/Tests/FieldCrudTest.php
@@ -138,6 +138,41 @@ function testCreateField() {
+   * Test creating a field instance with custom storage set.

instance ? instance ? That would fit on a post card yes for D7 :)

1. +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
   @@ -146,6 +146,13 @@ class FieldStorageConfig extends ConfigEntityBase implements FieldStorageConfigI
    
      /**
   +   * Flag indicating whether this field has custom storage.
   +   *
   +   * @var bool
   +   */
   +  protected $custom_storage = FALSE;
   +
   +  /**
   

   This can be removed now.

2. +++ b/core/modules/field/src/Tests/FieldCrudTest.php
   @@ -138,6 +138,41 @@ function testCreateField() {
   +   * Test creating a field instance with custom storage set.
   

   as @swentel said, just field.

3. +++ b/core/modules/field/src/Tests/FieldCrudTest.php
   @@ -138,6 +138,41 @@ function testCreateField() {
   +    $this->fieldStorage = entity_create('field_storage_config', $this->fieldStorageDefinition);
   ...
   +    /** @var \Drupal\Core\Field\FieldConfigInterface $field */
   +    $field = entity_create('field_config', $this->fieldDefinition);
   +    $field->save();
   

   If I don't then someone is going to tell you to use FieldConfig::create() and FieldStorageConfig::create(), should also make the @var unnecessary.

4. +++ b/core/modules/field/tests/modules/field_test/src/Plugin/Field/FieldType/CustomStorageTestItem.php
   @@ -0,0 +1,47 @@
   +    // No schema columns.
   +    return ['columns' => []];
   +  }
   

   So you need to return columns?

Yeah, I guess the reasoning was "if you are custom_storage, it means there is associated code somewhere that takes care of your storage, which means your code relies on your field, which is what base/code-defined fields are for". Things like : if the field is configurable, it can be removed. What happens then with the associated code ?

I'm fine with allowing the flexibility and letting people figure out the subtelties though :-)

As discussed with @Berdir and @damiankloip in BCN, I don't think this should involve the *field type* saying "I have custom storage", since that's not how it works for base fields : any base field can say "I'm of this field field type, and btw, I have custom storage", and the field type has no say in the process. FieldStorageDefinitionInterface::setCustomStorage() means any field of any type can have custom storage.

So we agreed that if configurable fields can have custom storage, it should be a regular property in the FieldStorageConfig yaml (most probably without a UI ?)
Then the case of OG ("for any field of type 'og_audience' that the admins create in Field UI, make sure it has custom_storage = TRUE") can be adressed by hook_entity_create() on 'field_storage_config' entities, since by the time the FSC entity is created, it already has a field type you can reason on.

1. +++ b/core/modules/field/src/Tests/FieldCrudTest.php
   @@ -138,6 +138,41 @@ function testCreateField() {
   +   * @see field_test_entity_bundle_field_info_alter
   

   No longer relevant :-)

2. +++ b/core/modules/field/src/Tests/FieldCrudTest.php
   @@ -138,6 +138,41 @@ function testCreateField() {
   +    $field_storage = FieldStorageConfig::create($field_storage_definition);
   ...
   +    $field = FieldConfig::create($field_definition);
   

   Nitpick, we could inline the definition arrays

3. +++ b/core/modules/field/src/Tests/FieldCrudTest.php
   @@ -138,6 +138,41 @@ function testCreateField() {
   +    $this->assertIdentical([], $field_storage->getColumns());
   +    $this->assertIdentical([], $field_storage->getSchema()['columns']);
   

   Not sure why we need this. The schema() and properties() should simply be the regular ones for the field type, we just want to ensure no table was actually created ?

   Which brings : not sure we actually need a specific 'test_field_custom_storage' to test this, using any field type shoud work ?

Thanks @damiankloip - RTBC for me when green :-)

Thanks !

Can we have a change notice and proper functional tests for a field with custom storage set to true?

I don't see the point in the change record. If people want to use it, they will look at the interface anyway. People don't have to update existing code.
Too many change records cause a big issue.

Why do we need additional tests past this? Other stuff doesn't have it and I'm not sure I see any benefit. This is just a property on a config entity, nothing more.

IMHO we should have a valid use case in the form of tests in core so that we can make sure it actually works and in that process if we have to fix/update somethings in the base class then we'll do it here and we don't have to wait for followup issues to fix that stuff.

I don't see the point in the change record. If people want to use it, they will look at the interface anyway. People don't have to update existing code.

If one has to look at the interface to understand it then IMHO the documentation is not covering everything.

Too many change records cause a big issue.

I don't think so. Big issue for whom?

This is just my opinion if you don't agree with it please go ahead :) I didn't block the issue on any of those items. It was merely a suggestion.

The test added there checks that no table gets created when the field storage is saved. I guess we could additionally check that you can create an entity, put a value in the field, save it without any error, and that the field comes up empty on next load.
I don't think there's much more to test here ?

Side note : is the clearCachedFieldDefinitions() call really needed ?

Thanks @damiankloip, looks good !

Nitpick : can we add a comment above the "tableExists" assert like : "Check that no table has been created for the field", and group the second part a bit more ? (better visual overview of the two things we're testing here)

Not a blocker obviously, so, RTBC.

Change record : well, yeah, let's create a tiny one to avoid controversy :-)

Thanks @damiankloip looks perfect now.

So the patch looks good, but don't we need an upgrade path to set the property to FALSE for existing fields?

re: update - yeah, that's probably right (discussed with @alexpott and he agrees). The new property has a default value so things would work if we didn't write an update hook, but then the yaml would change on next load/save, that would be slightly confusing ("But I didn't change anything ?")

- either we write a hook_update_N(), and directly read and write the raw config records (FieldStorageConfig::load() & ::save() are not allowed there)
- or we write a hook_post_update_NAME() that simply loads and saves the FieldStorageConfig entities (and the default value will automatically get added and saved) - like foreach (FSC::loadMultiple() as $storage) {$storage->save();}

Both work, the latter is probably simpler to write :-)

For me the latter is exactly what we added that hook for :)

Hm - then UpdatePostUpdateTest fails because it hardcodes the list of expected hook_post_update_NAME() implementations that exist, and explicitly adds the current one implementation we have in core, block_post_update_disable_blocks_with_missing_contexts().

It seems weird to have to update the expected list in UpdatePostUpdateTest each time we add a hook_post_update_NAME() ?

Left a note in #2538108: Add hook_post_update_NAME() for data value updates to reliably run after data format updates, but for now @catch agreed on IRC that we should just update UpdatePostUpdateTest here to make it pass...

Airport RTBC-if-green :-)

Hm, actually I just learned that post_update hooks were supposed to go in *.post_update.php rather than in *.install.inc
See #2578249-24: Some e_r fields get the wrong Selection handler and interdiff over there (one of those two patches will need a reroll after the other one gets in, obviously)

This does add a post_update.php file

HAH ! Indeed it does, I didn't even notice. Apparently I was the only one to not know about that. Don't mind me, sorry about the noise ;-)

Wrong permutation ;-). Yeah, UpdatePostUpdateTest is insane.

Moving to a syntax that better reflects the expected order :-)
I'll be bold and move back to RTBC, this and #79 do pass locally.

Committing this since it is a soft blocker for a contrib module (og) and is the lightest possible approach and has test coverage and an update function. Committed daad7b1 and pushed to 8.0.x. Thanks!

Awesome, thanks folks!

Published the change record.