Expand FieldStorageConfig with methods

Start to work on this ...

@japicoder Did you have any questions or intermediate work to post?

Postponed on #1953408: Remove ArrayAccess BC layer from field config entities

postponed now on #2095303: Rename 'field_entity' to 'field_config' and 'field_instance' to 'field_instance_config'

That issue seems to be resolved. Back to active.

List of public property's found via the FieldConfig API documentation:

• FieldConfig::$id
• FieldConfig::$name
• FieldConfig::$entity_type
• FieldConfig::$type
• FieldConfig::$module
• FieldConfig::$settings
• FieldConfig::$cardinality
• FieldConfig::$translatable
• FieldConfig::$locked
• FieldConfig::$indexes
• FieldConfig::$deleted

Renamed to match the new classname.

This is the first draft, changing only things in FieldConfig.php. As a consequence, properties are still public to see if the tests fail already. (See #14.)

Getters-setters table
('+' means added in the patch, '-' means none):

property; getter; setter
id; id(); -
name; getName(); +setName()
entity_type; getTargetEntityTypeId(); +setTargetEntityTypeId()
type; getType(); +setType()
module; +getModule(); +setModule()
settings; getSettings(); +setSettings() (also added a setSetting() method)
cardinality; getCardinality(); +setCardinality()
translatable; isTranslatable(); setTranslatable()
locked; isLocked(); +setLocked()
indexes; +getIndexes(); +setIndexes()
deleted; +isDeleted(); +setDeleted()

Not sure about

1. when to use the set() method in setters and when to just assign the value. I have found examples for both. For the former see CustomBlock::setInfo(). For the latter see FieldConfig::setTranslatable().
2. if getters and setters should be declared in the interface or not. If so, in which one: FieldConfigInterface or FieldDefinitionInterface?
3. who needs setters and who don't.
4. whether leaving $this->setters occurences this way is ok.

Patch passed tests, so let's replace other occurences of direct property access to getters and setters.

I found contradictory informations on whether the properties should be public or protected. The following patch sets all properties to protected. Also replaced all direct access to FieldConfig properties with getters and setters (ie. all that I found).

Note - berdir told not to make them protected so the patch in #12 is good.

So the patch in #12 needs review.

I'm sorry, but keeping the properties public completely defeats the purpose of this. The test failures on the protected version are showing very clearly that the job simply isn't finished.

Please make them protected and continue converting usages to use the new methods.

Needs a reroll of the patch from #14.

Will be working on this task; assigned via core mentoring.

I'm gonna attempt to tackle this.

My first patch. Hopefully it's not too ugly!

I should have included this info in comment 24 but forgot to. Here is a summary of my tasks with a few notes about things I did not do and why...

Changes:
- added setName to set $name
- added setType to set $type
- added setModule and getModule to set/get $module
- added setCardinality to set $cardinality
- added lock() and unlock() to handle $locked
- added setSetting to set an individual setting by key
- added setSettings to overwrite the entire settings value
- added setIndexes and getIndexes to set/get $indexes

Did Not:
- Did not create a setter for $id because it is made up of entity_type and name
- Did not create a setting for $entity_type because it is set in the constructor
- Did not do anything with $deleted because the documentation mention it is set by a delete() function but I don't see where that function is?
- Did not change all references to $this->name to use $this->getName() or $this->type to $this->getType. Since there is no additional processing happening this seems redundant to me.

- setName(), setType() : those informations are required in the constructor. Do we really want to add setters for those ?
- setModule() : this is derived from the field type, so not sure there should be a setter either ?

Working with Bart @ DrupalCon Austin Sprint. We talked through yched's comments and agree so I've updated the patch to reflect them.

I removed:

- setName()
- setType()
- setModule()

I have installed this patch successfully.

Talking at the drupalcon sprint with WebChick and found a few issues that need to be addressed. I am going to go back and re-roll the patch from #12 and then implement yched's comments into that. Also need to add some documentation to a few things.

This is just a re-roll of czigor's patch from comment #12.

I still need to apply yched's comments but wanted to get the re-roll submitted before tackling that. I'll continue to work on that if this passes the tests.

Forgot to mark as needs review to queue the test.

Remove setName(), setType(), and setModule()

Had a reference to the deleted setModule(). I fixed that.

Applied patch from #34. Applied without error, nothing blew up.
Enabled, disabled, and re enabled all field related modules.

Kicking back to needs review for a more in depth look.

Added type hinting where necessary and added comment to locked flag. Big thanks to YesCT for throwing this our way (my first patch!).
Shoutout to:
@AndyThornton
@jackbravo

This patch intentionally goes against the current standard of making properties public for testing purposes.

This patch will get tested by the testing system and test the assumptions about getters and setters in the rest of the code are correct and to identify any places in core that might need to be updated to use the new getters/setters.

I will submit a corrective patch right after it gets tested.

I need some advice on next steps here. There are definitely places where FieldConfig properties are referenced. The failed test points to line 1351 of ContentEntityDatabaseStorage which references deleted instead of using isDeleted(). But the test stops there after it has failed. Three questions I have...

A) For this patch (#38) to be accepted do all the places that reference it as a public property also need to be updated?

B) If so, I have not used SimpleTest before but I assume there is a way for me to get all failures not just the first failure? Otherwise uploading a patch to fix one issue just to wait for test bot is going to be painful. I guess I need to learn simpletest and SimplyTest.me to move this forward.

