Problem/Motivation

There are a few related problems with the FieldItemInterface::insert() and FieldItemInterface::update():

  • They are invoked, together with FieldItemInterface::delete() and FieldItemInterface::deleteRevision() from within SqlContentEntityStorage, which handicaps the development of alternative storage engines.
  • Their behavior is inconsistent with the corresponding hooks: while the hooks are called after entity insert/update, field item methods are invoked before. The rationale behind this behavior, is that field items may need to act before field values are written to perform some final massaging, but they may also need to know the entity identifier. However supporting this use case is no longer possible in D8, because with the default SQL storage we store base field values while allocating the entity ID, during the initial base table write. In fact at the moment, we have an inconsistent behavior between shared table fields and dedicated table fields: field methods are run after write for the former, and before write for the latter. This is a critical flaw and the only actual difference between them and FieldItemInterface::preSave(), which makes sense only for the default SQL storage.
  • FieldItemInterface::delete() is called before deletion, while the corresponding hook is invoked after deletion. Additionally, there is a pre-delete hook which is invoked before deletion, but there is no corresponding field method. A similar problem exists for FieldItemInterface::deleteRevision().

Proposed resolution

  • Remove the ::update() and ::insert() methods, since they happen to be currently broken for base fields, and replace them with a ::postSave() method that will act after field values are written to the storage.
  • The ::postSave() method will return a value telling the storage handler whether its item values should be rewritten to the storage. This will cover those (rare) use cases needing the entity ID to implement massaging for their item values.
  • Move the method trigger points from SqlContentEntityStorage to ContentEntityStorageBase.

Remaining tasks

  • Find consensus around a solution for the FieldItemInterface::delete() and related
  • Validate the proposed solution
  • Write a patch
  • Reviews

User interface changes

None

API changes

FieldItemInterface::insert() and FieldItemInterface::update() are removed.

interface FieldItemInterface {
  ...
 /**
   * Defines custom post-save behavior for field values.
   *
   * This method is called during the process of saving an entity, just after
   * values are written into storage.
   *
   * @param bool $update
   *   (optional) Whether the parent entity is being updated or inserted.
   *   Defaults to the former.
   *
   * @return bool
   *   Whether field items should be rewritten to the storage as a consequence
   *   of the logic implemented by the custom behavior.
   */
  public function postSave($update = TRUE);
  ...
}
interface FieldItemListInterface {
  ...
  /**
   * Defines custom post-save behavior for field values.
   *
   * This method is called during the process of saving an entity, just after
   * values are written into storage.
   *
   * @param bool $update
   *   (optional) Whether the parent entity is being updated or inserted.
   *   Defaults to the former.
   *
   * @return bool
   *   Whether field items should be rewritten to the storage as a consequence
   *   of the logic implemented by the custom behavior.
   */
  public function postSave($update = TRUE);
  ...
}
Files: 
CommentFileSizeAuthor
#122 field-item_callbacks-2478459-122.patch64.49 KBplach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,354 pass(es). View
#100 field-item_callbacks-2478459-100.patch45.08 KBplach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,072 pass(es). View

Comments

mkalkbrenner’s picture

Status: Active » Needs review
FileSize
1.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,836 pass(es). View
mkalkbrenner’s picture

Issue summary: View changes
mkalkbrenner’s picture

FileSize
400 bytes
1.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,844 pass(es). View

removed whitespace

Berdir’s picture

This does change when it is called, but the old place seems to be rather strange anyway (*before* saving the data).

mkalkbrenner’s picture

@Berdir: I recognized that as well. But the old position simply seemed wrong to me too, because it was not in sync with the corresponding hooks 'insert' and 'update'. These are already called *after* save. For me it makes sense to synchronize these calls. But I wanted to see first if the existing tests will pass.

mkalkbrenner’s picture

Assigned: mkalkbrenner » Unassigned
chx’s picture

FileSize
2.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Do. Or do not. There is no try.

Status: Needs review » Needs work

The last submitted patch, 9: 2478459_9.patch, failed testing.

mkalkbrenner’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -183,8 +183,20 @@ protected function invokeTranslationHooks(ContentEntityInterface $entity) {
+      'predelete' => 'preDelete',
...
+      'translation_insert' => 'insertTranslation',
+      'translation_delete' => 'deleteTranslation',

preDelete, insertTranslation, deleteTranslation are currently not part of the FieldItemInterface. Therfore we have two options: add them to the interface or remove them from the mapping.
If we enhance the interface, are there use-cases for these methods we can think of to justify that API change?
(I can think of backup on preDelete and notification on delete)

chx’s picture

Oh yeah we need to add to the interface and an empty to the base class, sure. There's no reason not to have them, that's my train of thought. Someone in the Drupal community will find a usage for them, guaranteed.

mkalkbrenner’s picture

Assigned: Unassigned » mkalkbrenner

@chx: I'm already working on it ... ;-)

chx’s picture

Thank you!

mkalkbrenner’s picture

Status: Needs work » Needs review
FileSize
6.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,347 pass(es), 670 fail(s), and 91 exception(s). View

Let's try this one ...

Status: Needs review » Needs work

The last submitted patch, 15: 2478459_13.patch, failed testing.

mkalkbrenner’s picture

Status: Needs work » Needs review
FileSize
11.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,830 pass(es), 33 fail(s), and 33 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 17: 2478459_17.patch, failed testing.

mkalkbrenner’s picture

Status: Needs work » Needs review
FileSize
14.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,930 pass(es). View
mkalkbrenner’s picture

Title: field item insert and update are only called for SQL » field item callbacks are only called for SQL storage and field API is incosistent with hooks
Issue summary: View changes
mkalkbrenner’s picture

Issue summary: View changes
mkalkbrenner’s picture

Title: field item callbacks are only called for SQL storage and field API is incosistent with hooks » field item callbacks are only called for SQL storage and field API is inconsistent with hooks
Berdir’s picture

+++ b/core/modules/field/tests/modules/field_test/src/Plugin/Field/FieldType/TestItem.php
@@ -106,8 +106,16 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
+   */
   public function delete() {
-    // Reports that delete() method is executed for testing purposes.
+    // Reports that preDelete() method is executed for testing purposes.

that looks like an accidental change ;)

Looks fine otherwise I think, but would still be good to have some feedback from @yched.

#2023473: Merge FieldItem::insert() and FieldItem::update() on fields into preSave() is somewhat related, where we tried to get rid of those methods. Looks like this issue is doing the opposite as it will then actually be different & useful to have.

mkalkbrenner’s picture

FileSize
14.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,938 pass(es). View
669 bytes

that looks like an accidental change ;)

You're right. Was a copy and paste error.

yched’s picture

Not sure I get the interest of having insert() / update() run *after* the values have been written.

The main intent is to act on the values before they are written, which would then need to be coded in preSave(). But then :
- not sure what's the use case for keeping update() and insert() if they are "post" ops,
- the code that needs to differenciate between an insert and an update needs to tell the difference themselves in the unified preSave(), but the discussion in #2023473: Merge FieldItem::insert() and FieldItem::update() on fields into preSave() seemed to imply that using isNew() is problematic at that point ?

mkalkbrenner’s picture

The main intent is to act on the values before they are written, which would then need to be coded in preSave()

You already have to do so, because 'update' and 'insert' are in fact called between or let's say already closer to after than before:

$insert_id = $this->database
  ->insert($this->baseTable, array('return' => Database::RETURN_INSERT_ID))
  ->fields((array) $record)
  ->execute();
...
$record->{$this->revisionKey} = $this->saveRevision($entity);
....
$this->saveToSharedTables($entity);
...
$this->saveToSharedTables($entity, $this->revisionDataTable);
...
$this->invokeFieldMethod($is_new ? 'insert' : 'update', $entity);
....
$this->saveToDedicatedTables($entity, !$is_new);
not sure what's the use case for keeping update() and insert() if they are "post" ops,

