Image module makes fatal assumptions about EntityInterface

Please read the actual code.

image_entity_presave

This function cares about "field_store_config" or "field_config" entity types, and will stop early in case a different entity type is passed in.
Both do have a settings property, so its not really clear what you try to achieve here :)

What @dawehner said, this is perfectly fine. ->settings is already specific to those classes, and those specific classes need methods to set settings.

I don't get what you are saying.

The function looks for a FieldStorageConfig entity, either directly or through the FieldConfig entity. Then it checks if it has one, and if the field type is image. If either is not TRUE, it returns and doesn't do anything.

It doesn't care about any entity type other than field_storage_config.

If it has one, then it accesses settings. Right now through the public property, if you made that protected and add methods specific to FieldStorageConfig (...Interface), then through those methods.

It could also check for the interface instead of entity type ID, but there is no difference in the end.

This might not be the nicest code, but it is perfectly fine.

An alternative implementation for this would be a helper function for the second part of the function and two specific hooks for field_config and field_storage_config that call the helper function. Maybe that would make it clearer but again, I'm very sure there is no functional bug here.

Sorry, but you are simply wrong.

This function only operations on field_storage_config entities. It returns early in case of anything else.

There can not be other entity types with the same entity type ID that do not have settings. Config entities have a fixed set of properties, that is defined through the schema, this can not and will not ever change. This is their API. We're adding methods and making their properties protected, but that does not change that field storage configs have settings and always will have.

Identifying an entity through the entity type ID is perfectly valid and is technically equivalent to doing an interface check, the only difference is that your IDE doesn't give you the correct type hint. As mentioned, it would probably be a good idea to refactor this with a helper method that type hints on FieldStorageConfigInterface and uses methods, but that would be a task. There is no bug here, now or in the future.

PS: Both @dawehner and myself are not native speakers. If anything we wrote offended you then I'm sorry, that was not our intention. But if we are both sure about something, then we are probably right ;)

That's just how the Entity system, and more specifically the hooks reacting to an entity lifecycle, work in D8.

For further proof : if you want to react to an event on a *specific* entity type, you use hook_[entity_type_id]_presave() - e.g hook_field_config_presave(). As @Berdir explained, the entity_type_id is the primary indicator for the entity type. From a given entity_type_id, you can assume class members or methods.

That code is correct, there's nothing to fix here.

@all

It would be great if we all calm down here. Being aggressive doesn't help anyone. Let's try to understand each other.

@mile23
So from my point of @yched and @berdir don't see an actual problem because its really not likely that you provide a different implementation
of field config / field storage config entities.

On top of that relying on the entity type ID is done EVERYWHERE in Drupal core, fixing it here, does not solve the problem you see,
even from my point of view, this is not a real existing problem. How much other total crazy problems with had, which could be also tackled.

In general some people are worried that you could spent your valuable time in different ways with higher impact. Everyone would be more happy with it, hopefully.

@Mile23 : sorry that you take it that way. The message is not "we don't give a shit", the message is simply "that code is correct, the whole of core works that way, and that is what contrib modules are expected to do too, nothing needs to be changed in image_entity_presave()".