C) Also, the method in ContentEntityDatabaseStorage type hints a FieldConfigInterface object for the $field it works with. FieldConfigInterface is fairly sparse, only defining isLocked and getBundles. Should we augment it to have a isDeleted(), and potentially other functions, as well?

Yeah, I'd tend to think this patch doesn't have to convert all direct accesses to the public properties

I have uploaded patch from 38 renamed to match this comment number as it was the last passing (and should still pass) patch.

I am not uploading an interdiff because it is the negation of the last interdiff, if that makes sense. If that's incorrect procedure let me know.

I am setting this to needs review to kick off test bot and move it along.

If we want to create a separate issue to change the direct-access to use setters who would do that? I'd like to take a stab at it if that's ok?

@carsonevans

If we want to create a separate issue to change the direct-access to use setters who would do that? I'd like to take a stab at it if that's ok?

Sure, go for it :-) You can create the issue yourself (tag it with "Novice, Entity Field API"), and link to it in a comment here ?

I created the issue at #2282299: Accessing FieldConfig properties should be done through methods.

The patch needed to be re-rolled because of format_string was changed to String::format on lines 301, 311, 317, 467

I re-rolled it but couldn't figure out how to do an interdiff that just had the changes that were conflicting.

I read through all the lines in this patch. It looks ok to me.
I have not looked at the class to see if any properties are missing getters and setters.

Here is a table with class properties and associated methods and notes about each to help reviewers. The elements in bold are the ones that have been added in this series of patches.

EDIT: Deleted Table. Reproduced in #51 below with more recent data.

There is no need for setTargetEntityTypeId() because that is also set in the constructor and can not be changed.

Instead of setLocked(), it would also be possible to have lock() and unlock(), not sure what's more clear.

Not sure about getIndexes().. if you look at the documentation, it's never accessed directly from the outside but always only merged into what is returned by getSchema(). I suggest you removed getIndexes() as it could be misleading (does it only return custom indexes or also those defined by the field type?)

New methods should be documented on the interface (FieldConfigInterface) and the implementation should only refer to it with {@inheritdoc}

All new methods have now been added to the FieldConfigInterface. setTranslatable() was also added to FieldConfigInterface even though it was not introduced in this patch.

Here is a table with class properties and associated methods and notes about each to help reviewers. The elements in bold are the ones that have been added in this series of patches. The notes try to compile most of the previous comments into why we have or haven't added certain getters and setters.

Whoops. Left a getIndexes() in after removing it per #50.

Interdiff-51-53.txt will show the specific error fixed in this latest patch.

Interdiff 46-53.txt is the full meat of the interface and documentation updates that incorporate bedir's comments.

53: 2030633-expand-fieldconfig-and-interface-with-methods-53.patch queued for re-testing.

LanguageFallbackTest.php failed the first time through. I scanned it but didn't see how it affected FieldConfig so I just re-queued the test. Second time through it passed. I'm not sure if that should be worrisome or not.

Looks good to me. Remember to set the issue back to needs review.

Patch in #53 no longer applies. Please review updated patch which applies cleanly.

Interdiff failed to create interdiff.txt file so I am not attaching it.

FieldConfig is now FieldStorageConfig.