As proven above, they're already close to be "post" ops. I have a use-case where I need such "post" ops. This code is already working with current 8.0.x. But as I had a look at the implementation I was convinced that it really needs some cleanup:

  • The documentation is wrong. 'update' and 'insert' are definitely not called before save.
  • They're called within the SQL storage, which is wrong according to #2427001: field item presave is only called for SQL.
  • All the small differences between the callbacks and the hooks are confusing for developers.
  • The API is incomplete regarding pre and post ops. Again a bad DX for contrib developers.

the code that needs to differenciate between an insert and an update needs to tell the difference themselves in the unified preSave()

Again, that's already the case.

the discussion in #2023473: Merge FieldItem::insert() and FieldItem::update() on fields into preSave() seemed to imply that using isNew() is problematic at that point?

In fact, it isn't. I already implemented stuff like this. One example is the ChangedItem in #2428795: Translatable entity 'changed' timestamps are not working at all.

alexpott’s picture

re #25 the isNew() thing is now solved - this will be correct until the entity save is complete. This was fixed by #2297817: Do not attempt field storage write when field content did not change. In fact looking at the code it didn't go far enough. Filed #2481371: Move enforcing new and new revision out of storage specific code to address that. I think that this change makes sense because then the entity insert and update hooks are being fired at the same time as the entity's field hooks.

yched’s picture

Priority: Normal » Critical

the isNew() was fixed by #2297817: Do not attempt field storage write when field content did not change

Oh, nice - but gee, I didn't even notice that over there, which, after the hair pulling in #2405165: Entity::setOriginalId() does enforceIsNew(FALSE), that is wrong for ConfigEntities shows that this area is a bit messy :-/

Well then, I'm fine with using preSave() for "adjust field values before they are written".

$this->saveToSharedTables($entity);
...
$this->invokeFieldMethod($is_new ? 'insert' : 'update', $entity);
....
$this->saveToDedicatedTables($entity, !$is_new);

Aw. That's... pretty wrong indeed :-). Item::insert()/update() being called either after or before writing values depending on whether you're in a shared or dedicated table is badly broken. This probably got lost at some point during the many D8 refactors in there, because those were definitely called before originally.

Bumping to critical for that :-/

About update() / insert() being post-write, or simply removed :

- I'm curious to know what's your use case for acting on post-write rather than on pre-write ? If you can't do your thing in pre, that means you modify the field values and don't want those changes to be stored, but then those values get modified for the rest of the $entity life in the request, and will get written if it gets saved again ?

- Either option constitutes an API change, and a rather big one, since the fact that they are "sometimes pre, sometimes post" in HEAD is a bug (and possibly a relatively recent one, I didn't dig in the git history), the intent advertised in the phpdoc and the existing change notice about "D7 hook_field_(update|insert)() -> D8 FieldItemInterface::(update|insert)()", is that they are pre.
Also, the current "brokenness" only affects base fields, and most contrib field types that implement update() / insert() at the moment are probably very rarely used for base fields, so the bug hasn't likely affected them. Moving them to post *will* affect them though.

yched’s picture

Also :

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -753,10 +753,10 @@ public function deleteRevision($revision_id) {
+      $this->invokeHook('revision_predelete', $revision);
       $this->database->delete($this->revisionTable)
         ->condition($this->revisionKey, $revision->getRevisionId())
         ->execute();
-      $this->invokeFieldMethod('deleteRevision', $revision);
       $this->deleteRevisionFromDedicatedTables($revision);
       $this->invokeHook('revision_delete', $revision);

That still leaves two invokeHook() called only by SqlStorage, isn't that what we want to fix here as well ?

All the other calls to invokeHook() are now in one of the generic base classes (EntityStorageBase / ContentEntityStorageBase / ConfigStorage), which is awesome. So looks like we should move that to ContentEntityStorageBase, with a doDeleteRevision() submethod ?

mkalkbrenner’s picture

I'm curious to know what's your use case for acting on post-write rather than on pre-write ? If you can't do your thing in pre, that means you modify the field values and don't want those changes to be stored, but then those values get modified for the rest of the $entity life in the request, and will get written if it gets saved again?

it's obvious if you think about drupal being part of a bigger software product and not just stand-alone. To have a custom field type that triggers various actions *after* it has been saved successfully is one example. That's not about changing it's own value. For example the new field value needs to be logged or replicated or indexed or ... for or into a different system.
Another use-case is a kind of semaphore or lock. A value that could only be set once and that will be released after save.
I think it's good to offer such a complete API to developers of contrib and custom code.

Also, the current "brokenness" only affects base fields, and most contrib field types that implement update() / insert() at the moment are probably very rarely used for base fields, so the bug hasn't likely affected them. Moving them to post *will* affect them though.

That's not true! if they are effected, they are broken right *now* (or since beta whatever version), because 'insert' and 'update' are definitively working as post at the moment.
I think the API should be fixed - or cleaned up - right now, before we have an RC: 'insert' and 'update' should act post save.

You're right about the delete callbacks. But I suggest that we add the task to move them from the SQL storage to an abstract base class to issue #2481371: Move enforcing new and new revision out of storage specific code and focus on the API here. What do you think?

yched’s picture

Issue tags: +Entity Field API

I think we're mixing several things in this issue.

1) update() / insert() working pre-write or post-write :

if they are effected, they are broken right *now* (or since beta whatever version), because 'insert' and 'update' are definitively working as post at the moment.

No, as you pointed yourself, in current HEAD, update() and insert() :
- are working pre-write, as they should and as they are advertised, for fields stored in dedicated tables (that is, configurable fields, and base fields with cardinality > 1)
- they are working post-write for fields stored in shared tables (that is, mono-valued base fields), and that is a bug, that doesn't make "post-write" the official or expected behavior for those methods. Those have always worked pre-write, like the sister preSave() / delete() / revisionDelete() methods, since CCK D5 (they were magic callbacks rather than methods back then).

Most if not all existing 3rd party field types out there that implement update() / insert() necessarily expect them to work pre-save, because that's what the doc says, and that's what happens for the large majority of the current uses of those field types (configurable fields), since contrib field types are much less likely to be used as base fields.

In short : the current behavior on base fields is a very unfortunate and critical bug, but it's a bug, not some indecision or vagueness from the API, which works as expected and documented (pre-write) in the vast majority of actual cases.
So changing update() / delete() to be post-save is an API change and will break existing code that currently works. That BC break is not required to fix the bug.

2) The fact that some of those calls are triggered only by SqlContentEntityStorage instead of the generic ContentEntityBase. Right, that's a bug, which should be fixed. D8 has been a bumpy road for entity storage code :-)

3) After that, it can be discussed whether :

- we need additional methods that let field types do stuff post-write,
(I can't say I'm convinced atm, that's what the existing hook_entity_*() are here for, the examples in #30 feel like semantics attached to a specific field in a specific entity type, not to a field type. At any rate that seems like edge cases that I'm not sure is worth adding more verbosity on FieldItemInterface and more runtime method calls ?)