image_entity_presave() checks whether it is fired on an entity that is either a field_config or a field_storage_config.
- if not, it does nothing and returns
- if yes, it does stuff on the "settings" of that entity, because it can confidently assume the entity has (currently) a public 'settings' property or (better, and that's the intent of #2030633: Expand FieldStorageConfig with methods) a setSettings() method.

Bottomline is : entity type ids are the entry point for code that receive "some entity, indistinctively", like hook_entity_[operation]().
The 'field_config' entity type, for example, is mapped to the FieldStorageConfig class.
You can use hook_entity_type_alter() to map it to a different, custom class of yours, but then that class *has* to extend FieldStorageConfig, or at least implement FieldStorageConfigInterface - or code will simply break. We won't be adding if(... instanceof ...) checks all over the place.

The latter (an entity_type_id is tied to an interface rather than an actual class) is better of course, and is the point of #2016679: Expand Entity Type interfaces to provide methods, protect the properties, as you know.
So #2030633: Expand FieldStorageConfig with methods will change image_entity_presave() to do $entity->getSettings() / ->setSettings() instead of working with $entity->settings. Meanwhile, I don't see what else there is to fix here.

After #2030633: Expand FieldStorageConfig with methods is done, the situation will be : checking an entity_type_id is enough to assume the interface of the $entity and work with its methods, without having to additionally check for that interface. That's what entity-type-specific variants like hook_[entity_type_id]_presave() are based on - and, BTW, you *will* find implementations in core that type-hint their $entity param on the interface of the entity type. You can do that in an entity-type-specific variant, but not in the generic hook_entity_presave(), by design.

That's the D8 entity system, and while I guess it can be discussed or argued, it's now way too late in the cycle to redesign it.

It's great and truly welcome that you're moving #2030633: Expand FieldStorageConfig with methods forward, thanks for that. But when the primary D8 contributor and two Entity API maintainers agree to disagree with you, you should consider giving them the credit that maybe they're not simply "not giving a shit" :-p

Thanks @yched.

To complete that, your initial example was perfect IMHO. The following line in EntityStorageBase::invokeHook() is how the entity type specific hook is called:

$this->moduleHandler()->invokeAll($this->entityTypeId . '_' . $hook, array($entity));

Then we implement a hook for example as:

function mymodule_node_insert(NodeInterface $node)

This IMHO explains the strict 1:1 relationship between interfaces and entity type IDs perfectly.

There are only very few reasons to replace an entity type class (There are *some*, like file_entity, but it is very rare). And if you do, then you are only allowed to extend the API, not remove things from it. And for content entities, their field definitions *are* part of the API and for config entities, it is their defined properties, through the schema. This is not an implementation detail, this is how the entity API works in 8.x and I would not know how to do it differently.

Entity classes/interfaces are very different from e.g. services behind an interface, which could have completely different implementations.

This is my last comment on this issue. And my last try to mark it closed.

OK, thinking about it while falling asleep, I finally got what the problem is :-)

Reformulating :

- image_entity_presave() has a need to read & write the "settings" of an $entity that can be either a field_config or a field_storage_config. It currently does so by directly manipulating the public 'settings' property that both those entity classes have.

- #2016679: Expand Entity Type interfaces to provide methods, protect the properties intends to move away from public properties in each per-entity-type classes, in favor of methods in each per-entity-type interfaces.
This will let code do: "entity_type_id == I can assume an Interface with methods"
instead of currently: "entity_type_id == I can assume a class with public properties".

- This task has separate sub issues for the actual entity types that remain to be converted.
For field_config and field_storage_config:
#2030633: Expand FieldStorageConfig with methods
#2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods
@Mile23 is working on those two issues - two separate issues, thus two separate patches currently.

- What each of those two patches does on the entity type it deals with is (among other things):
remove the 'settings' property from the public API by making it protected,
add a public setSettings() method to the class and the interface (both entity types already have the getter),
change existing code from using the property to using the getter/setter.

- But that specific code in image_entity_presave() runs on both entity types. So, when one patch removes the public property and tries using the setter for field_config, the setter doesn't exist yet on fied_storage_config, since it's only added in the other patch.

We have a "patch race condition", simply caused by the way those two tasks are currently shaped :-)
So it's merely a question of battle plan for those two issues.

Proposals :
a) Merge those two patches in one single issue. Makes sense IMO since field_config & field_storage_config are sibling entity types with sibling APIs that should stay consistent. Even in two separate patches, each issue needs to be coded and reviewed with the other in mind anyway.

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.

c) Variant : keep separate patches, and each patch temporarily changes image_entity_presave() to do

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
}

Same as above, minor overlap, and the last patch that gets in can re-unify in one single code branch when setSettings() exists on the two interfaces.

The three options work IMO, no strong preference on my side.
@Mile23, what do you think ?

I'll update the issue summary.

Using tags the tag for dx.
https://www.drupal.org/issue-tags

This issue was reviewed during a group triage meeting in #bugsmash. smustgrave and myself discussed the issue.

We read the issue, a big thank you to @YesCT for the Issue Summary update. Our understanding is that this can be postponed on #2396713: Figure out how to encapsulate writable settings supplied by DataDefinitionInterface. Once a decision is made there, then any changes needed to the image module can be done in this issue. I am updating the issue meta data accordingly.