Retitling.

Re-rolling #64 for FieldStorageConfig and latest checkout.

The entity variables are not protected. They must be. The idea of encapsulation is that you cannot access the variables directly. You must use the functions getVariableName() and setVariableName(). If the variable is a boolean you have the function isVariableName().

The patch is almost a month old so also a reroll.

The entity variables are not protected. They must be.

Is it what was done in the other issues in #2016679: Expand Entity Type interfaces to provide methods, protect the properties ? Most properties of ConfigEntities are still public, aren't they ?

@yched: As far as I know is that the variables must be protected. The whole idea for doing is so is that we have encapsulation of the entity variables. Please correct me if I am wrong.

This is what alexpott said in #2030645: Expand Menu with methods comment #31:

Reclassifying as a bug since exposure of properties as public allows code to not use the API eg ->id vs ->id(). This makes Drupal more stable and prevents bugs. Nice work.

@daffie: OK, right, this is the new direction for #2016679: Expand Entity Type interfaces to provide methods, protect the properties.
Never mind my #71, #70 is correct.

The most important problem is that the class variables are not protected. The whole idea of adding functions to get and set the class variables is the principle of encapsulation.

A minor issue:

+++ b/core/modules/field/src/FieldStorageConfigInterface.php
@@ -31,4 +31,92 @@ public function getBundles();
+  /**
+   * Sets the field config deleted property.
+   *
+   * @param bool $deleted
+   *   The field config deleted property.
+   *
+   * @return $this
+   */
+  public function setDeleted($deleted);
+

You need to change @return $this to

  * @return \Drupal\field\FieldStorageConfigInterface
  *   The called FieldStorageConfig entity.

@return $this is just fine: https://www.drupal.org/coding-standards/docs#types

Here's a re-roll of #69.

Let's fix that re-roll... :-)

@Mile23: The class variables are still public. They must be protected.

needs tests, and I think one of the methods is missing a doc block.

We should also think about wether we actually need all these methods, so a reviewer (or someone) should see if/where (in tests?) the methods are used.

this will be an api change.
making the properties protected allows us to make sure we replace all the usages of direct access to them.
And else where we decided that public properties were a bug... so I think we should have the properties protected in this issue also (not sure if the meta is up to date will all this policy/process changes). (See the issue summary in the meta)

OH. this had a follow up to replace all the direct accesses...
#2282299: Accessing FieldConfig properties should be done through methods
Which means... we would have to leave the properties public.

What a mess.

Need to see how a recent one of these issues went in.

moving this to bug similar to #2030645-31: Expand Menu with methods

and closing #2282299: Accessing FieldConfig properties should be done through methods since we will make the properties protected here.

also updating the issue summary.

https://www.drupal.org/core-mentoring/novice-tasks
The whole issue is not novice, but the next step can be:
making the properties protected.
Make a new patch and add an interdiff. Set to needs review to trigger the test bot. (We know there is more to do here than just that, but it's ok to make little steps of progress.)

@YesCT: #2030661: Expand Tour with methods landed 4 days ago and #2030613: Expand EntityViewMode (really EntityDisplayModeBase) with methods landed 5 days ago. Alexpott wants this fixed because he thinks that the exposure of properties as public allows code to not use the API eg ->id vs ->id(). "This makes Drupal more stable and prevents bugs." (see comment #72)

And yes we can add tests, but is that really necessary for getters and setters?

Oh right, if they are just getters and setters, then tests might not be needed. (I googled, and the internet is full of opinions on if getters and setters need to be unit tested.) Maybe just keep your eye out for any methods that are more complicated that might need tests, and update issue summaries to point them out.

updating the summary to take adding tests out of the remaining tasks.

I attacked this problem a bit yesterday and it's a huge refactoring task. It's not like FieldStorageConfig is used by *everything.* :-)

There's no way it's a novice task, and it will be heck to review.

Also, it's not a needs-tests issue, because the tests tell you whether you're on the right path or not. Do not add tests at this point; they'll only be confusing.

I'd suggest going through and changing one property to protected, then make all the tests pass. Then change the next property to protected and make all the tests pass, and so forth. There's no way to parcel this out as multiple issues because of the way some code deals with these properties... Patches would overlap in irreconcilable ways. It might be possible for participants to change one or two properties at a time and then post a patch with an interdiff. Then the next person can work on another property or two after the testbot passes the previous patch.