- we break BC by repurposing the exisisting update() / insert() to be those post-write methods for "save" instead of the pre-write methods that they currently are used for,
(I don't see the consistency, next to the explicit "postXxx()" methods added by the current patch, that should rather be postSave() ?)

- we state (as #2023473: Merge FieldItem::insert() and FieldItem::update() on fields into preSave() set out to do 2 years ago but couldn't because of a bug with isNew() that was just recently fixed) that update() / insert() are redundant with preSave() and make the API leaner by just removing them
(that one has my preference :-))

But those look more like separate feature-request / API cleanup that bugs ?

chx’s picture

I find it somewhat odd that insert/update is/should be pre given how for example node presave/insert/update worked since, well, forever.

yched’s picture

I have tried to explain that above, but let me try again :

- insert() /update() are pre because their intent is to let the field type prepare the field values before they get written.They are here to act, not react - they are not hooks, and never were, even in D5. Along with preSave(), delete(), they are what allows the field type do its job of managing the content of the field values

- insert() / update() exist mostly for historical reasons, because preSave() didn't exist at some point. They could be removed now. That would leave us with preSave(), delete(), deleteRevision() - all pre, because that's what the field type needs to do its job.

- I don't really see a use case for adding methods for post counterparts, that is not the business of a field type IMO, that makes the interfaces more verbose, adds maintainance burden and runs more runtime code for what seems edge use cases for which the existing entity hooks are just fine. I can be proven wrong, but atm I'd rather not do it.

- *If* we add those post methods though, then IMO we should consistently name them "postDelete(), postSave()..." rather than "postDelete(), update(), insert()..."
For consistency, and because clearly removing moot methods and adding new ones with a different behavior is a cleaner BC break than reusing the same method name for a radically different thing (pre or post makes a radical difference for existing implementations)

chx’s picture

Thanks for this awesome writeup! I get it: the hooks and the methods have diverged very very long ago and reconciliation now is painful.

> pre or post makes a radical difference for existing implementations

Sure they do but are there any? It's D8 after all and these are rather obscure methods.

mkalkbrenner’s picture

@yched: Thanks for your detailed explanation. I see your concerns regarding BC if we don't just look at D8 but older contrib modules that might be ported soon.

Nevertheless I would like to see one post save callback on fields. You might consider that as an edge case but I think of Drupal as a framework having the use-cases in mind I described in #30. And implementing them using the the entity API or module hooks means that you have to implement them for every entity or module instead of a single field type. Additionally it's more complex for a site builder to add such a behavior to an existing entity if it's not a field (type).

What do you think about this approach:

webchick’s picture

Tentatively tagging.

xjm’s picture

alexpott’s picture

Discussed with @catch, @effulgentsia, @webchick and @xjm. We decided to defer triaging until the issue has had more discussion and consensus has been built. There is no doubt that what @yched outlined in #28 is critical. The only question is do we spin off a separate issue for that? Ideally we'd like to minimise the number of API changes so we're giving this issue time to work out how to make Entity and Field API consistent.

yched’s picture

And implementing them using the the entity API or module hooks means that you have to implement them for every entity or module instead of a single field type. Additionally it's more complex for a site builder to add such a behavior to an existing entity if it's not a field (type).

I still don't really get this.
"You have to implement them for every entity type" - sure but hook_entity_*() catches all entity types, so one or several entity types makes no difference ?
"You have to implement them for every module" - I don't get what that means :-)
"Additionally it's more complex for a site builder to add such a behavior to an existing entity if it's not a field (type)" - that's what hook_entity_*() is there for ?

Sorry for being a pain there, but I do feel that post methods are not just a minor addition. Having those in the interfaces is a philosophical shift from "fields as semantically agnostic data modelling units", with semantics/behavior being assigned from the outside, to officially shaping an API for "fields as standalone behavior units" - just like CreatedItem / UpdatedItem currently are "timestamp fields with some hardcoded behavior", which I still feel is a bad idea.
This multiplies the number of separate field types, meaning formatters and widgets need to account for them (while they only care about the field data sctructure). This means the list of field types in Field UI's "Add field" mixes pure-data field types and data+behavior field types with no explanation and no telling them apart, so it's not clear whether adding a field will just add passive data or trigger some behavior.

I'm not vetoing this (I don't have that power anyway), but that really is a feature request IMO, and I'd really prefer if that was discussed in a dedicated issue rather than bundled in a critical bugfix that is ultimately unrelated ?

Can we just commit a fix that :
- removes the update() and insert() methods, since they are redundant with preSave(), and happen to be currently broken for base fields,
- move the trigger points from SqlContentEntityStorage to ContentEntityStorageBase
and open a separate issue for postSave() / postDelete() / "fields as behavior units" ?

alexpott’s picture

+1 for #39.

Can we just commit a fix that :
- removes the update() and insert() methods, since they are redundant with preSave(), and happen to be currently broken for base fields,
- move the trigger points from SqlContentEntityStorage to ContentEntityStorageBase
and open a separate issue for postSave() / postDelete() / "fields as behavior units" ?

That makes a great deal of sense to me.

jibran’s picture

Issue tags: +Needs change record

We need a change notice for new hooks.

dawehner’s picture

Can we just commit a fix that :
- removes the update() and insert() methods, since they are redundant with preSave(), and happen to be currently broken for base fields,
- move the trigger points from SqlContentEntityStorage to ContentEntityStorageBase
and open a separate issue for postSave() / postDelete() / "fields as behavior units" ?

Especially if this solves the criticality of it, +2

Berdir’s picture

- removes the update() and insert() methods, since they are redundant with preSave(), and happen to be currently broken for base fields,

Actually, #2023473: Merge FieldItem::insert() and FieldItem::update() on fields into preSave() showed that it's not so redundant. some insert/update methods need the entity ID, and we also got lost in naming discussions there.

mkalkbrenner’s picture

Can we just commit a fix that :
- removes the update() and insert() methods, since they are redundant with preSave(), and happen to be currently broken for base fields,
- move the trigger points from SqlContentEntityStorage to ContentEntityStorageBase
and open a separate issue for postSave() / postDelete() / "fields as behavior units" ?

This is not the way to just fix the critical!
Fixing just the critical just means to move the call of update() and insert() to the right position!

Moving them out of the SQL storage, removing these callbacks or replace them by postSave() are "just major" bug fixes.

It's ok to fix the critical ASAP but please do not remove update() or delete() until the discussion about them or their successor led to a decision.

Personally I need a callback like update or insert that is different from preSave() to solve #2453153: Node revisions cannot be reverted per translation. And in core there's also an implementation that distinguish between the callbacks to achieve a functionality that I described in my comments above (and which were considered as edge cases here): Drupal\path\Plugin\Field\FieldType\PathItem

class PathItem extends FieldItemBase {
  ...
  public function preSave() {
    $this->alias = trim($this->alias);
  }

  public function insert() {
    if ($this->alias) {
      $entity = $this->getEntity();

      if ($path = \Drupal::service('path.alias_storage')->save($entity->urlInfo()->getInternalPath(), $this->alias, $this->getLangcode())) {
        $this->pid = $path['pid'];
      }
    }
  }

  public function update() {
    // Delete old alias if user erased it.
    if ($this->pid && !$this->alias) {
      \Drupal::service('path.alias_storage')->delete(array('pid' => $this->pid));
    }
    // Only save a non-empty alias.
    elseif ($this->alias) {
      $entity = $this->getEntity();
      \Drupal::service('path.alias_storage')->save($entity->urlInfo()->getInternalPath(), $this->alias, $this->getLangcode(), $this->pid);
    }
  }
  ...
}
jibran’s picture

Title: field item callbacks are only called for SQL storage and field API is inconsistent with hooks » FieldItem callbacks are only called for SQL storage and field API is inconsistent with hooks
plach’s picture

Assigned: mkalkbrenner » plach

FYI I just submitted a D8 Accelerate Application to bring the critical side of this forward.

After reading the whole discussion and #2023473: Merge FieldItem::insert() and FieldItem::update() on fields into preSave() , my personal feeling is that we should handle the various outlined aspects as a single issue. In fact, even though there are quite distinct concerns being raised here, IMO they are deeply related and it seems many of them would end up affecting DX and BC in a significant way. I'm not sure incremental steps in this case would be beneficial as, depending on the outcome of the various sub-discussions, we may end-up changing or even partially reverting previous solutions.