The relevant tests during this refactoring will be the ones related to field module. Then of course run all of them and see if you're really done. :-)

Ideally, someone who thought these properties should be public would be the one to go through and do this. [evil grin]

Before we start making the patch. I would like us to agree on a few points:

1. I would like that we all agree on which getter and setter functions will be created.
2. There will be not tests added.
3. Because it is a huge task. Can we make an agreement over who does what. I am afraid that if we take to long between creating the patch and getting it committed, another issue lands that make our patch needing a reroll.

Maybe a stupid question but can this issue be do with one patch file or do we need multiple patch files?

If alexpott is the one who is going to commit this patch, I would also like a confirmation from him about this.

1. The getters and setters are already created in the current patch. You can see the discussion about it above. (This is comment #89, after all...)
2. Yup.
3. If someone wants to take it on, they can say, "Hey, I'm working on this." Or assign themselves to the issue.

opps. you are totally right, I meant to only make the task to make them protected a novice task.
changing all the usages is not novice.
updating the issue summary.

@Mile23: I know that there are in your patch. What I really would like is that alexpott signs off on that. So that when the patch is ready and RTBC, we do not get in the discussion over which getter and setter function should or should not be in the patch. With the result that that will take some time and then we need to reroll the patch.

This is my first collaboration to an issue, I have changed the $deleted to protected.

Thank you @fernando_calsa

Setting to needs review to trigger the testbot. (d.o was making @fernando_calsa wait, thinking the new account might be spamming the comments. I gave them the not the spammer role, which should help for the future.)

@daffie I dont think we should have to clear it with a committer right now, Berdir or yched should be fine (or other person from Maintainers.txt for field stuff).
I think we should read the previous comments from Berdir and yched (if there are any) in this issue about the naming of methods.
And we should put the table in #51 in the issue summary.

I can also look more in depth at that and see what I think about the names and methods, but I would have to read all the comments on the issue and think about it... so let's at least do that before we ping a committer (or Berdir or yched).

Just a start at summarizing what methods should or shouldn't be in this patch, by copying the table from #51 into the summary.

updated the summary remaining tasks to indicate the novice part is done (for now).
also added a column to note change to the property visibility that this patch does.

oops. just a copy paste mistake.

In the issue summery we added the table from comment #51. The table is a proposal for which getter and setter functions to add and not to add to the FieldStorageConfig class. Can one of the field system component maintainers or a branch maintainer sign of on this. This is going to be a big patch and a lot work. In some other sub-issues of #2016679: Expand Entity Type interfaces to provide methods, protect the properties we are getting in a discussion when the patch is made and RTBC over which getter and setter functions to include. The problem for this issue is that that will result in a reroll and a lot of extra work. So I really want an agreement over which getter and setter functions will be included.

Talked with alexpott on IRC and think that most functions are oke. But he want to remove setDeleted() and setIndexes(). I have updated the issue summary.

Note that some settings stuff might be solved over here: #2396713: Figure out how to encapsulate writable settings supplied by DataDefinitionInterface

This patch makes all properties of FieldStorgeConfig protected except for: entity_type, field_name, settings, and type.

It removes setDeleted() and setIndexes() from previous patches (and rightly so, I'd wager).

It passes the field module's tests, to the lazy extent that I worked on that, but let's see what the testbot says.

Update: aw crap, I named the interdiff with .patch. Sigh.

Found a lot more. This might fail testing, but it shouldn't have fatal errors.

Fixing a few things... More later as time permits.

Did you know that Drupal\field_ui\Tests\ManageFieldsTest takes 15 minutes to run locally?

This should fix all the test failures since #105.

This patch makes all properties of FieldStorgeConfig protected except for: entity_type, field_name, settings, and type.

I'll get those encapuslated later.

Add $settings to the list of protected properties.

And now, a blocker: #2398901: Image module makes fatal assumptions about EntityInterface

Thank you @Mile23 for moving these issues forward. You have done a lot on these issues.

in
#2398901-16: Image module makes fatal assumptions about EntityInterface
@yched suggests 3 ways forward.

I'm in favor of:

b) Keep separate patches, and each patch temporarily adds setSettings() to the other interface.
Minor overlap, perfectly acceptable, will just mean a reroll when the first one gets in.

--

Unassigning so it is clear anyone can try to do that.
@Mile23 if you start to try that, just make a comment here, so we can avoid two people both doing it.

See also #2030637-39: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods

Ok, here is a patch that fixes image_entity_presave(), without needing anything more than we already have here.

Notes:
- This does not contradict what I said in [#9466051], I switched to interface checks as that gives better IDE integration, but it would work just as well with checking the entity type ID.
- This method works with exactly two entity types/objects/interfaces. field configs and field storage configs. I'm making it a bit more explicit, but not changing any actual logic, it already did exactly this before.
- There is a fair amount of old code that has only been partially ported. For example, loading the field storage is not necessary, we already have getType() on both interfaces for this. And the default settings are also applied automatically, so we can drop that.
- We can just drop the if/else at the end in #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods, once both interfaces have a setSetting() method.
- This is IMHO better than having a common interface, because a generic settings interface would be meaningless. We know that those two entity types have settings and that we can change them. A generic (entity or data definition) interface with setters for settings does not tell you anything about what kind of settings it includes. That's just an object-version of an array. Also note that FieldStorageConfig does implement DataDefinitionInterface, #2396713: Figure out how to encapsulate writable settings supplied by DataDefinitionInterface would us help nothing here.

If you have questions, ping me in IRC, I think that is a better place to discuss this through than the issue queue.

This does not contradict what I said in [#9466051], I switched to interface checks as that gives better IDE integration, but it would work just as well with checking the entity type ID.

Disagree, but thanks. :-)

I can live with the approach of #119, assuming tests pass.

Still to go on this issue: Encapsulating $entity_type, $field_name, and $type.

Fixing failures.

That line of code would have worked in PHP 5.5. That is why the IDE didn't report it. Fixing it for PHP 5.4.

Fixing more failures.

Fixing more failures. The exception is fixed. Let's see if we find any more places accessing the property directly.

Fixing the typo and another use of settings property. I think that some of the failures might be random. Let's see if they come up again.

Fixing the problem before moving on. I am sorry for so many interdiffs and patches. :)

Moving on with $type property. I am beginning to think it would be better with these as separate issues. The interdiff alone is 15 KB and I am replacing only those that I found with a simple regex. I am sure tests will tell us more.

It seems there was only one cause for all the errors. Let's see.

Moving on to $entity_type property. I am replacing it with getEntityTypeId() method wherever I found it. On to the testbot.

You need to use getTargetEntityTypeId(), get EntityTypeId() is the entity type of your own entity (field_storage_config)

@Berdir: I actually meant getTargetEntityTypeId() in my previous comment (#136). But it looks like I have used getEntityTypeId() in some places. Fixing that now.

Fixing the method calls as per #138.

The last property now: $field_name. I am replacing it with getName() method wherever I found it. May the testbots smile upon it!

It's been 5 hours and it still shows "Test request sent". I know this is not normal but does this mean that we should initiate a retest somehow?

Fixing the failure.

Yay! All properties are now protected and all tests are passing. I don't think there is anything else left to do except review.

Good work hussainweb!

+++ b/core/modules/field/src/Entity/FieldStorageConfig.php
@@ -288,30 +288,36 @@ protected function preSaveNew(EntityStorageInterface $storage) {
+    // Make sure all settings are present, so that a complete field
+    // definition is passed to the various hooks and written to config.
+    $this->settings += $field_type_manager->getDefaultStorageSettings($this->getType());
+
...
+    // @todo Or this? (taken from patch at https://www.drupal.org/node/2030633#comment-9015925)
+    // $entity_manager->getStorage($this->getTargetEntityTypeId())->onFieldStorageDefinitionCreate($this);

@@ -353,6 +359,8 @@ protected function preSaveUpdated(EntityStorageInterface $storage) {
+    // @todo Or this? (taken from patch at https://www.drupal.org/node/2030633#comment-9015925)
+    // $entity_manager->getStorage($this->getTargetEntityTypeId())->onFieldStorageDefinitionUpdate($this, $this->original);

Why is this code necessary for this issue?

+++ b/core/modules/field/src/Entity/FieldStorageConfig.php
@@ -481,10 +491,11 @@ public function getColumns() {
-    if (empty($this->deleted)) {
+    $deleted = $this->isDeleted();
+    if (empty($deleted)) {

Why not do "if ($this->isDeleted()) {"

+++ b/core/modules/field/src/FieldStorageConfigStorage.php
@@ -135,13 +135,28 @@ public function loadByProperties(array $conditions = array()) {
-          case 'uuid';
+          case 'uuid':
...
+          case 'deleted':
+            $checked_value = $field->isDeleted();
+            break;
+
+          case 'module':
+            $checked_value = $field->getModule();
+            break;
+
+          case 'type':
+            $checked_value = $field->getType();
+            break;
+
+          case 'field_name':
+            $checked_value = $field->getName();
+            break;

Good find!!!

+++ b/core/modules/field/src/Tests/FieldCrudTest.php
@@ -191,9 +191,10 @@ function testDeleteFieldCrossDeletion() {
+    $field_name = $field_storage->getName();
...
-    $this->assertFalse(FieldConfig::loadByName('entity_test', $this->fieldDefinition['bundle'], $field_storage->field_name));
-    $this->assertFalse(FieldConfig::loadByName('entity_test', $field_definition_2['bundle'], $field_storage->field_name));
+    $this->assertFalse(FieldConfig::loadByName('entity_test', $this->fieldDefinition['bundle'], $field_name));
+    $this->assertFalse(FieldConfig::loadByName('entity_test', $field_definition_2['bundle'], $field_name));

@@ -202,10 +203,11 @@ function testDeleteFieldCrossDeletion() {
+    $field_name = $field_storage->getName();
...
-    $this->assertTrue(FieldStorageConfig::loadByName('entity_test', $field_storage->field_name));
+    $this->assertTrue(FieldStorageConfig::loadByName('entity_test', $field_name));
...
-    $this->assertFalse(FieldStorageConfig::loadByName('entity_test', $field_storage->field_name));
+    $this->assertFalse(FieldStorageConfig::loadByName('entity_test', $field_name));

I think that directly using $field_storage->getName() is better.

+++ b/core/modules/field_ui/src/Form/FieldStorageEditForm.php
@@ -194,7 +194,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+      $field_storage->set($key,$value);

Nitpick: add a space between $key, and $value.

+++ b/core/modules/image/image.module
@@ -321,49 +322,52 @@ function image_filter_keyword($value, $current_pixels, $new_pixels) {
 function image_entity_presave(EntityInterface $entity) {

This is not solvable. There is problem with the function image_entity_presave(). See comment https://www.drupal.org/node/2398901#comment-9466051. I think the best option is to merge #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods with this issue. Which I did.

This is not solvable

It is solved or at least will only require a minor update in #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods, which we should postpone on this. Merging the patches will probably double the patch size of this issue again.

Please use the solution from #2398901: Image module makes fatal assumptions about EntityInterface:

if ($entity_type == 'field_config') {
  use the correct code (property or method) to manipulate the settings
}
else {
  use the correct code (property or method) to manipulate the settings
}

In #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods shall we remove this construction.

The patch is using interfaces for that, which is the same.

To explain the change from comment #150 a little bit more. Change in file image.module on line 343 in the function image_entity_presave() to:

if ($entity_type == 'field_storage_config') {
  $fid = $entity->getSetting('default_image')['fid'];
}
else {
  $fid = $entity->settings['default_image']['fid'];
}

In #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods shall we remove this construction. After that can we easily commit the interface from #2398901: Image module makes fatal assumptions about EntityInterface.

Again, image_entity_presave() is already perfectly fine as far as this issue goes, both objects have a getSetting(), only setSetting() is missing on field_config, and that will be trivial to update in #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods. And I still insist that #2398901: Image module makes fatal assumptions about EntityInterface is going in a wrong direction and is a won't fix.

1. +++ b/core/modules/field/src/ConfigImporterFieldPurger.php
   @@ -140,7 +140,7 @@ public static function getFieldStoragesToPurge(array $extensions, array $deletes
        foreach ($field_storages as $field_storage) {
   -      if (!in_array($field_storage->module, $providers)) {
   +      if (!in_array($field_storage->getModule(), $providers)) {
            $storages_to_delete[$field_storage->id()] = $field_storage;
          }
   

   Confused why this is still module and not provider. Also fun fact: It is "core" for field types defined in Drupal\Core\Field :)

   But that has nothing to do with this issue, *except* that we might want to name the method getProvider() instead. Not sure.

2. +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
   @@ -288,30 +288,36 @@ protected function preSaveNew(EntityStorageInterface $storage) {
        $this->module = $field_type['provider'];
    
   +    // Make sure all settings are present, so that a complete field
   +    // definition is passed to the various hooks and written to config.
   +    $this->settings += $field_type_manager->getDefaultStorageSettings($this->getType());
   +
        // Notify the entity manager.
        $entity_manager->onFieldStorageDefinitionCreate($this);
   +    // @todo Or this? (taken from patch at https://www.drupal.org/node/2030633#comment-9015925)
   +    // $entity_manager->getStorage($this->getTargetEntityTypeId())->onFieldStorageDefinitionCreate($this);
      }
   

   As @daffie mentioned, this is strange. I would like to see what exactly fails without adding this, can we have a patch without those changes and then see what happens?

3. +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
   @@ -353,6 +359,8 @@ protected function preSaveUpdated(EntityStorageInterface $storage) {
        $entity_manager->onFieldStorageDefinitionUpdate($this, $this->original);
   +    // @todo Or this? (taken from patch at https://www.drupal.org/node/2030633#comment-9015925)
   +    // $entity_manager->getStorage($this->getTargetEntityTypeId())->onFieldStorageDefinitionUpdate($this, $this->original);
   

   Same here, no need to change anything here, so drop this.

4. +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
   @@ -481,10 +491,11 @@ public function getColumns() {
   -    if (empty($this->deleted)) {
   +    $deleted = $this->isDeleted();
   +    if (empty($deleted)) {
   

   yes, as mentioned above, if (!$this->isDeleted()) should be enough here.

5. +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
   @@ -540,6 +565,22 @@ public function getSetting($setting_name) {
   +    $this->settings[$setting_name] = $value;
   ...
   +    $this->set('settings', $settings);
   
   @@ -591,6 +632,14 @@ public function getCardinality() {
   +    $this->set('cardinality', $cardinality);
   
   @@ -626,6 +675,14 @@ public function isLocked() {
   +    $this->set('locked', $locked);
   

   Why are some methods methods using $this->set() and others not?

   This should be unified. I'm not sure if it as a good idea, but #2399965: Config entity get() and set() should invoke getters/setters wants to change set() so that it calls e.g. setModule(), so calling set() would result in a loop and we should probably access the property directly.

6. +++ b/core/modules/field/src/FieldStorageConfigInterface.php
   @@ -39,4 +39,72 @@ public function isLocked();
   +   * @return bool
   +   *   The deleted property.
   

   I don't think this should talk about a property, just write something like "Returns TRUE if the field was deleted, FALSE otherwise" or so.

7. +++ b/core/modules/field/src/FieldStorageConfigInterface.php
   @@ -39,4 +39,72 @@ public function isLocked();
   +   * Sets field settings by overwriting the settings array.
   

   "array" is an implementation detail, I'd write something like "Sets field settings (overwrites existing settings)."

8. +++ b/core/modules/field/src/FieldStorageConfigInterface.php
   @@ -39,4 +39,72 @@ public function isLocked();
   +   * @param string $settings
   +   *   The array of field settings.
   

   string is wrong.

9. +++ b/core/modules/field/src/FieldStorageConfigInterface.php
   @@ -39,4 +39,72 @@ public function isLocked();
   +  /**
   +   * Returns the name of the module providing the field type.
   +   *
   +   * @return string
   +   *   The name of the module that provides the field type.
   +   */
   +  public function getModule();
   

   As mentioned above, we use almost everywhere "provider" instead of module now, so maybe we should use that for the method name.

10. +++ b/core/modules/field/src/FieldStorageConfigStorage.php
    @@ -135,13 +135,28 @@ public function loadByProperties(array $conditions = array()) {
    +          case 'uuid':
                 $checked_value = $field->uuid();
                 break;
     
    +          case 'deleted':
    +            $checked_value = $field->isDeleted();
    +            break;
    +
    +          case 'module':
    +            $checked_value = $field->getModule();
    +            break;
    +
    +          case 'type':
    +            $checked_value = $field->getType();
    +            break;
    +
    +          case 'field_name':
    +            $checked_value = $field->getName();
    +            break;
    +
               default:
                 $checked_value = $field->$key;
    -            break;
             }
    

    This could also use $field->get($key), I'm not sure if this list is really complete. Because the default will definitely not work anymore.

11. +++ b/core/modules/field/src/Tests/FieldStorageCrudTest.php
    @@ -363,7 +363,11 @@ function testUpdateFieldType() {
    -      $field_storage->type = 'integer';
    +      // Set the field storage type to 'integer.' Since $type is protected,
    +      // we have to use reflection.
    +      $ref_field_storage_type = new \ReflectionProperty($field_storage, 'type');
    +      $ref_field_storage_type->setAccessible(TRUE);
    +      $ref_field_storage_type->setValue($field_storage, 'integer');
           $field_storage->save();
    

    No, we do not, set('type', 'integer') works too.

12. +++ b/core/modules/field/src/Tests/reEnableModuleFieldTest.php
    @@ -39,6 +39,8 @@ protected function setUp() {
    +   *
    +   * @see \field_system_info_alter()
    

    We usually do not prefix functions with a \.

13. +++ b/core/modules/system/src/Tests/Entity/FieldSqlStorageTest.php
    @@ -411,15 +411,23 @@ function testFieldUpdateIndexesWithData() {
    -    // Add an index.
    -    $field_storage->indexes = array('value' => array(array('value', 255)));
    +    // Add an index. Since $indexes is protected, we have to use reflection.
    +    $ref_field_storage_indexes = new \ReflectionProperty($field_storage, 'indexes');
    +    $ref_field_storage_indexes->setAccessible(TRUE);
    +    $ref_field_storage_indexes->setValue(
    +      $field_storage,
    +      array('value' => array(array('value', 255)))
    +    );
    

    Same, use set() Or add a setter, because this seems like a valid thing that you might want to set?

re: #153.1 :
$storage->module / $storage->getModule() :

we might want to name the method getProvider() instead. Not sure.

+1. This issue is breaking BC anyway, we might as well move to something meaningful. Could even be getFieldTypeProvider(), for clarity. This is not the provider of the "storage definition".

re: #153.5 :

@@ -626,6 +675,14 @@ public function isLocked() {
    +    $this->set('locked', $locked);

Why are some methods methods using $this->set() and others not?

Noticed the same pattern in #2030607: Expand EntityDisplayBase with methods, was wondering about that too.

@yched: Thanks for the +1 ;) Do you know a reason why we couldn't get the provider from the plugin definition directly instead of persisting it? I understand that was convenient to have earlier, but when it is a method, it shouldn't matter? that's not a change that belongs in this issue of course, just wondering.

Some of these issues use set() and some don't because it's completely unclear when one should or shouldn't use set().

set() has side-effects. Are they necessary? Is it an API or just something that's there for some reason?

@Berdir #155:

Do you know a reason why we couldn't get the provider from the plugin definition directly instead of persisting it?

Good question.
IIRC, we persisted it in the field definition to be able to know it in case the providing module went missing (disabled, etc)...
I think that comes from CCK, and was kept in the D7 Field API port.

Later in the D7 cycle, "prevent disabling modules that provide field types in use" was added, and in D8 that area moved further with config dependencies and stuff. So it might be that we could stop saving it in the config files now, yes.

Not directly related to the current patch, though, so it's probably best as a followup / separate issue - opened #2407463: Stop storing the provider module in FieldStorageConfig yamls.

As for this issue here : the current uses of $field_storage->getModule() evidenced by the current patch show that it's still handy to have a method to access it from the $field_storage, so the said followup would still keep the method.
Thus, still +1 on giving it a clearer name like getFieldTypeProvider() in this patch here.

Lot of reviews to work on! I am starting with a reroll. It was quite a massive reroll considering that it is just 15 days. There were conflicts to setting translateable on fields, better loading dump files in migrate tests, #2357801: File field default values are not deployable -- use UUIDs for the default file and some smaller ones.

Setting it to needs review to let this run through the testbot. I will set it to needs work once tests run and will then work on the reviews. I am also adding "Needs issue summary update" as it seems that some of the comments above will need it.

I was afraid I'd miss something. Trying again.

Sorry, it was the exact same patch. Gotta focus.

Fixing the error (and in other places). I am hoping there won't be many like these as it is not long since the last pass.

Looks good. Setting to needs work for review points.