That said, this is my proposed way forward (inspired to @yched's one from #39):

  • Remove the ::update() and ::insert() methods, since they happen to be currently broken for base fields. I agree with @Berdir that they are not redundant currently, because it seems they are needed by code requiring the entity id, which for the SQL storage is available only after saving the shared table records. However, as pointed out by @yched, this needs to be fixed somehow, and the only way forward I see is finding a way to move the related logic to ::preSave() so base fields are covered too. Additionally we should try not to keep around a differentiation between ::preSave() and ::insert()/::update() that makes sense only for a specific storage backend.
    Btw, I suspect the critical bug was not introduced during conversions but has always been there, simply there were no fields in shared tables prior to D8 :)
  • Move the trigger points from SqlContentEntityStorage to ContentEntityStorageBase.
  • Keep discussing ::postSave() / ::postDelete() / fields as behavior units™ in parallel to see whether we can come up with legitimate use cases for them: I share @yched's perplexities around this area. PathItem is not a good example because, even if it could not work on ::preSave(), as the entity URL could not be determined without its ID, that's a legacy piece of code that should actually be implemented as a PathAlias entity reference and thus leverage the regular field save workflow. It would be good to have some clarifications around the #2453153: Node revisions cannot be reverted per translation use case.
plach’s picture

Issue tags: +D8 Accelerate

The grant was confirmed. I started experimenting with this in #2496337: [plach] Testing issue, after discussing it with @Berdir.

plach’s picture

Issue summary: View changes

I updated the IS with the solution discussed with @Berdir (see here for details). I outlined it also in a hangout we had this morning about critical issues and @alexpott, @catch and @fago agreed it makes sense as a way forward. We still need @yched's sign-off but now I'm confident enough to start coding it.

I left out the solution for this part:

FieldItemInterface::delete() is called before deletion, while the corresponding hook is invoked after deletion. Additionally, there is a pre-delete hook which is invoked before deletion, but there is no corresponding field method. A similar problem exists for FieldItemInterface::deleteRevision().

The reason is that we did not discuss it with Berdir, it's not part of the critical issue, and there was no consensus on it AFAICT. However, I think we should keep discussing it here and open a separate issue only if we identify a solution that does not imply significant BC breaks, otherwise better doing a single big change.

plach’s picture

Title: FieldItem callbacks are only called for SQL storage and field API is inconsistent with hooks » FieldItemInterface methods are only invoked for SQL storage and are inconsistent with hooks
Issue summary: View changes
plach’s picture

Issue summary: View changes

Small IS improvements

plach’s picture

Issue tags: -needs technical direction +Needs tests, +Needs subsystem maintainer review
FileSize
23.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,293 pass(es), 0 fail(s), and 2 exception(s). View

This implements the proposed solution, additional test coverage still missing. I'd like to get some feedback on the current approach before spending more time on this. I will merge the previous patches as soon as we figure out how to proceed with deletion / revision deletion.

@yched's feedback would be especially welcome, since he's the only stakeholder I couldn't get in touch with.

plach’s picture

FileSize
1.3 KB
23.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,274 pass(es), 0 fail(s), and 2 exception(s). View
+++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
@@ -201,12 +201,9 @@ public function preSave() { }
+    return TRUE;

This was meant to be FALSE, otherwise all field values will be always resaved. OTOH a green patch in #51 will show this is working despite the lack of explicit test coverage.

The last submitted patch, 51: field-item_callbacks-2478459-51.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 52: field-item_callbacks-2478459-52.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
1.49 KB
24.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,274 pass(es). View
24.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,286 pass(es). View

These should be green for reals.

The fix is just setting $node->setNewRevision(FALSE); before saving the entity in the test hook implementations. Saving the entity again in the post-save phase is not something we should encourage but there are use cases for that, albeit not so common. I think it's fair to have to manually set the new revision flag to FALSE (that normally happens after post-save hooks have run), because it may be useful to know in the post save phase whether the current revision has just been created.

dawehner’s picture

I can't judge parts of the actual code, my knowledge is really limited in that area, so here are just some general comments
...

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -158,6 +159,23 @@ public function purgeFieldData(FieldDefinitionInterface $field_definition, $batc
    +
    +    if ($this->entityType->isRevisionable()) {
    +      $entity->setNewRevision(FALSE);
    +    }
    

    We should document why this is needed

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -183,30 +201,86 @@ protected function invokeTranslationHooks(ContentEntityInterface $entity) {
    +    $call = is_callable($method);
    

    What about $method_is_callable as variable name?

  3. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
    @@ -444,6 +432,32 @@ public function save(EntityInterface $entity) {
    +    $this->invokeHook(!$update ? 'insert' : 'update', $entity);
    

    Is it just me that $update ? 'update' : 'insert' would be a bit easier to read?

  4. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -1301,8 +1354,11 @@ protected function loadFromDedicatedTables(array &$values, $load_from_revision)
    +   * @param string[]|NULL $names
    +   *   (optional) The names of the fields to be stored. Defaults to all the
    +   *   available fields.
    ...
    +  protected function saveToDedicatedTables(ContentEntityInterface $entity, $update = TRUE, $names = NULL) {
    
    @@ -1317,7 +1373,13 @@ protected function saveToDedicatedTables(ContentEntityInterface $entity, $update
    +    if ($names) {
    +      $definitions = array_intersect_key($definitions, array_flip($names));
    +    }
    

    Any reason to not use an empty array by default? The empty array condition below triggers the same

plach’s picture

FileSize
3.55 KB
25.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,510 pass(es). View

Fixed, thanks.

Any reason to not use an empty array by default? The empty array condition below triggers the same

I guess it was to avoid instantiating an array unnecessarily :)

dawehner’s picture

It would be great to get feedback from @mkalkbrenner I guess?

plach’s picture

Yep, definitely

mkalkbrenner’s picture

I started reviewing the patch. It looks promising but I'll need some more time to think about it.

+++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
@@ -189,20 +189,19 @@ public function view($display_options = array());
+   * @return bool
+   *   Whether field items should be rewritten to the storage as a consequence
+   *   of the logic implemented by the custom behavior.
...
+  public function postSave($update);

I don't know if you had a closer look at #2453153: Node revisions cannot be reverted per translation. But this boolean return value here will simplify that task a lot :-)
Using this feature I can remove some ugly code that I didn't upload as a patch yet because it looked too much like a hack.

I'll suggest that I do a quick port of my (local) solution for #2453153: Node revisions cannot be reverted per translation based on plach's patch.
Afterwards I'll come back and finish the review here based on the experience with my use-case in #2453153: Node revisions cannot be reverted per translation.

plach’s picture

Sounds good, hopefully @yched won't bash it meanwhile ;)

mkalkbrenner’s picture

For me the patch looks good and I already leveraged it at https://www.drupal.org/node/2453153#comment-9988055

As already mentioned the patch requires test. Additionally I'm not sure if we should already include a solution for delete and deleteRevision in the patch or if that should be handled by a follow-up.

yched’s picture

Ouch. Indeed, $entity->id is the crux of the issue here. In D7 we always had an entity ID by the time field values were saved in separate tables, so insert() / update() worked as "let the field type adjust its values *before* they are written, and it can use the entity ID if it needs".

Now that the entity ID is only known after writing (some of) the (base) fields, that are written in an atomic bulk since they are in the same table, that's indeed kind of messy :-/

In other words, a file field used as a base field would probably be pretty broken atm ?
And right, not sure how path.module / PathItem even works currently ?

What bugs me is that the proposed FieldItemInterface::postSave() workaround:
- is fairly convoluted :-)
- does temporarily write non-final, potentially invalid (incomplete / unmassaged) values to the db, which makes me a little nervous...
- the logic and DX is IMO off with regard to the actual needs of the field type :

The use case that FileItem / PathItem / [any other contrib or custom field type that will get bitten by this] have is not "I need to fire after my own values were written a first time, and I might then want them to be written once again", the actual need is "I need to tell you what to save for my values, but I'll need the $entity to be a real $entity with an ID and revision ID at this point".

The current solution of "we give you a chance to re-save after you were saved" doesn't really hint at why I, as a field type implementor, would need to resort to it. My worry is not "before or after *I* am saved", it's "before I am saved, but after some other (very important) fields are saved (because that's how they happen to get a value I can work with)"

It does seem like having a separate primary step where the entity gets its basic incremental IDs (id and revision_id), and then the rest of the fields get saved, would be precious. Can we elaborate on the reasons why that's not doable ?

mkalkbrenner’s picture

@yched:

... it's "before I am saved, but after some other (very important) fields are saved (because that's how they happen to get a value I can work with)"

It does seem like having a separate primary step where the entity gets its basic incremental IDs (id and revision_id), and then the rest of the fields get saved, would be precious.

Yes, that's exactly my use-case in #2453153: Node revisions cannot be reverted per translation. I need the new revision_id *before* my field is saved.

It does seem like having a separate primary step where the entity gets its basic incremental IDs (id and revision_id), and then the rest of the fields get saved, would be precious. Can we elaborate on the reasons why that's not doable ?

To implement it that way for the sql storage means to call preSave() after the data is written to the base table and the revision table where the auto-increments for id and revision_id happen. I'm not 100% sure if it's impossible to store fields in the base table. If I assume that this is impossible we have to split doSave() in two separate functions to get this workflow:

  1. doSaveBase()
  2. fire preSave() callbacks
  3. doSaveData()

The split is required because one motivation of this issue is to move the callback executions out of the concrete storage implementations.
This workflow might be OK for fields only. But what is about the preSave() callback on entities? Does it need to be fired before doSaveBase()?

mkalkbrenner’s picture

Yes, that's exactly my use-case in #2453153: Node revisions cannot be reverted per translation. I need the new revision_id *before* my field is saved.

I need to revert my previous statement. I need the new revision_id and new changed timestamp. The new changed timestamp itself is not one of the "basic incremental IDs" as yched called them, but "calculated" within the preSave() callback on fields. Therefor the workflow I described above will not work but plach's patch is a solution to my problem.

@yched

does temporarily write non-final, potentially invalid (incomplete / unmassaged) values to the db, which makes me a little nervous...

That's mitigated by the usage of transactions.

plach’s picture

First of all I must admit I'm not a fond of the proposed solution too, however I think it's the lesser evil. Here's why:

It does seem like having a separate primary step where the entity gets its basic incremental IDs (id and revision_id), and then the rest of the fields get saved, would be precious. Can we elaborate on the reasons why that's not doable ?

I initially played with this idea too, i. e. introducing an ::allocateEntityId() (and revision ID) method, however this has the disadvantage of always requiring two writes, while the proposed solution allows to avoid the double write whenever no field requires to be re-saved, and even if we have one, it will be actually written only if its value has changed (at least for the SQL storage).

Additionally this solution allows us to natively support use cases that otherwise would require to implement a hook and then re-save the whole entity, which is way less performant and clean, since in that case we initiate a nested save process without having completed the parent one.

The use case that FileItem / PathItem / [any other contrib or custom field type that will get bitten by this] [...]

Actually files and paths are not rewritten: they just act post save, because that way they have access to the entity ID. In fact it's not so easy to come up with a use case were rewriting would be needed: probably only when the stored value actually contains the entity ID. If path items were stored in the regular storage their source property would require that (only on insert), but in that case I guess they would no longer need a source property at all, and only alias would be stored.

I see this as an edge case trick for which it's better to have native support in the storage layer, rather than using hooks and (nested) full entity save.

yched’s picture

Actually files and paths are not rewritten: they just act post save, because that way they have access to the entity ID

I might very well be missing something, but looking at the code, looks like there are cases where PathItem needs the entity to have an ID so that we can determine its path so that we can save an alias and write the pid back as part of the field value ?

+        if ($path = \Drupal::service('path.alias_storage')->save($entity->urlInfo()->getInternalPath(), $this->alias, $this->getLangcode())) {
+          $this->pid = $path['pid'];
plach’s picture

Mmh, you are totally right. That probably means we are missing test coverage, because the value is not being rewritten atm. I will investigate it tomorrow. Circular references are the typical example rewriting is meant to address, silly of me not thinking about that.

Anyway, in this case we could also create the path entry at ::preSave() with a dummy internal value and then update it on ::postSave() with the actual entity internal path. No rewriting but still two writes, I think that's unavoidable.

yched’s picture

- adding a allocateEntityId() step : agreed that two writes on each entity / revision creation is not great. Thus, I'm slowly coming over to postSave() :-)
We should IMO try to make the phpdocs of preSave() and postSave() clearer as to what's the difference and when postSave() should be used (id & revision_id not necessarily available in the former)

- about allowing postSave() to trigger a field rewrite (which does add weirdness to the API, and complexity in the storage code) :
If we did rewrite PathItem as per #68, that would get rid of the need for a field rewrite there.
@mkalkbrenner, does your use case in #65 also require a field rewrite ?

(FWIW, PathItem is IMO a good example of the antipattern I was talking about in #39 : a field *type* with embedded behavior (here, save an alias to the alias_storage service) hardcoded to any field of this type. Thus, PathItem is not a harmless data-modelling unit, like 95% other field types, and weird things happen if you attach more than one to a given entity)

plach’s picture

- about allowing postSave() to trigger a field rewrite (which does add weirdness to the API, and complexity in the storage code) :
If we did rewrite PathItem as per #68, that would get rid of the need for a field rewrite there.
@mkalkbrenner, does your use case in #65 also require a field rewrite ?

@yched, so you would still be fine with nested entity saves? I'm asking because in D7 we did not have this problem since people could update DB tables directly via manual queries, although that was discouraged, but this is no longer allowed in D8.

yched’s picture

Not sure I see the connexion with nested entity saves (which, yeah, are bad :-/) ?

You might very well be right, and it's possble we do need to support rewrites in the end. Just asking because the code to support them really adds to the complexity of the storage code - not only core SQL storage, but any custom/contrib alternate storage - and it's already not one of the most trivial areas :-)

mkalkbrenner’s picture

(FWIW, PathItem is IMO a good example of the antipattern I was talking about in #39 : a field *type* with embedded behavior (here, save an alias to the alias_storage service) hardcoded to any field of this type. Thus, PathItem is not a harmless data-modelling unit, like 95% other field types, and weird things happen if you attach more than one to a given entity)

I totally agree. But to solve that you need to come up with fields that are just representing data types. Any logic needs to be moved to a different layer. If you move it to Entities you can't share this logic between different Entity Types without introducing different anti-patterns we know from other frameworks or languages (multi-inheritance implemented via traits, god classes and stuff like that).
My motivation in the current state of the game is to fix real world (multilingual) problems in core before 8.0.0 is released.
Currently there's some logic in some fields. And I think we can't avoid that in the current architecture if we don't want to have probably more complex and redundant code across entity implementations.

@mkalkbrenner, does your use case in #65 also require a field rewrite ?

I would say 98% yes ;-)
I tried different approaches which lead to a huge amount of code to deal with race conditions and stuff like that. Using the new postSave it's simple, small and readable.
I need to track which translations have been modified for real when a new revision is created. Therefor I store the revision_id along with the changed timestamp of a translation.

plach’s picture

Not sure I see the connexion with nested entity saves (which, yeah, are bad :-/) ?

Well, if a module needs to resave a value and ::postSave() does not support that, it needs to implement hook_entity_[insert|update], update the field item value and re-save the whole entity before the previous process has completed, i.e. while we are still in the scope of the parent transaction (which makes sense, I guess).

You might very well be right, and it's possble we do need to support rewrites in the end. Just asking because the code to support them really adds to the complexity of the storage code - not only core SQL storage, but any custom/contrib alternate storage - and it's already not one of the most trivial areas :-)

Not sure whether you are more concerned about the changes to support collecting results from the field items, or the SQL storage changes. I think the former are clean enough, and should be reusable, should we need to introduce new methods for which we need to collect results. The latter are a bit dirty, especially the shared table rewriting, but I think sooner or later we'll have to perform a final internal clean-up of the SQL storage to make sure all the code is reusable and reused. @Berdir did a great unification work so far, but I think there's still room for some improvements.

Ideally the abstract ::doSaveFieldItems() would be the method we'd always use to write field item values to the storage, for any storage backend. If we got there the overall complexity wouldn't be that bigger.

plach’s picture

Btw, PathItem does not need to be re-saved on insert, it only needs to update the entity data structure with the generated alias id. In fact everything is already stored in its custom storage, there is no data stored in the entity field storage to update.

yched’s picture

Not sure whether you are more concerned about the changes to support collecting results from the field items, or the SQL storage changes

A bit of both actually :-)

On the first part,

1. looking at an implementation of postSave(), it's not really intuitive to see "we are *after* save, yet this code still modifies field values - oh yes, that works because it also returns TRUE".
Silly idea, might not work : woudl it be clearer if we passed a $resave param by reference ? Seing "$resave = TRUE" is more telling than just "return TRUE" ?

2. I still don't get how FieldItem works without asking for resave, btw : how is the pid written in the field on entity creation ?

3. having to collect the return values for Item::PostSave() makes weird things with FieldItemList::delegateMethod() so that it can also act as either as "delegate the call to each child" or as a generic iterator for any callable. Feels like bulging two separate jobs, and the method name becomes misleading.
Can't we instead modify delegateMethod() so that it accepts an array of params to be passed to each $item->${method}() call ? Looks like it would remove the need for the closure in FieldItemList::postSave(), and delegateMethod() no longer needs to be turned into a "generic iterator" ?
[edit: that probably contradicts my suggestion about a by-ref $presave param in 1. above - well, I'd say DX of postSave() is more important, so if the by-ref idea is deemed worthwhile, we might as well leave delegateMethod() alone and inline the special logic directly in FieldItemList::postSave()]

4. having FieldItemList and PathItem postSave() return their parent::postSave() result, when we know their parent returns nothing, and they actually don't need to either, adds uneeded bloat (and uncertainty - "maybe they do ask for resave after all ?"). Can we leave them returning nothing ?

On the SQL storage part,

5. yeah, the current code where :
- doSave() (phpdoc : "Performs storage-specific saving of the entity") saves all fields a 1st time
- doSaveFieldItems() (phpdoc : "Writes entity field values to the storage") then saves some of them a second time,
seems very confusing :-)

Ideally the abstract ::doSaveFieldItems() would be the method we'd always use to write field item values to the storage, for any storage backend

YES, big +1 :-)
Plus, if the logic for "call doSaveFieldItems() on all fields, run postSave(), call doSaveFieldItems() again on the fields that asked for it" lives in ContentEntityStorageBase, we don't put the burden of understanding/implementing that weird save/resave dance on every alternate or overriden storage backend.

plach’s picture

1: I can try, not sure it will simplify the code that much though
2: The related base field definitions are marked as custom storage: there is no field data stored, at least for the two cases in core. This exemplifies very well why using field items to implement business logic is not a good idea. Hardcoded assumptions--
3: I hope I'll get this right ;)
4: We can :) The idea was setting an example so child classes wouldn't forget to consider/forward the parent value.

Plus, if the logic for "call doSaveFieldItems() on all fields, run postSave(), call doSaveFieldItems() again on the fields that asked for it" lives in ContentEntityStorageBase, we don't put the burden of understanding/implementing that weird save/resave dance on every alternate or overriden storage backend.

Yep, that's exactly what I meant, thanks for making it explicit :)
Not sure: are you suggesting to deal with that in a non-critical follow-up or to try and achieve that here?

yched’s picture

1. by-ref $resave param in postSave() : yeah I was more throwing that in the air for DX feedback, not sure if that's really a good idea.

2. PathItem : "The related base field definitions are marked as custom storage, there is no field data stored"
Oh HAH ! Indeed, and PathItem defines no fields schema anyway :-) I guess path only bothers providing a field / field type so that it can integrate into entity forms using a Widget. Then $entity->path only ever has values when processing entity form submit, because they are placed in submitted values by PathWidget. The field has no values in other, non-form entity manipulations.

That feels a bit fishy, IMO path would be better off doing its stuff whithout fields, but well, that's a separate issue.
The takeaway for this issue here is indeed : PathItem needs to run code after we have an entity ID, and it doesn't need to save anything, even less re-save :-)

re: storage flow / doSaveFieldItems() : Yeah, tough one. I'm tempted to say it would be best to get a clean flow first off ;-), at least without adding a doSaveFieldItems() that doesn't really do what its doc says. But if that means doing The Big Storage Cleanup (tm) as a pre-requisite, then of course that's not too cool.

Is there a way we can shape ContentEntityStorageBase's flow around doSaveFieldItems() here in a way that has a chance to be final / without further API breaks planned for later, and then leave the *Sql*ContentEntityStorage cleanup as an internal, non-API breaking followup task ?
This issue here introduces the notion of resaves and the need for storage backends to account for those. It seems part of the critical to try to get the corresponding API for storage backends right in the same patch, rather than breaking them once here with a "quick-n-dirty" version and again later for the "real clean" version ?

plach’s picture

FileSize
9.12 KB
26.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,330 pass(es). View

This should address #75.3 and #75.4. It also improves the related documentation, which should also help with #75.1.

I will now work on #77:

Is there a way we can shape ContentEntityStorageBase's flow around doSaveFieldItems() here in a way that has a chance to be final / without further API breaks planned for later, and then leave the *Sql*ContentEntityStorage cleanup as an internal, non-API breaking followup task ?
This issue here introduces the notion of resaves and the need for storage backends to account for those. It seems part of the critical to try to get the corresponding API for storage backends right in the same patch, rather than breaking them once here with a "quick-n-dirty" version and again later for the "real clean" version ?

plach’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -254,20 +258,12 @@ protected function invokeFieldMethod($method, ContentEntityInterface $entity) {
+      $resave = array_filter($results);

This was meant to use a +=. To be fixed in the next patch.

plach’s picture

This addresses #77.

@chx:

Do these changes make sense to you? Will they make the Mongo storage simpler/harder to implement?

@all:

If @yched, @chx and @Berdir are happy with this, hopefully we should be missing only additional test coverage...
(and fixing test failures :)

plach’s picture

FileSize
31.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,256 pass(es). View

And now even with patch!

yched’s picture

Thanks @plach !

I'm away from my coding environment for the next 3 weeks :-\, so reviewing #80 on my phone isn't too easy. Looks very nice, but would probably be best vetted by @Berdir anyway.

- Any way we can merge Sql's doRegularSave() and doResaveFieldItems() back into a single common implementation inside doSaveFieldItems() ? As is, the separation kind of defeats the "single unified doSaveFieldItems() entry point". No biggie, I guess this can happen in a non-API-breaking followup.

- nitpick: while it's there, though, "doRegularSave()" is not a great name for "a subpart of doSaveFieldItems()", feels more connected to doSave(), which is a higher step in the callstack.

Regarding #78 :

- I might be missing something, but the multilingual logic in CESB::invokeFieldPostSave() looks weird . If postSave() returns TRUE in one language we resave the whole entity, not just that language ? Meaning we might resave the whole entity several times ?

- Nitpick : does it work if FileItem/PathItem::postSave() go back to just returning nothing instead of an FALSE ?

chx’s picture

> Do these changes make sense to you? Will they make the Mongo storage simpler/harder to implement?

I do not see a problem on first read but this should not be a decision making factor now.

plach’s picture

Issue tags: -Needs tests, -Needs subsystem maintainer review
FileSize
10.71 KB
37.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,914 pass(es). View

This addresses #82 and adds test coverage.

Any way we can merge Sql's doRegularSave() and doResaveFieldItems() back into a single common implementation inside doSaveFieldItems() ? As is, the separation kind of defeats the "single unified doSaveFieldItems() entry point". No biggie, I guess this can happen in a non-API-breaking followup.

That's exactly the hard part :)
I will look into that to see whether I can come up with something better without needing to do a big clean-up.

I might be missing something, but the multilingual logic in CESB::invokeFieldPostSave() looks weird . If postSave() returns TRUE in one language we resave the whole entity, not just that language ? Meaning we might resave the whole entity several times ?

Actually it's just collecting results for the various translations but field values are written only once. We have no support for (re)saving a single language at the moment. I added a comment to clarify what's currently happening.

plach’s picture

FileSize
8.72 KB
38.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,069 pass(es). View
38.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,129 pass(es). View

This unifies shared table field save logic. Attached you can find also a patch always resaving all fields. Let's see how many things I've broken.

The last submitted patch, 85: field-item_callbacks-2478459-85.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 85: field-item_callbacks-2478459-85.always_resave.patch, failed testing.

Status: Needs work » Needs review
plach’s picture

FileSize
6.8 KB
41.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,069 pass(es). View

Ok, the two green patches above should mean that the SQL storage refactoring is pretty reliable: both regular save and regular save + resave all entity field values pass all tests.

The attached patch improves test coverage and should be ready to go IMO.

Working on a CR, reviews welcome now :)

plach’s picture

Created two draft change records:

https://www.drupal.org/node/2508662
https://www.drupal.org/node/2508673

However I realized this is not ready yet, we still need to agree on a solution for:

FieldItemInterface::delete() is called before deletion, while the corresponding hook is invoked after deletion. Additionally, there is a pre-delete hook which is invoked before deletion, but there is no corresponding field method. A similar problem exists for FieldItemInterface::deleteRevision().

First of all: is this an actual problem? And if so, would it make sense to fix it as we did for the save workflow? That is:

  • rename FieldItemInterface::delete() to FieldItemInterface::preDelete()
  • add a FieldItemInterface::postDelete() method
  • rename FieldItemInterface::deleteRevision() to FieldItemInterface::preDeleteRevision()
  • add a FieldItemInterface::postDeleteRevision() method

Thoughts?

plach’s picture

Issue summary: View changes
Issue tags: -Needs change record +API change, +sprint
mkalkbrenner’s picture

Thoughts?

@chx an myself already agreed on "completing" the Field API in that way in earlier patches for this issue.
From my point of view we should still go for it :-)

plach’s picture

@mkalkbrenner:

@chx an myself already agreed on "completing" the Field API in that way in earlier patches for this issue.

Yep, I borrowed my proposed solution from the previous patches :)

My only doubt about that is whether we have actual use cases for ::postDelete(). If we do, I'd be fine with proceeding with that plan. Ideally we should have sign-off from @yched or a framework manager if @yched is not available.

yched’s picture

Still a bit hard for me to do actual code reviews atm, but the interdiff in #85 looks awseome :-) @plach++ !!
Ideally it would be nice to have @Berdir take a look at the storage changes, but if he's not available I think we can move ahead.

re: delete - I wish we didn't introduce two separate preDelete() / postDelete() to the interface without a notion of a use case.
Also, does it really belong to the same issue that fixes update() / insert() ? Albeit related, the changes in the current patch look standalone to me.

plach’s picture

re: delete - I wish we didn't introduce two separate preDelete() / postDelete() to the interface without a notion of a use case.
Also, does it really belong to the same issue that fixes update() / insert() ? Albeit related, the changes in the current patch look standalone to me.

Well, my rationale behind addressing everything here was to make all the BC breaking changes in a single issue, however we could implement the solution above in a BC way by deprecating ::delete() and make it invoke a new ::preDelete() method, so probably a separate issue would be fine.

benjy’s picture

Few small nitpicks I noticed while reading the code.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -158,6 +158,67 @@ public function purgeFieldData(FieldDefinitionInterface $field_definition, $batc
    +    if (!$is_new) {
    

    Why do we "not" the logic here, wouldn't be easier to read if we just switched the two code blocks?

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -158,6 +158,67 @@ public function purgeFieldData(FieldDefinitionInterface $field_definition, $batc
    +      // @todo, should a different value be returned when saving an entity with
    +      //   $isDefaultRevision = FALSE?
    

    Should this @todo have an issue number next to it? Or maybe be removed.

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -158,6 +158,67 @@ public function purgeFieldData(FieldDefinitionInterface $field_definition, $batc
    +   * and update the entity object with their values.
    

    s/update/updating

  4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -183,27 +244,71 @@ protected function invokeTranslationHooks(ContentEntityInterface $entity) {
    +        // call_user_func_array is way slower than a direct call so we avoid
    

    call_user_func_array missing brackets, noticed this in one other place as well.

yched’s picture

Re: delete()

Yeah, grouping BC breaks would be better. My reluctancy is that we eventually figured the only reason to split preSave() and postSave(), and that's because IDs are not available during the former. Thus, we have been able to explain that in the Interface (why there are two methods, and why you would want to use one or the other). I can't think of a similar explanation for a pre/post split for delete(), so I feel we might be just adding unneeded cruft and mental overhead with two methods when it actually makes no difference.

Also, note that delete() is different since, because of field purge, it can get invoked in batch after (read : long after) the entity record itself has been deleted.

Overall, I have no major objections to the plan in #91 (adding hooking points with no well-identified use cases, "just in case someone finds one at some point" is sort of Drupal's ADN). But as you know ;-), the delete/purge topic is a nasty area, for which base/configurable fields still behave differently. This might or might not be an argument for "separate issue" ?

plach’s picture

FileSize
3.84 KB
41.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,071 pass(es). View

Thanks, addressed #97 plus a small clean-up.

call_user_func_array missing brackets, noticed this in one other place as well.

From a theoretical POV the name without brackets is the right way to refer to a function, because brackets imply an application of the function, however in practice this is more readable and we do that all the time, so +1 :)

plach’s picture

FileSize
45.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,072 pass(es). View
4.92 KB

@yched:

Overall, I have no major objections to the plan in #91 (adding hooking points with no well-identified use cases, "just in case someone finds one at some point" is sort of Drupal's ADN). But as you know ;-), the delete/purge topic is a nasty area, for which base/configurable fields still behave differently. This might or might not be an argument for "separate issue" ?

Good point. I think we should split the deletion/revision deletion fixes into two areas:

  • Bringing them in line with the save workflow, that is implementing a consistent behavior between base and bundle fields and moving hook triggering into ContentEntityStorageBase.
  • Deciding whether #91 is actually needed.

The attached patch implements the former. We should definitely take purging logic into account when addressing the latter, so I'm now +1 on a separate issue implementing a BC solution, if needed.

mkalkbrenner’s picture

Just want to mention that hook_entity_storage_load and hook_ENTITY_TYPE_storage_load are only fired within SqlContentEntityStorage::getFromStorage(). That's the same kind of issue.

And the persistent entity cache is directly populated in SqlContentEntityStorage::doLoadMultiple(). SqlContentEntityStorage::setPersistentCache() and SqlContentEntityStorage::getFromPersistentCache() aren't SQL specific at all.

mkalkbrenner’s picture

FileSize
15.98 KB
61.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,306 pass(es). View

I moved the non SQL specific generic functions from SqlContentEntityStorage to ContentEntityStorageBase.
Even if we discuss to split this issue here into several parts I want to upload this patch based on #100 because it's impossible to run the complete test suite here.

mkalkbrenner’s picture

@catch asked me on IRC to mark #2474075: Fix Node::preSave() and document that preSave() and postSave() are not working with ContentEntity translations as related issue:

well one is only getting invoked for SQL storage, the other is only invoked for current translation. in both cases the hook firing is not happening when expected.

plach’s picture

The interdiff makes sense to me. There are certainly cases where entity caching is not very useful, NoSQL storage is probably one of them, but opting out is very easy, so I guess this should not be a concern.

plach’s picture

Assigned: plach » Unassigned

This needs review from @yched or @berdir now.

plach’s picture

There are certainly cases where entity caching is not very useful, NoSQL storage is probably one of them,

Well, actually being able to leverage memcache would be useful also for those.

catch’s picture

#1596472: Replace hard coded static cache of entities with cache backends deals with this for the static caching, but not the entity caching itself.

plach’s picture

FileSize
45.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,907 pass(es), 2,564 fail(s), and 1,091 exception(s). View

Rerolled after #2453153: Node revisions cannot be reverted per translation: revision translation flags are now populated in ContentEntityStorageBase, which is a Good Thing™.

plach’s picture

FileSize
61.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,441 pass(es). View

Oops, my local branch was missing #102, please ignore #108 (interdiff is #102 :).

The last submitted patch, 108: field-item_callbacks-2478459-108.patch, failed testing.

plach’s picture

Issue tags: +D8 Accelerate London
Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -158,6 +181,114 @@ public function purgeFieldData(FieldDefinitionInterface $field_definition, $batc
    +   * This method is responsible for allocating entity and revision identifiers
    +   * and updating the entity object with their values.
    

    Should/Can we update user storage according to this, which currently overrides the save() method?

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -158,6 +181,114 @@ public function purgeFieldData(FieldDefinitionInterface $field_definition, $batc
    +   *
    +   * @api
    +   */
    +  abstract protected function doDeleteFieldItems($entities);
    

    Is @api a thing now? not sure we came to a conclusion in that issue..

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -180,30 +311,116 @@ protected function invokeTranslationHooks(ContentEntityInterface $entity) {
    +  protected function invokeLoadUncachedHook(array &$entities) {
    

    This is bogus. It seems it was added as part of the entity cache patch but nothing uses it, this hook does not exist. Let's just drop it here.

  4. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
    @@ -444,6 +452,32 @@ public function save(EntityInterface $entity) {
    +   * @param $update
    +   *   Specifies whether the entity is being updated or created.
    

    nitpick: missing type.

  5. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -414,7 +396,10 @@ protected function doLoadMultiple(array $ids = NULL) {
    -    $entities_from_storage = $this->getFromStorage($ids);
    +    if ($entities_from_storage = $this->getFromStorage($ids)) {
    +      $this->invokeStorageLoadHook($entities_from_storage);
    +    }
    +
         $this->setPersistentCache($entities_from_storage);
    

    I was never a big fan of calling those methods even if we have no $ids/$entities_from_storage. Others like it instead of having many if conditions here, though.

    However, now that we have that anyway, can we move the setPersistentCache() into the if?

  6. +++ b/core/modules/block_content/tests/modules/block_content_test/block_content_test.module
    @@ -60,6 +60,7 @@ function block_content_test_block_content_insert(BlockContent $block_content) {
       if ($block_content->label() == 'new') {
         $block_content->setInfo('BlockContent ' . $block_content->id());
    +    $block_content->setNewRevision(FALSE);
         $block_content->save();
    

    why is this needed now?

Overall, this looks fine to me. Seems like we're putting a lot of internal refactoring into an issue that could also happen somewhere else and doesn't need to be critical, but it's also good to have that cleaned up.

Looking through the remaining code in SqlContentEntityStorage, I noticed that the only method (that I could find), that's still directly implemented is loadRevision(). it has very little generic code in it, apart from the postLoad(), but I am wondering if we should also introduce the do*() pattern for that. Possibly even with a default implementation that returns NULL, because 3 of 4 implementations do that now. Also interesting, I just noticed that we are likely forgetting to call the storage_load() hook on that at the moment.

Also, SqlContentEntityStorage::save() still calls $entity->updateOriginalValues(); That seems like something that should be generic too. not doing it might result in saving the wrong values or so?

saveRevision() is also interesting. That also contains some seemingly generic code (a method on $entity) but it's actually not generic at all. It passes $record, which is clearly an sql implementation detail :( But we needed something there to deal with revision things and node weirdness.

I'm also aware that those things contradict my earlier comment about putting a lot of refactoring into this. Totally open to do some things in a follow-up.

plach’s picture

FileSize
7.28 KB
63.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,280 pass(es), 722 fail(s), and 142 exception(s). View

This should address #112. Let's see whether I broke anything.

Status: Needs review » Needs work

The last submitted patch, 113: field-item_callbacks-2478459-113.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 113: field-item_callbacks-2478459-113.patch, failed testing.

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -183,6 +183,32 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
    +      $entities = [$revision];
    

    this might need to be keyed by the entity id.

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -183,6 +183,32 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
    +   * @param int $revision_id
    +   *   The revision identifier.
    

    probably consistent with the parent, but I think it's int|string now...

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -183,6 +183,32 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
    +   * @return \Drupal\Core\Entity\ContentEntityInterface|null
    +   *   The re
    

    Unfinished comment.

  4. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -575,12 +576,13 @@ public function loadRevision($revision_id) {
           if ($entity) {
    -        return $entity;
    +        $revision = $entity;
    

    Why is the $revision assignment needed?

plach’s picture

Status: Needs work » Needs review
FileSize
2.2 KB
64.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,453 pass(es). View

Addressed #117 and hopefully fixed failures.

Why is the $revision assignment needed?

Because otherwise FALSE may be returned instead of NULL.

plach’s picture

FileSize
64.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,242 pass(es). View
2.37 KB

While updating one of the CRs I noticed a naming inconsistency in the new methods. Renamed ::doDeleteFieldItemsRevision() to ::doDeleteRevisionFieldItems().

plach’s picture

FileSize
856 bytes
64.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,326 pass(es). View

Spoke with @Berdir, fixed also #117.4.

plach’s picture

@Berdir:

::saveRevision() is also interesting. That also contains some seemingly generic code (a method on $entity) but it's actually not generic at all. It passes $record, which is clearly an sql implementation detail :( But we needed something there to deal with revision things and node weirdness.

I just created #2524250: RevisionableInterface::preSaveRevision() is SQL-specific to address this.

plach’s picture

FileSize
2.3 KB
64.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,354 pass(es). View

@dawehner reviewed this live.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

So all my minor points are adressed. I really like EntityStorageBase::save() for example.

plach’s picture

Issue summary: View changes
Fabianx’s picture

RTBC + 1

jibran’s picture

WoW patch looks quiet good. CRs perfectly make sense. Nice work @plach. RTBC++.

@Berdir had already reviewed the patch and he is entity system maintainer so I think we can remove "Needs subsystem maintainer review" tag unless we are waiting for a French guy.

Fabianx’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This looks great it is really nice to nice non-SQL logic leaving the SQL storage class.

Committed 883c209 and pushed to 8.0.x. Thanks!

  • alexpott committed 883c209 on 8.0.x
    Issue #2478459 by plach, mkalkbrenner, chx, yched, Berdir, dawehner,...
Fabianx’s picture

Published the change records

yched’s picture

Slowly going through my pile of unread issues... Thanks a ton for driving this home, folks !

Status: Fixed » Closed (fixed)

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