Problem

When revisions are turned on for a node (type), new revisions are saved for each individual field edit. It "spams" the node revisions list heavily, therefore we need to figure out a way to queue up the changes and only save occasionally (either with user action or timed autosaves).

Proposed solution

Utilize tempstore to store a temporary version of the entity while it is being edited and do actual saves only occasionally.

CommentFileSizeAuthor
#114 edit-concurrency-fieldapi-114.patch6.01 KBGábor Hojtsy
#108 edit-concurrency-fieldapi-106.patch5.94 KBGábor Hojtsy
#106 interdiff.txt3.76 KBGábor Hojtsy
#106 edit-concurrency-fieldapi-106.txt5.94 KBGábor Hojtsy
#101 edit-concurrency-fieldapi-101.patch5.42 KBGábor Hojtsy
#99 edit-concurrency-fieldapi-99.patch6.14 KBGábor Hojtsy
#91 edit-concurrency-fieldapi-91.patch6.02 KBGábor Hojtsy
#89 edit-concurrency-fieldapi.patch5.35 KBGábor Hojtsy
#86 edit-tempstore-conflict-detect-86.patch7.3 KBGábor Hojtsy
#82 edit-conflict-error.jpg183.87 KBGábor Hojtsy
#82 edit-tempstore-conflict-detect.patch7.31 KBGábor Hojtsy
#48 edit-tempstore-1901100-48.patch19.87 KBjessebeach
#47 edit-tempstore-1901100-47.patch19.2 KBWim Leers
#45 interdiff.txt2.74 KBGábor Hojtsy
#45 edit-tempstore-1901100-45.patch18.42 KBGábor Hojtsy
#43 interdiff.txt5.43 KBGábor Hojtsy
#43 edit-tempstore-1901100-43.patch18.24 KBGábor Hojtsy
#42 edit-tempstore-1901100-42.patch12.81 KBGábor Hojtsy
#38 edit-tempstore-1901100-37.patch14.14 KBjessebeach
#38 interdiff.txt1.24 KBjessebeach
#36 edit-tempstore-1901100-36.patch13.94 KBjessebeach
#34 interdiff.txt5.95 KBGábor Hojtsy
#34 edit-tempstore-1901100-34.patch12.59 KBGábor Hojtsy
#32 edit-tempstore-1901100-32.patch10.02 KBGábor Hojtsy
#29 edit-tempstore-1901100-29.patch17.48 KBjessebeach
#27 edit-tempstore-1901100-27.patch17.33 KBjessebeach
#27 interdiff_25-to-27.txt4.1 KBjessebeach
#25 edit-tempstore-1901100-25-do-not-test.patch16.85 KBjessebeach
#24 edit-tempstore-1901100-24.patch16.88 KBjessebeach
#23 edit-tempstore-1901100-23.patch16.85 KBjessebeach
#19 edit-tempstore-19.patch15.54 KBGábor Hojtsy
#19 interdiff.txt3.29 KBGábor Hojtsy
#18 interdiff.txt720 bytesGábor Hojtsy
#18 edit-tempstore-18.patch14.56 KBGábor Hojtsy
#15 interdiff.txt864 bytesGábor Hojtsy
#15 edit-tempstore-15.patch14.56 KBGábor Hojtsy
#12 edit-tempstore-12.patch14.49 KBGábor Hojtsy
#11 edit-tempstore-11.patch13.12 KBGábor Hojtsy
#11 interdiff.txt10.26 KBGábor Hojtsy
#8 scaffolding_for-1901100-8-do-not-test.patch8.68 KBWim Leers
edit-tempstore.patch1.51 KBGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Nice! :)

Wim Leers’s picture

To get this up & running (i.e. actually usable by an end user), I think we need to:

  • add a entitySaveURL to edit_library_info(), which is called when the entity-level save button is pressed (and which implements a scoped AJAX command callback — see e.g. Drupal.edit.util.form.load() for an example
  • update Edit's routing to route that URL to a new EditController::entitySave() method
  • add a EntitySavedCommand.php AJAX command, which is called by EditController::entitySave()
webchick’s picture

Status: Active » Needs review

Marking needs review, since there's a patch. Though it might be a needs work based on #2. :)

larowlan’s picture

This is kind of similar problem space to what we're facing here: #1510544: Allow to preview content in an actual live environment
Note there sun is talking about different classes of revisions, some that are to be thrown away with garbage collection.
Just posting for cross-reference.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

@Wim: do we have a UI component to hinge the entity save on?

Wim Leers’s picture

#6: no; not yet. A quick temporary hack for a PoC would be to leverage Edit's hook_toolbar() and put a entity-level "Save" button in its tray (ironically we want through quite a bit of trouble to *not* have a tray).

Wim Leers’s picture

Title: Make edit module work with tempstore, so revisions are not saved on all atomic field edits » Make Edit module work with TempStore, so revisions are not saved on all atomic field edits
Assigned: Unassigned » Gábor Hojtsy
Status: Needs review » Active
Issue tags: +sprint
FileSize
8.68 KB

#4: Yes, comments #130, #140, #141 and #151 of #1510544: Allow to preview content in an actual live environment are very important and relevant for this issue.


Gábor is going to work on this soon, so assigning to him. Jesse is going work on #1678002: Edit should provide a usable entity-level toolbar for saving fields. This is the back-end part of the puzzle, the latter is the front-end part.

Attached is a patch that only does very superficial scaffolding, but it adds a few metric tonloads of comments with explanations of how it should/could work, tricky aspects to be overcome, and so on. It excludes the changes in the OP, it focuses on scaffolding. It does include the changes indicated to be necessary in #2.

Possibly most notably, the scaffolding that I added should allow us to not change a line of Edit's JavaScript — assuming I made no mistakes, which is a big assumption :)

Wim Leers’s picture

Wim Leers’s picture

I forgot: when I was looking into this issue, I had opened the following relevant files for investigating how to use TempStore: TempStore.php, views_ui.routing.yml, ViewEditFormController.php, BreakLockForm.php.

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
10.26 KB
13.12 KB

Here is an initial working version. The tempstore version of the entity is used for the form and the validation/submission in the form. This should not invoke entity hooks IMHO. Then when the dedicated edit path is invoked, it will save the entity for realz. It does not yet resolve session concerns. I also did not remove most of Wim's comments so we can still assess whether this actually does something like he envisioned :D

Interdiff against the the scaffolding patch.

Gábor Hojtsy’s picture

FileSize
14.49 KB

Missed two changes (in services.yml and EditFieldForm) from the patch which were in fact in the interdiff. Now putting those changes in as well.

Wim Leers’s picture

Status: Needs review » Needs work

This works amazingly well. Awesome job, Gábor :)

Bugs

  1. New Taxonomy Terms in "free tagging" Vocabularies: they're rendered correctly, but it is impossible to edit them. The more general problem is how to make entity reference fields (the Taxonomy Term field is now effectively an entity reference field) that allow you to create new entities work well. Because, alas, it is not safe to assume we can create the new (referenced) Entity already until we call EditController:entitySave().

Tricky edge cases confirmed to be working correctly

  1. Validation errors are displayed immediately (i.e. upon saving each individual Field into TempStore, and not upon saving the Entity from TempStore into the DB).
  2. I confirmed that the Field/Entity pre(save) hooks don't fire when saving individual field changes to the TempStore by adding the following code to edit.module and clearing caches:
    function edit_field_attach_presave(EntityInterface $entity) {
      watchdog('edit', 'hook_field_attach_presave fired!');
    }
    
    function edit_field_attach_update(EntityInterface $entity) {
      watchdog('edit', 'hook_field_attach_update fired!');
    }
    
    function edit_entity_presave(EntityInterface $entity) {
      watchdog('edit', 'hook_entity_presave fired!');
    }
    
    function edit_entity_update(EntityInterface $entity) {
      watchdog('edit', 'hook_entity_update fired!');
    }
    

    These are indeed only called when the Entity is saved from the TempStore into the DB.

  3. Analogously, this hook *does* fire for each modified Field that gets saved into TempStore, i.e. for the per-field re-rendering:
    function edit_field_attach_view_alter(&$output, $context) {
      watchdog('edit', 'hook_field_attach_view_alter fired!');
    }
    

The now proven fact that the above tricky edge cases are working correctly tells me that Edit module does not suffer from the architectural concerns that sun described at #1510544-131: Allow to preview content in an actual live environment. We don't run into the *truly* problematic cases of e.g. how to preview Node's "Provide a menu link" setting, because we *only* allow visible Fields to be edited. I.e. we don't allow metadata to be edited, only visible content. The one exception: the bug listed above, i.e. entity references to entities that are not yet created (and cannot be created yet).
Or am I missing something here?

Questions/remarks

  1. We should call call TempStore::delete($entity->uuid) in EditController::entitySave().
  2. A question in my scaffolding that hasn't been answered yet:
    So: how to determine when to *clear* the existing data in TempStore?

    The preceding remark only fixes the obvious case. For "obviously abandoned edits", I guess we should just leverage TempStore's expiration ability. The default is 1 week, which seems like *a lot*. However, what about editing a node today, being called away, and wanting to continue the next morning? And what if that same editor forgot about that and intends to start fresh? I think there's only one way to truly correctly handle this: the TempStore should be cleared whenever the user starts in-place editing a field on an entity that doesn't have *any* field already that is re-rendered from the TempStore. What this means is that if a user *starts* editing a node in-place, the TempStore entry is deleted, and when he *continues* editing a node in-place (i.e. some TempStore value is already visible to the end user), the TempStore entry is not deleted. If we did not clear the TempStore entry when a user *starts* editing a node in-place, then the presentation to the user (the entity as it lives in the DB, not the TempStore) would effectively be a lie and would lead to confusion.
    (Whew, that was very hard to explain, I hope that's kind of clear?)

Gábor Hojtsy’s picture

As for the bug, I think it is relevant for various field types. If you handle an image field that takes a file upload, that could have a very similar problem, it might not even re-render properly before you saved (I did not try yet). It is true that this affects fields where external data is involved, and the whole thing is not just stored integrated inside the entity. I'm not sure how to best solve this. The current entity system does not have a "please save this little piece of external data for me for now but don't save the whole field on the entity" and then "oh, by the way I want to revoke the thing that you just temporarily saved for me". Also, if that is about taxonomy terms (or even for images if you don't have deep backend permissions), you likely can only submit things that are instantly published. Terms don't have a status flag for example still. So if we are to create that term / save that image, it would show up in the site elsewhere in views, listings, in your most recent uploads, etc. even though it is just temporary for this edit. So depending on your needs and requirements, this can be a pretty sizable problem that has far-reaching consequences (eg. need to add unpublished terms that people can see who cannot see unpublished content otherwise - sounds like a big twisted problem). So this is essentially the crux of what Sun brought up elsewhere, I think it affects us less because we let people edit less of the problematic things (menu is another example but is not possible to make visible as a field right now; however you never know where D8 ends up :).

I'm looking into rerolling the patch to tempstore-deleting the item on save and on edit initiation.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
14.56 KB
864 bytes

Minor update for the first suggestion.

I'm not sure we have a clear place to know when editing is *started* though. If it is the metadata callback, then we loose all changes if the page is reloaded for some reason (eg. your browser hangs and is relaunched like it happens to me sometimes on mobile). Not supporting page reload sounds like would be a problem. Otherwise I don't think there is an existing indication as to what *starts* an edit. Which is similar to not knowing when an edit ends if not saved, but is even more ridiculous that we don't know I think :D

Moving to needs review, since this looks like needing more discussion not right into implementation.

Status: Needs review » Needs work

The last submitted patch, edit-tempstore-15.patch, failed testing.

Wim Leers’s picture

#14: Indeed, far-reaching consequences. So… do you see a way to continue with that/work around that? :)

#15: Detecting when editing is *started* needs to happen on the client-side. So, EditController::fieldForm() should probably receive an additional parameter to indicate whether the TempStore entry for the current entity should be cleared (i.e. "editing started") or not (i.e. "continued editing").

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
14.56 KB
720 bytes

It helps to not add random syntax elements on the middle of lines.

Gábor Hojtsy’s picture

FileSize
3.29 KB
15.54 KB

Added tempstore restoration capability in the form callback. I was not sure how best to write this into the JS *and* I did not want to venture there too much to leave it a clean slate for Jesse.

Status: Needs review » Needs work
Issue tags: -sprint, -Spark

The last submitted patch, edit-tempstore-19.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +sprint, +Spark

#19: edit-tempstore-19.patch queued for re-testing.

Gábor Hojtsy’s picture

I've opened #1979566: Use request upcasting functionality in edit module which simplifies the upcasting of entities in the URL and would remove the need for some duplicate code in this patch to handle the {entity_type}/{entity} combo in the URL so they are upcast to the properly loaded entity. I don't want to make this postponed on that though, so whichever lands first will make the other one needing to react and adapt. Looks to me a better strategy to keep improvements flowing here.

jessebeach’s picture

Rerolled on 891f4c5d6928d122bc1b255327f7077599f3eab3.

jessebeach’s picture

Rerolled on 8.x (50ae3296382669baebee8b9afa909361eb5aa412).

This is the conflict I resolved:

-107,9 +161,9 @@ public function fieldForm(EntityInterface $entity, $field_name, $langcode, $view)
if (!empty($form_state['executed'])) {
-      // The form submission took care of saving the updated entity. Return the
-    // updated view of the field.
-      $entity = entity_load($form_state['entity']->entityType(), $form_state['entity']->id(), TRUE);
+      // The form submission saved the entity in tempstore. Return the
+     // updated view of the field from the tempstore copy.
+      $entity = Drupal::service('user.tempstore')->get('edit')->get($entity->uuid);
      // @todo Remove when http://drupal.org/node/1346214 is complete.
      $entity = $entity->getBCEntity();
      $output = field_view_field($entity, $field_name, $view_mode, $langcode);
jessebeach’s picture

Rerolled in anticipation of #1979784: Factor Create.js and VIE.js out of the Edit module being committed.

Had to resolve this conflict:

diff a/core/modules/edit/edit.module b/core/modules/edit/edit.module    (rejected hunks)
@@ -93,7 +93,8 @@ function edit_library_info() {
       array(
         'data' => array('edit' => array(
           'metadataURL' => url('edit/metadata'),
-          'fieldFormURL' => url('edit/form/!entity_type/!id/!field_name/!langcode/!view_mode'),
+          'fieldFormURL' => url('edit/form/!entity_type/!id/!field_name/!langcode/!view_mode/!reset_tempstore'),
+          'entitySaveURL' => url('edit/entity/!entity_type/!id/!langcode'),
           'context' => 'body',
         )),
         'type' => 'setting',
lightsurge’s picture

#14

So if we are to create that term / save that image, it would show up in the site elsewhere in views, listings, in your most recent uploads, etc. even though it is just temporary for this edit.

Maybe there's call for a 'temporary' revision with status unpublished and flagged appropriately that becomes the working entity when 'in-edit' within tempstore, which then becomes the confirmed, normal revision when a user hits 'save', inheriting whatever status the previous revision had. So essentially a revision is created soon as the overall entity is being edited, rather than if/when a user hits the overall save button after moving and editing between fields.

You could have a hook in there if for any reason status unpublished for the temporary revision is no good.

Know that's not perfect but I figure a lot of modules will check to see, for example, if there are any published nodes referencing say a taxonomy term, before they will display the term (or, at least, there's often the option to do that), and a lot of views and other routes naturally already use content as the base table rather than listing files/terms direct, so all the edit cruft wouldn't feature if indeed the site owner already didn't already want to show terms and files that only relate to unpublished content?

Just a thought. Maybe I'm misunderstanding.

Would be nice if, unlike wordpress, all this stuff is done away with if it doesn't make it into the confirmed changed content, though. Whenever I use that I end up with media libraries full of rubbish that I thought I was going to use but didn't bother.

jessebeach’s picture

Rebased on 3ad81ec0a6b8cba88cabd0b422c3f9cd5f750f91. Rerolled because of this conflict:

diff a/core/modules/edit/edit.module b/core/modules/edit/edit.module    (rejected hunks)
@@ -53,7 +53,8 @@ function edit_page_build(&$page) {
     'type' => 'setting',
     'data' => array('edit' => array(
       'metadataURL' => url('edit/metadata'),
-      'fieldFormURL' => url('edit/form/!entity_type/!id/!field_name/!langcode/!view_mode'),
+      'fieldFormURL' => url('edit/form/!entity_type/!id/!field_name/!langcode/!view_mode/!reset_tempstore'),
+      'entitySaveURL' => url('edit/entity/!entity_type/!id/!langcode'),
       'context' => 'body',
     )),
   );

I'm not sure why it came up again, but here we are. It might be because I based the patch in #25 off of a branch with #1979784-30: Factor Create.js and VIE.js out of the Edit module committed to it. Maybe there are conflicts with that patch. I'll find out when I put them all together to form In-Place Editor Voltron in a moment.

I also moved changes from #1678002-114: Edit should provide a usable entity-level toolbar for saving fields into this patch and removed them from that issue.

jessebeach’s picture

It might be because I based the patch in #25 off of a branch with #1979784-30: Factor Create.js and VIE.js out of the Edit module committed to it. Maybe there are conflicts with that patch.

Yes, there is an unresolvable conflict with #1979784-36: Factor Create.js and VIE.js out of the Edit module. When that issue is committed, we'll have to reroll this one. Not a big deal.

jessebeach’s picture

Rerolled on e769455e4434ad90572a00aac415648764d9606d after #1979784-36: Factor Create.js and VIE.js out of the Edit module was committed.

Wim Leers’s picture

Status: Needs review » Needs work
+++ b/core/modules/edit/edit.services.ymlundefined
@@ -6,6 +6,10 @@ services:
diff --git a/core/modules/edit/js/util.js b/core/modules/edit/js/util.js

diff --git a/core/modules/edit/js/util.js b/core/modules/edit/js/util.js
index ad7136ec..dd8bd66 100644

index ad7136ec..dd8bd66 100644
--- a/core/modules/edit/js/util.js

--- a/core/modules/edit/js/util.js
+++ b/core/modules/edit/js/util.jsundefined

These changes don't belong in this patch, they belong in the entity toolbar patch.

+++ b/core/modules/edit/lib/Drupal/edit/Access/EditEntityAccessCheck.phpundefined
@@ -0,0 +1,70 @@
+  public function accessEditEntity(EntityInterface $entity) {
+    $entity_type = $entity->entityType();
+    // @todo Generalize to all entity types once http://drupal.org/node/1862750
+    // is done.
+    return ($entity_type == 'node' && node_access('update', $entity));

Should use $entity->access(), as Edit module is already doing by now :)

+++ b/core/modules/edit/lib/Drupal/edit/EditController.phpundefined
@@ -85,6 +87,46 @@ public function metadata(Request $request) {
+   * @todo When using entity toolbar/TempStore, this will have to handle
+   *   1) rendering form
+   *   2) validating form
+   *   3) rendering the modified field in its formatter.
+   *
+   * I don't remember if we'd said that validation should only occur when saving
+   * the entire entity, but AFAICT that's technically actually impossible:
+   * validation errors occur on the per-field level, and to be able to show each
+   * field's validation errors, we'd have to open up the "Create.js
+   * PropertyEditor widgets" ("in-place editors" if you will) for each field,
+   * and show the validation errors within that for each field. Imagine a image,
+   * taxonomy, file, entity reference, etc. field, all being opened up at the
+   * same time. That will be a disastrous UI. We could potentially make it work
+   * to just have all field validation errors at the top (in the entity toolbar?)
+   * like for a regular form validation, but then the user would still have to
+   * click into each invalid field (which would likely have red outlines) to
+   * open its Create.js PropertyEditor widget to be able to enter a valid value.
+   * That would require big changes in the JS, PHP and overall UI, so I think it
+   * would be better to defer that to a second iteration. If we can make it work
+   * this way, that will probably be easier.
+   *
+   * Another technical aspect that we haven't addressed yet is the session
+   * aspect. We don't support starting to edit an entity via in-place editing
+   * (and having it live in TempStore) today, then browsing away and then start
+   * in-place editing it again tomorrow and having that continue where you left
+   * off.
+   * So: how to determine when to *clear* the existing data in TempStore?
+   *
+   * Finally, the tricky bit here is that (AFAICT, see above) we want validation
+   * but not submission. So… we want to trigger the "save" process, without
+   * actually saving. This can be pretty easy, or insanely hard. Especially
+   * because we DON'T want "entity (pre)save" or "field (pre)save" hooks to be
+   * fired… (that should only happen in entitySave() below).
+   *
+   * And after all of the above, there's sun's big comment at
+   * http://drupal.org/node/1510544#comment-6988746 (#130) which argues that
+   * using the TempStore for content entities is a bad idea in general, that we
+   * should use revisions for this. Also definitely see comments #140, #141 and
+   * #151.

Remove this scaffolding comment :)

+++ b/core/modules/edit/lib/Drupal/edit/EditController.phpundefined
@@ -134,4 +186,49 @@ public function fieldForm(EntityInterface $entity, $field_name, $langcode, $view
+   * @todo All fields should already be valid, and the currently rendered
+   * representation on the client side should already be up to date. Both thanks
+   * to fieldForm() above. So, this hook should really do:
+   *  - take the entity from TempStore and save it into the database
+   *  - ensure the relevant "entity/field (pre)save" hooks are fired
+   *  - remove the entity from TempStore upon success
+   *
+   * Question is whether we can really assume there will be no validation
+   * errors? Can *nothing* go wrong?
+   *
+   * If you were to follow the approach I outlined (and it would be possible to
+   * make it work this way), then:
+   * - in-place editing should continue to work as it does today, with the
+   *   current UI
+   * - BUT even though the changes would be immediately visible, as if they were
+   *   being saved like they are today, they would really just live in tempstore
+   * - Doing a GET request to edit/entity/<entity type>/<entity ID>
+   *   would cause it to really be saved into the DB. Just using the browser
+   *   should be fine; you don't have to do it via AJAX.
+   *
+   * This means you *should* be able to not change a line of JavaScript — if I

Also remove this comment :)

+++ b/core/modules/edit/lib/Drupal/edit/EditController.phpundefined
@@ -134,4 +186,49 @@ public function fieldForm(EntityInterface $entity, $field_name, $langcode, $view
+    // @todo add response that makes sense.
+    $output = array();

We have output below, delete these two lines.

tim.plunkett’s picture

+++ b/core/modules/edit/lib/Drupal/edit/EditController.phpundefined
@@ -99,6 +141,16 @@ public function metadata(Request $request) {
+    $tempstore_entity = Drupal::service('user.tempstore')->get('edit')->get($entity->uuid);

@@ -107,9 +159,9 @@ public function fieldForm(EntityInterface $entity, $field_name, $langcode, $view
+      $entity = Drupal::service('user.tempstore')->get('edit')->get($entity->uuid);

@@ -134,4 +186,49 @@ public function fieldForm(EntityInterface $entity, $field_name, $langcode, $view
+    $tempstore = Drupal::service('user.tempstore')->get('edit');
+    $tempstore->get($entity->uuid)->save();
+    $tempstore->delete($entity->uuid);

You should have user.tempstore injected into your Controller.

And this should use uuid().

When you have it injected, do $this->tempstore = $temp_store_factory->get('edit'); or similar.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
10.02 KB

Only implemented @Wim's suggestions so far:

- removed the two big scaffolding comments
- removed the declaration of the entitySaveURL pattern because it was not actually used (also it was incorrect :D)
- used simple $entity->access() in the access checker
- removed core/modules/edit/js/util.js
- removed the superfluous two lines from the output generation

Did not address @tim.plunkett yet.

Wim Leers’s picture

Note that the removal of the changes in core/modules/edit/js/util.js will now have to exist in the #1678002: Edit should provide a usable entity-level toolbar for saving fields patch again.

Gábor Hojtsy’s picture

@tim.plunkett:

- Converted the use of the uuid property to the uuid() method instead.

- I made the tempstore injectable from the route controller; I did not find docs on route controllers supporting arguments (like services), so I made it default to the global Drupal::service('user.tempstore') - better suggestions welcome :) we can maybe also use $this->container, but as per http://api.symfony.com/2.0/Symfony/Component/DependencyInjection/Contain... it looks like this is set via a method later, so it would not be available in the constructor; so again better suggestions welcome

- I passed on the tempstore factory to the form class instance via the build method which is our entry point to that

Now this theoretically looks good to me. My manual testing of the patch resulted in Chrome reporting *both* a 404 on the form display and a "A fatal error occurred:" output from the response without further details (which should not make it a 404?).

Also not very happy with the capitalisation and underscore mish-mash but that is how its used elsewhere.

Wim Leers’s picture

The 404s reported in #34 are not caused by the patch in #34, but 6fa50c64eb5bd5a765c89968b0725921ae777358 have broken the edit_field_form route, presumably because of the {view_mode} in there.

jessebeach’s picture

Status: Needs review » Needs work

The last submitted patch, edit-tempstore-1901100-36.patch, failed testing.

jessebeach’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
14.14 KB

I rolled #36 incorrectly. Here's the update.

Wim Leers’s picture

Issue tags: +Needs tests

This definitely needs test coverage.

Gábor Hojtsy’s picture

@Wim Leers: can you elaborate on how you see test coverage would be best implemented? A full circle edit form display + submission test would be useful but Edit module does not even have that yet if I'm not mistaken(?).

Wim Leers’s picture

#40: There is test coverage for #1821906: Allow more Field API public API functions to act on a single field within an entity, which is what Edit uses. Edit maintains no state. This patch brings a whole lot of state maintaining (and state clearing at the right time).
The test coverage added in #1821906: Allow more Field API public API functions to act on a single field within an entity already ensures that if a commit breaks the ability to generate/validate/submit single field forms, then tests will fail. We need similar test coverage for entities that live in TempStore, so that if a commit changes something in Entity or TempStore or Field API, then we'll know whether it broke single field forms that update the entire entity stored in TempStore.

I hope that made sense :)

Gábor Hojtsy’s picture

A quick reroll first since the prior patch did not apply cleanly anymore.

Gábor Hojtsy’s picture

Adding test coverage. It is not very nice but I don't know of a nicer way to get the form id and token to use, so I grep that out of the response. Then use that info to build a post payload with a new body text, test if the new body text is printed back from the response (with the correct edit command) but not saved to the public version of the node. Then invoke the save command on the entity and check that the public version of the entity is saved.

Suggestions to make this cleaner or improve the test in other ways are welcome!

Wim Leers’s picture

Status: Needs review » Needs work
+++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.phpundefined
@@ -144,9 +144,49 @@ function testUserWithPermission() {
+      // Submit field form and check response.

Should mention this causes the modified field value to be stored in TempStore.

+++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.phpundefined
@@ -144,9 +144,49 @@ function testUserWithPermission() {
+      // Ensure the text on the original node did not change yet.
+      $this->drupalGet('node/1');
+      $this->assertText('How are you?');

Good :)

+++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.phpundefined
@@ -144,9 +144,49 @@ function testUserWithPermission() {
+      $response = $this->saveEntityEdits('node/1');

Should have a comment that this moves the modified field values from TempStore to the database: it actually saves the entity.

+++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.phpundefined
@@ -188,6 +228,39 @@ protected function retrieveMetadata($ids) {
+  protected function submitFieldForm($field_id, $post) {

AFAICT this is not necessary, you should be able to use WebTestBase::drupalPostAJAX()?

That method submits a form like ajax.js would (which is precisely what we're doing here), plus it returns the AJAX commands in the response (like we need).

If true, this would remove most ugliness — though that is a pretty impressive bit of ugliness :D

+++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.phpundefined
@@ -214,6 +287,31 @@ protected function retrieveFieldForm($field_id) {
+  protected function saveEntityEdits($entity_type_id) {

Nitpick: Maybe rename this method to just saveEntity()?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
18.42 KB
2.74 KB

Ok, first of all I looked at drupalPostAJAX(). It does not seem to be applicable to this case. It can post forms that are in plain HTML but not post against a JSON payload. What it does is it gets the URL first with drupalGet() and then matches the HTML with xpath(). The JSON payload does not have unencoded HTML like that, so form elements will not be matched. It fails right away at the start of drupalPostAJAX where it attempts to find the triggering element.

I tried filling in $this->content with $ajax_commands[0]['data'] in the test, which then found the form elements at least, but I'd still need to mock drupalSettings (notices there from drupalPostAJAX for those not present). And even then, since now I'd need to invoke drupalPostAJAX() without a path, it will post to /system/ajax, which is not the controller want to invoke here.

So looks to me like we'd need lots of workarounds and even then not get what we needed.

I modified the preg's slightly so now they work off the already parsed AJAX commands, making them nicer. I could make xpath() run on them, but that would likely be even more code and not necessarily less obscure.

Wim Leers’s picture

Issue tags: -Needs tests

Right :/ Sorry for putting you on the wrong track :( The remainder of drupalPostAjax() looks good though. But that's out of scope here probably.

Other than that, this looks good to go to me! I won't mark this RTBC, since this needs to be merged with #1678002: Edit should provide a usable entity-level toolbar for saving fields for commit.

Yay! So close now :)

Wim Leers’s picture

Reroll on top of 56f4362ad49ae274dfe6296678f78fb6bf6d7ed3.

jessebeach’s picture

Reroll on top of 1745bd21e2105f3be03d3a53d299343a525c1cb9

Berdir’s picture

Are you still having issues with the tempstore or is that working now?

Wim Leers’s picture

Berdir: it appears to be working fine for the Taxonomy term reference field right now, but not for images. For images, I get this:
Fatal error: Call to undefined method PDOStatement::fetchField() in /Users/wim.leers/Work/drupal-tres/core/lib/Drupal/Core/Database/Query/Merge.php on line 418

If you have the bandwidth to test this yourself: you can apply #48 + #1678002-146: Edit should provide a usable entity-level toolbar for saving fields on top of 1745bd21e2105f3be03d3a53d299343a525c1cb9 to reproduce it locally.

jessebeach’s picture

Here's the full stack trace:

[17-Jun-2013 03:01:02 UTC] PHP Fatal error:  Call to undefined method PDOStatement::fetchField() in /Users/jbeach/code/drupal/core/d8/core/lib/Drupal/Core/Database/Query/Merge.php on line 418
[17-Jun-2013 03:01:02 UTC] PHP Stack trace:
[17-Jun-2013 03:01:02 UTC] PHP   1. {main}() /Users/jbeach/code/drupal/core/d8/index.php:0
[17-Jun-2013 03:01:02 UTC] PHP   2. drupal_handle_request() /Users/jbeach/code/drupal/core/d8/index.php:13
[17-Jun-2013 03:01:02 UTC] PHP   3. Symfony\Component\HttpKernel\Kernel->handle() /Users/jbeach/code/drupal/core/d8/core/includes/bootstrap.inc:1904
[17-Jun-2013 03:01:02 UTC] PHP   4. Drupal\Core\HttpKernel->handle() /Users/jbeach/code/drupal/core/d8/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/Kernel.php:190
[17-Jun-2013 03:01:02 UTC] PHP   5. Symfony\Component\HttpKernel\HttpKernel->handle() /Users/jbeach/code/drupal/core/d8/core/lib/Drupal/Core/HttpKernel.php:52
[17-Jun-2013 03:01:02 UTC] PHP   6. Symfony\Component\HttpKernel\HttpKernel->handleRaw() /Users/jbeach/code/drupal/core/d8/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:61
[17-Jun-2013 03:01:02 UTC] PHP   7. call_user_func_array() /Users/jbeach/code/drupal/core/d8/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:117
[17-Jun-2013 03:01:02 UTC] PHP   8. Drupal\Core\Controller\AjaxController->content() /Users/jbeach/code/drupal/core/d8/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:117
[17-Jun-2013 03:01:02 UTC] PHP   9. Drupal\Core\HttpKernel->forward() /Users/jbeach/code/drupal/core/d8/core/lib/Drupal/Core/Controller/AjaxController.php:55
[17-Jun-2013 03:01:02 UTC] PHP  10. Drupal\Core\HttpKernel->handle() /Users/jbeach/code/drupal/core/d8/core/lib/Drupal/Core/HttpKernel.php:81
[17-Jun-2013 03:01:02 UTC] PHP  11. Symfony\Component\HttpKernel\HttpKernel->handle() /Users/jbeach/code/drupal/core/d8/core/lib/Drupal/Core/HttpKernel.php:52
[17-Jun-2013 03:01:02 UTC] PHP  12. Symfony\Component\HttpKernel\HttpKernel->handleRaw() /Users/jbeach/code/drupal/core/d8/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:61
[17-Jun-2013 03:01:02 UTC] PHP  13. call_user_func_array() /Users/jbeach/code/drupal/core/d8/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:117
[17-Jun-2013 03:01:02 UTC] PHP  14. Drupal\edit\EditController->fieldForm() /Users/jbeach/code/drupal/core/d8/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:117
[17-Jun-2013 03:01:02 UTC] PHP  15. drupal_build_form() /Users/jbeach/code/drupal/core/d8/core/modules/edit/lib/Drupal/edit/EditController.php:156
[17-Jun-2013 03:01:02 UTC] PHP  16. drupal_process_form() /Users/jbeach/code/drupal/core/d8/core/includes/form.inc:416
[17-Jun-2013 03:01:02 UTC] PHP  17. form_execute_handlers() /Users/jbeach/code/drupal/core/d8/core/includes/form.inc:922
[17-Jun-2013 03:01:02 UTC] PHP  18. call_user_func_array() /Users/jbeach/code/drupal/core/d8/core/includes/form.inc:1540
[17-Jun-2013 03:01:02 UTC] PHP  19. Drupal\edit\Form\EditFieldForm->submit() /Users/jbeach/code/drupal/core/d8/core/includes/form.inc:1540
[17-Jun-2013 03:01:02 UTC] PHP  20. Drupal\user\TempStore->set() /Users/jbeach/code/drupal/core/d8/core/modules/edit/lib/Drupal/edit/Form/EditFieldForm.php:103
[17-Jun-2013 03:01:02 UTC] PHP  21. Drupal\Core\KeyValueStore\DatabaseStorageExpirable->setWithExpire() /Users/jbeach/code/drupal/core/d8/core/modules/user/lib/Drupal/user/TempStore.php:153
[17-Jun-2013 03:01:02 UTC] PHP  22. Drupal\Core\Database\Query\Merge->execute() /Users/jbeach/code/drupal/core/d8/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php:102
Berdir’s picture

I tried to reproduce this in Portland, and editing and saving an image worked fine for me. What PHP version are you using? If you don't have 5.4, can you try it with that version?

Can you provide exact steps on what to do to trigger this bug?

I think a test that triggers this would be useful, as low-level as possible, e.g. directly using keyvalue or even a merge query to try and save the data that triggers the bug.

jessebeach’s picture

@Berdir I tried with PHP 5.4.4 and got the same error. I usually run 5.3.14.

I don't have the chops to put together a test for this layer of the stack. PDO is the thing in Drupal I know the least. But I can make screencasts! Here are the steps I followed to trigger the error, with debugging thrown in.

https://www.facebook.com/video/video.php?v=990353002486

There's an HD option on the video player. Make sure you've clicked this and made it full-screen.

The big difference I noticed is that when I arrive in Merge.php from the image save sequence, the result of $select->execute() is a string rather than an object. I poked around for an hour but I can't figure out why it would return a string in this one instance. In the screencast, I changed this code

if (!$select->execute()->fetchField()) {...}

to this, so I could see the value returned by execute before the fatal error is thrown.

$result = $select->execute();
if (!$result->fetchField()) {...}

The field save sequence is controlled by \Drupal\edit\EditController::fieldForm. In fieldForm(), the trouble starts when $form = drupal_build_form('edit_field_form', $form_state); is called.

Berdir’s picture

That was an interesting one :)

It's related to serialize/unserialize, looks like we're working on an unserialized database connection. And in that case, we're working with a PDO connection that's missing the correct statement class setting. So we get the default PDOException, which doesn't have fetchField() (which is a custom wrapper for fetchColumn that we added for DX)... and kaboom.

And *after* I figured that out and had a fix, @alexpott linked me to #2004350: Unserialised database connection objects don't use Drupal's PDO Statement class. Apply that patch and it should work :)

jessebeach’s picture

Patch applied from #2004350-4: Unserialised database connection objects don't use Drupal's PDO Statement class and it worked!!

Berdir, you're the best. HIGH FIVE.

swentel’s picture

+++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.phpundefined
@@ -252,6 +328,31 @@ protected function retrieveFieldForm($field_id) {
+    // Perform HTTP request.
+    return $this->curlExec(array(
+      CURLOPT_URL => url('edit/entity/' . $entity_type_id, array('absolute' => TRUE)),
+      CURLOPT_POST => TRUE,
+      CURLOPT_POSTFIELDS => $post . $this->getAjaxPageStatePostData(),

Just out of curiosity, why are we not using Guzzle instead of curl ?

Wim Leers’s picture

#56: because this is a variant of tests that already exist. Crell made the same remark elsewhere before. This is not newly invented code, this is copying an existing pattern because the API provided by WebTestBase is too limited. Once WebTestBase is converted to Guzzle, I'll be happy to change this too. Until that day, I don't want to waste time on refactoring *tests*, which doesn't buy us anything.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Since #46, this issue has been de facto RTBC. However, it did not make sense to commit this without #1678002: Edit should provide a usable entity-level toolbar for saving fields. I just marked that issue RTBC, hence this one should be as well.

This patch adds test coverage for changed/new functionality within Edit's AJAX endpoints that are used by Edit's JS.

See #1678002-176: Edit should provide a usable entity-level toolbar for saving fields for more details.

jessebeach’s picture

Issue tags: -sprint, -Spark

#48: edit-tempstore-1901100-48.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, edit-tempstore-1901100-48.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +sprint, +Spark

#48: edit-tempstore-1901100-48.patch queued for re-testing.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

The test fail was a fluke.

jessebeach’s picture

Priority: Normal » Major

Raising to major. This should have been committed with #1678002: Edit should provide a usable entity-level toolbar for saving fields.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

dawehner’s picture

Just a couple of points I realized when reviewing the patch.

+++ b/core/modules/edit/lib/Drupal/edit/Access/EditEntityAccessCheck.phpundefined
@@ -0,0 +1,67 @@
+    return $entity->access('update');

Access checkers should return static::ALLOW and static::DENY.

+++ b/core/modules/edit/lib/Drupal/edit/Access/EditEntityAccessCheck.phpundefined
@@ -0,0 +1,67 @@
+      if (!$entity_type || !entity_get_info($entity_type)) {
...
+      $entity = entity_load($entity_type, $entity_id);

This information should be better injected.

+++ b/core/modules/edit/lib/Drupal/edit/Access/EditEntityAccessCheckInterface.phpundefined
@@ -0,0 +1,22 @@
+interface EditEntityAccessCheckInterface {

It is hard to see why this need to be a new interface? There is no single place in core where this is called, beside of the only class which implements it.

+++ b/core/modules/edit/lib/Drupal/edit/EditController.phpundefined
@@ -24,6 +27,23 @@
+  public function __construct(TempStoreFactory $temp_store_factory = NULL) {
+    $this->tempStoreFactory = $temp_store_factory ?: Drupal::service('user.tempstore');
+  }

I don't get why we even need lazy-injection, can't we just implement ControllerInterface and inject the dependency?

+++ b/core/modules/edit/lib/Drupal/edit/EditController.phpundefined
@@ -110,17 +130,27 @@ public function attachments(Request $request) {
+    if ($tempstore_entity && !(isset($_POST['reset']) && $_POST['reset'] === 'true')) {

$_POST could be easily replaced by using the information on the $request object.

jessebeach’s picture

Issue tags: -sprint, -Spark

Removing tags.

Wim Leers’s picture

Issue tags: +Spark

.

jessebeach’s picture

Added a followup to address @dawehner's concerns in #65: #2033433: Clean up edit controller

catch’s picture

Status: Fixed » Active

Just seen this, not really sure about the approach at all. #1678002: Edit should provide a usable entity-level toolbar for saving fields was only RTBC for two hours before commit and barely mentions temp store, so missed it there as well.

It looks like there's potential race conditions that weren't discussed here:

1. What happens if two people are editing the entity at the same time via inline editing? Are they going to get different tempstore objects or the same one? If they're the same, then there'll be conflicts while editing. If they're different, there'll be conflicts on the final entity save. This is much less of a problem with revisions.

2. What happens if the entity is saved via the 'normal' process while someone is editing inline. Views uses a lock on the tempstore for multiple people editing a view, but there's two different entities here. That can happen with revisions but at least there's an atomic record of each edit, so if people overwrite each other nothing gets lost as such.

3. What happens if an entity is in the temp store and the structure of the entity changes in the meanwhile - for example running an update or similar. This is is possible with a normal form submission but feels like there's a longer window introduced here.

Finding a way to garbage collect revisions seems like it might have saved some of these problems - i.e. flag a revision as potentially disposable then have configuration for clearing those out (or not) on a schedule.

Gábor Hojtsy’s picture

We considered unpublished revisions extensively from a workflow standpoint. As soon as you have an unpublished forward revision, the assumptions of the edit module UI do not really hold. When you see an entity and click to quick edit it, it would need to re-render the whole entity based on the unpublished forward revision (it may have other set of fields, different values, etc). If it does not do that, some of the fields on the forward revision may not be editable (they are not visible), or when you click certain fields, it will start editing different values (loading from the forward revision instead of what was visible). Also, it would need a way to get out of the unpublished state and make you know if you are editing an unpublished state of a published thing, so in other words, much more extensive workflow support.

Gábor Hojtsy’s picture

@Wim asked me to re-explain in different words to make it clear :)

In other words revisions may mitigate some of the concurrency problems but they introduce this workflow where you don't actually edit what you see. So to solve multiple people editing the same thing, you don't really edit what you see, because it is already in a later temporary state. Which is a whole can of worms for UX then. Drupal core cannot really aim to have a google docs / etherpad style collaborative editing thing.

I think to keep the UX simple and avoid possibilities for concurrency, we would need to have an entity edit lock that is shared between regular editing and in place editing, so you cannot go into editing something locked.

Also I'm not sure this is really a longer window vs. form submission. I regularly write / update long posts in Drupal entity forms. Updating a sizeable change notice or issue summary by multiple people are drupal.org use cases.

klonos’s picture

Regarding point 1. I guess that we could check on node edit if any tempstore object is already attached to the entity and then throw a "locked content - somebody else is currently editing" message. That though raises the question of for how long do we allow a piece of content to be locked.

A solution for that would be to branch the entity into revisions instead of locking content. A warning would still be displayed that the node is already being edited by another user (or by multiple users if the is the case), but the user has the option to ignore it and continue with their edit. So each user that invokes either an inline edit or a full node edit form gets their own "snapshot" (think VM snapshots) to work on.

Now, here comes the tricky part... after both edits are done, which revision is the one to be published (actually the one to replace the default because the original node might not be in published state to begin with)? I think that it is proper to use first-come first-served logic, but there could be solutions in contrib to override that behavior.

So, taking for granted that only the first user to claim edit has the privilege to replace the default revision (for the sake of this scenario), we then have the option to either present other users with a diff after save end let them revise their edit or simply save future revisions and let users with the right permissions review and decide.

Now, if the user that initiated the first edit cancels, then the next user in line becomes privileged to save their revision as the default instead of a future revision.

Does this workflow make sense?

Gábor Hojtsy’s picture

Drupal is not supposed to support branched revisions in any capacity. I think this complexity is way above what we should focus on in this phase.

klonos’s picture

I agree. Perhaps we should go with the simple(ier) solution of content locking then.

catch’s picture

Just a simple content lock (on both inline and node edit form) seems reasonable. Would be good to leave it open for more exotic stuff in contrib.

tim.plunkett’s picture

We have KeyValueStoreExpirableInterface, which user.tempstore uses.
Also Views has the content locking.
See Drupal\views_ui\Form\BreakLockForm, Drupal\views_ui\ViewUI::isLocked(), etc.

Gábor Hojtsy’s picture

Implementing this only for edit module sounds like a bit of an issue for me. We also want other edits of entities to block possible race conditions. If I retrieve a node through an API to edit, and then submit it, should it do the same thing? If I have a normal edit form and an in-place edit, should those two interact within the locking? Users should not be able to edit a custom block with tags on a normal form and then overwrite that from edit module, right?

So I looked at what node module does now, and what it does is when in form submission, it validates the latest loaded changed timestamp vs. the timestamp of the node entity in the form when the form was generated. Then it fails the form if the node changed in the meantime. I think ideally we want to expand this to one level higher in the entities, and validate entity saves with this in general? That requires that changed timestamps are on entities (which I'm working on at #2074191: [META] Add changed timestamp tracking to content entities). Then moving up the changed timestamp checking from the node form to entities in general it would hopefully work across different modes of edit. "Normal" web based forms, in-place editing, API based content submission, etc.

Is that something that sounds like to people it makes sense or should we approach this from differently? Locking custom for edit module only? What do you think?

Gábor Hojtsy’s picture

Talked with @berdir about this and he provided this great insight:

[6:12pm] berdir: GaborHojtsy: speaking of that, the node changed/locking question you asked in the edit issue..
[6:12pm] berdir: GaborHojtsy: node now has a NodeChanged validation constraint on the changed column that basically implements that, for the entity validation api
[6:23pm] berdir: GaborHojtsy: NodeChangedConstraintValidator
[6:24pm] berdir: GaborHojtsy: $old_node->validate() => gives you a validate violation if someone else saved that node

Which makes me think if edit module does not do this yet, its an issue since the field changes should save / validate the whole entity save (which may launch hooks, that are appropriate with a field change even).

Wim Leers’s picture

Indeed, if Edit module doesn't do it yet, that would be a big issue.

However, NodeChangedConstraint is an API addition/change that happened on July 29, long after Edit was committed. They should've made sure Edit was compatible with it and should've extended EditLoadingTest.

Also: this would only solve the problem for nodes! Why wasn't an EntityChangedConstraint created, that we could apply to other entities (e.g. Term) as well?

Gábor Hojtsy’s picture

Because other entities don't have a changed timestamp? See #2074191: [META] Add changed timestamp tracking to content entities.

Wim Leers’s picture

Fair enough, I can see how that could've been premature :)

Gábor Hojtsy’s picture

So as part of #2074191: [META] Add changed timestamp tracking to content entities it turns out changed timestamp tracking will not be intrinsic to all entities. We do have an interface now though, which we can use to check if the entity we used for editing is outdated. I think ideally we'd add a generic editing/overwrite conflict detection system to entity forms, but entities will not universally have this capability. Also, @webchick asked me to present at least an interim solution without spinning off further general solutions :)

So here is a solution that should be good at least for the interim:

- when retrieving / saving a field with edit module, check changed times with the live entity vs. the tempstore entity
- do the same when saving from the tempstore to the live entity
- if either place we find a mismatch, freak out

This is implemented and seems to function well for the field editing. I did have some issues reproducing it all the time on the frontend but I wrote a nice test for it to ensure it always works. So I blame JS glitches for it not appearing 100% on the frontend. The patch uses the regular error reporting mechanism that is already present for fields, so the error will *pop up on the field* like so:

edit-conflict-error.jpg

I did not really know how to make the error reporting happen on the save callback, since that does not have any error handling for some reason. Not sure how the JS would react to starting to throw random errors. I think it would be beneficial either way if the save handler could deal with errors. As-is (before/after the patch equally), only success is returned from the save callback. I made the save not happen for now though.

Feedback warmly welcome! @Wim Leers?

Bojhan’s picture

I thought we had some inline edit way to handle errors like that? That it becomes part of the top of the enity bar?

klonos’s picture

typo in the message text:

The copy of the content being edites...

Gábor Hojtsy’s picture

@Bojhan: the same UI is used to display errors with form validation. Eg. if the field is an integer field and there are min/max values, the error would some back the same way from the server ATM.

Gábor Hojtsy’s picture

Status: Active » Needs review
Issue tags: +sprint
FileSize
7.3 KB

Reuploading #82 with typo "edites" => "edited" fixed. Looking for reviews.

Gábor Hojtsy’s picture

Discussed this with @berdir:

[4:14pm] berdir: GaborHojtsy: right now, edit.module calls field attach validate stuff. that's deprecated and will go away in favor of $entity->validate()
[4:14pm] berdir: GaborHojtsy: so you will have to switch to that. When you do that, you will get node changed validation support for free. I haven't looked at your code yet, but it seems strange to me to build something custom, even if it's based on the interface
[4:15pm] GaborHojtsy: berdir: I think we also want to see a cross-entity solution here, not just nodes
[4:16pm] GaborHojtsy: berdir: I assume you say we should do the node changed validator for all entities 1-1
[4:16pm] GaborHojtsy: berdir: ?
[4:18pm] berdir: GaborHojtsy: I understand you need something now, it's just that it seems wrong to introduce custom code for this. I'd rather focus that time on making ->validate() work. because you will have to do that anyway
[4:20pm] GaborHojtsy: berdir: right, do we have a change notice I can learn from how field attach validate should be replaced with ->validate()?
[4:22pm] berdir: GaborHojtsy: not sure about change notice, if you want to see some code that uses it, look at either EntityResource::validate() in rest.module or EntityFormController::validate()

Looking at Drupal\Core\Entity\EntityFormController::validate() there is lots of double coding for NG and BC entities. I think WimLeers argued earlier that we don't want to use the entity form controller but want to have our own controller here for per-field editing reasons. We'll need to implement the validation for ourselves then like in the form controller, delegating to the field and entity validation. Not sure if we only validate the field like it is now, how it would get us entity validation (and therefore changed time validation) for free though.

Gábor Hojtsy’s picture

Also got actual code review from @berdir:

[4:47pm] berdir: GaborHojtsy: as for actual review of your code, I noticed two things: a) the node changed code queries the the database directly, to make sure that it's really current data. You could use loadUnchanged() to do something similar in a bit more generic way (that is overhead, but if you assign it to $entity->original, you avoid the second call that would happen in the storage controller)
[4:48pm] berdir: GaborHojtsy: b) the test smells like it could fail randomly, if the server is fast enough to both requests at the same time?

Not sure if to act on this ATM, will discuss with @Wim Leers.

Gábor Hojtsy’s picture

Here is another take trying to use field validation. It does not work yet, but I *need* feedback/help. Some quirky things:

1. We don't want to validate the whole entity, since we don't display the whole entity; we want to validate / save changes only to the fields we displayed; eg. if you add a new required select field to your entity and you don't display it, we want to keep it editable even if you did not yet specify the value on the instance of the entity.

2. So we want to keep the field-edit-validate only approach, but to validate the changed timestamp, we need to run a validation for that only. We don't know the name of the field, since that is not mandatory to be 'changed' (not even that for file entities ATM), so we need to backwards engineer that from the constraint assignment. Fun stuff.

3. Then we need to put the errors into the form. However the changed field does not have a form element counterpart, so it will not show up, and therefore the failing part of the patch.

Note that the added code here is 99% the same as in field_attach_form_validate() which is almost the same (for NG entities) as in EntityFormController::validate(). Not sure we should make more copies of this code, but since this uses a mix of procedural stuff anyway, not sure either what we should do to abstract this anywhere... Feedback welcome!

Status: Needs review » Needs work

The last submitted patch, edit-concurrency-fieldapi.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
6.02 KB

Here is a reroll and slightly updated patch which should pass (it passes for me :). No interdiff, since the test did not apply or work anymore due to recent changed around drupalPost/drupalPostForm, etc. as well as form langcode changes so it needed massive changes to even apply.

Once again, the crux of the problem here is we **don't want to validate the whole entity**, we want to validate a field that is being edited and the changed timestamp. Not the other fields. If other fields have issues (maybe not even visible in the display), that should not stop edit module from working. The entity form controller solution that have been pointed out above as *the new way* of validating validates all fields. The field_attach_form_validate() function is deprecated, but there seems nothing in its place (the entity form validator copy-pastes most of it basically). So we cannot get one field validated in a *new way*.

Then even if we could do that for the changed field, we still need to (1) find out the name of the field, so we can get that one validated only. We need to traverse the entity definition and see if there is a field with the changed constraint for this - not very nice but we have no other options, we don't know the name. And (2) we need to bubble up the validation error in the form. Since there is obviously no form element included to edit/display changed date, we need to hack around the form to get this error move onwards in the system and get it block edit module and display that error. We could theoretically include a changed field in the form with some special name, eg. 'edit_changed_time' but we'd need to avoid any name clashes with actual fields that there may be... Then we could attach form errors to that. I think that is similarly hackish but the hack would be in the form build and similar hacks would remain in form validation instead of hacks being in the AJAX rendering.

Feedback welcome!

catch’s picture

Hmm. I actually think it's a good thing that we spam the node revisions table - makes it possible to roll things back with some granularity. We could look at making it easier to thin out old revisions etc. though.

I brought up only validating one field in #2002162: Convert form validation of users to entity validation and it was rightly pointed out in there that field validation might depend on values in another field. So you could end up making inline edits which all pass validation OK separately, but then the actual entity save at the end would fail validation. Has that been considered here?

Gábor Hojtsy’s picture

@catch: right, the way edit module is implemented, cross-field dependencies in terms of validation is not considered. What is considered though is if we have an entity type and you add more fields (and maybe not display them), it will not fail all of the prior entities for in-place editing until you fill in the required fields on the normal edit form. Essentially we are trying to avoid displaying errors for things you cannot fix in in-place editing and by definition in-place editing only displays one field at once, so we should not display the other errors or stop the user from being able to do things if there are errors elsewhere.

Status: Needs review » Needs work

The last submitted patch, edit-concurrency-fieldapi-91.patch, failed testing.

catch’s picture

@Gabor

or stop the user from being able to do things if there are errors elsewhere.

Validation is explicitly supposed to do prevent you doing things like that, that's the point of having validation. I think this needs a separate, critical issue to discuss whether it's safe to skip that.

Gábor Hojtsy’s picture

@catch: I see edit as a use case for a limited editing experience for a bigger whole. The same "limited editing experience for a bigger whole" situation applies to translations (if you don't have perms to edit non-translatable values for example) or if there are any field level permissions applied and you only see and can only edit some of the fields. I'm not sure it would be wise to not let translations save with validation errors or changes in the entity edit form save with validation errors, if there are errors in the entity outside of the control of the form. So if you add a new required field to an entity without a default value (force people to select something) and that is not translatable or the user does not have access to it, throwing all entity validation errors for the limited forms would mean you cannot translate such entities anymore and you cannot edit them unless you have perms to edit that field and fix the error. I think the same situation applies to edit. This is a limited editing environment in pretty much the same way, it focuses on producing validation for only its environment. If the entity API does not have a solution for these limited editing environments than the same issues apply to translation forms and entities with field permissions.

Wim Leers’s picture

#69/#92: I'm terribly sorry that you didn't see the TempStore stuff happening. But TempStore vs. revisions was being discussed in that issue since December 2012 (well over 6 months before it got committed), see #25–#29 there. In #44 there it was explicitly split in a back-end half (this issue) and a front-end half (that issue). It should have been mentioned in the issue summary. I am truly sorry that you seemingly almost felt cheated, but I hope you trust that that was not at all the intention.
I can't find the comments right now where the problems of using revisions and not TempStore are being discussed. I think that the crux was this: many contrib modules implement hooks that react to new revisions, e.g. to do workflow stuff. But when creating new revisions with in-place editing, you don't really want those hooks to be fired, since it is really only temporary stuff you're dealing with. There is quite a bit of complexity with "cleaning up temporary revisions". And then there's the problem of a user potentially stopping in-place editing (e.g. navigating away) before having hit the "Save" button, which implies the user decided to just stop the in-place editing session; another user might have started editing based on those changes, but we want to clean up those temporary in-place editing revisions — now what?
(I hope I remembered that correctly. As you can see, there are a lot of tricky aspects, that we simply avoid by using TempStore.)

Finally, assuming we wouldn't remove those temporary in-place editing revisions: "rolling back things with granularity" is arguably pointless; you don't have that ability when using "full node edit", so why should you have that ability when having used "in-place node edit"? Retaining so many revisions is not only costly storage-wise, but especially UI-wise — we'd degrade the UX significantly.


#83/@Bojhan: no, making validation errors part of the entity toolbar has only been designed, not implemented, see #2026861: Implement validation in the entity toolbar and not as a separate modal dialog.


#87:

I think WimLeers argued earlier that we don't want to use the entity form controller but want to have our own controller here for per-field editing reasons.

See the stuff below from DrupalCon Prague :)


#92/#93: "Has that been considered here?" -> Yes, it has, and it cannot be applied while in-place editing, or the UX would become so poor that it'd no longer be useful to use in-place editing in the first place, because the whole point is to provide a nicer UX. IMHO: if you have validation that depends on other fields, you should just disable in-place editing.

Furthermore, as Gábor explains in #96, there are multiple other scenarios where we have limited validation occurring already:

  • translation
  • field permissions: a user may not be allowed to edit all fields
  • form display modes: a user may not be able to edit all fields

Conclusions from "hard meetings" on September 21, 2013 at DrupalCon Prague

Gábor and I together with the Field API folks have been looking into getting rid of EditFieldForm and getting the edit form for the Entity instead, and dynamically modifying the EntityFormDisplay, removing (=hiding) all components except the one for the one field that is being in-place edited. The benefit would be: all the filtering+mapping of validation errors that Gábor put together and explained in #89 and #91 would just go away — because EntityFormController would handle it for us. Hurray! Byebye, field_attach_*() & EditFieldForm!
This is the conclusion we settled on yesterday, in one of the "hard meetings" at DrupalCon Prague.

Today, I worked on making that happen. And quickly ran into a wall. The problem: EntityFormDisplay only applies to configurable fields, not to base fields (e.g. promoted, revision log message, URL alias…). I could unset() all of those. Which is very inefficient, but it could work. But then it is also still possible for contrib modules to alter the entity form and make changes, which can break that. That can happen because getting rid of EditFieldForm in favor of a "default entity edit form with a dynamically manipulated form mode" still results in a form with the same form ID; we want Edit's in-place editing forms to have different form IDs.
Everything in the above paragraph (starting at "today") was the conclusion of a discussion with amateescu and yched.

This is what amateescu and yched decided should happen:

  1. Edit must unfortunately continue to use field_attach_*() for now.
  2. Edit should generate the widget for the specific field that's being in-place edited, but doing that directly would cause problems if we'd do it today, because hook_field_attach_form() (which is called for each widget that gets generated) wouldn't be called anymore.
  3. So, we have to wait until we have something like field_attach_form(), but simpler/cleaner/better/… (I think that will happen in #2095195: Remove deprecated field_attach_form_*()), then use that, and use $entity->validate().

That means that for now we're condemned to use validation error filtering+mapping.

Gábor Hojtsy’s picture

@Wim Leers: right, that validation mapping is already done in the existing field_attach code we use, and looks like we don't have a better alternative ATM. As for the proposed patch in #91, looks like we need to do that even if we don't use tempstore, because we just validate the changed timestamp there. There is no tempstore related code touched in the patch, we need to do this for the runtime copy of the entity regardless of whether it came from tempstore or just built out of the form or from a previous unpublished revision.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
6.14 KB

Quick reroll of the patch. Looking to fix fails.

Status: Needs review » Needs work

The last submitted patch, edit-concurrency-fieldapi-99.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
5.42 KB

Slightly cleaner implementation that also works :D No need to change the form handling logic if we make up a form element for the changed field and attach the error to that as it happens. Also added more docs, eg. @return docs on getChangedFieldName(). Should fully pass. (Passes locally).

webchick’s picture

catch, tim.plunkett, and I had a discussion today on "why not revisions instead of tempstore?" Basically, catch's stance was that although tempstore clearly avoids many issues associated with revisions as Wim lays out in #97, a better approach might be to just fix those instead, and introduce something like the concept of "minor" revisions with automated garbage collection.

We reached consensus that tempstore is in fact the right pattern to use here, based on:

- There is no "autosave" functionality happening here; the edits between fields are intended to be throwaway if the node itself isn't saved after edits are done. $node->save()s between field edits, by contrast, would be making permanent changes to the node (and realistically could be 20+ revisions on a single "edit").
- If each field edit made a "first-class" revision, this would get super messy with other people coming in to make subsequent edits. They'd not know which revision to start their edits from and we'd effectively have to introduce Git-like branching of revisions to handle conflicts that could arise.

Tim pointed out that we should open up a follow-up task though for using the tempstore locking system for this to avoid the edit conflict issue, but that the solution proposed by Gábor is enough for now to close this major bug.

tim.plunkett’s picture

Category: task » bug

open up a follow-up task though for using the tempstore locking system for this to avoid the edit conflict issue

Yes please! If you've gotten this far with TempStore you can probably figure out the locking just from looking at Views UI code, but I am glad to help if you have questions.

tim.plunkett’s picture

  1. +++ b/core/modules/edit/lib/Drupal/edit/Form/EditFieldForm.php
    @@ -139,6 +148,17 @@ protected function init(array &$form_state, EntityInterface $entity, $field_name
    +    $changed_field_name = $this->getChangedFieldName($entity);
    +    if (!empty($changed_field_name)) {
    

    Could be a onliner.

  2. +++ b/core/modules/edit/lib/Drupal/edit/Form/EditFieldForm.php
    @@ -139,6 +148,17 @@ protected function init(array &$form_state, EntityInterface $entity, $field_name
    +      $changed_field_errors = $entity->$changed_field_name->validate();
    +      if (isset($changed_field_errors) && count($changed_field_errors)) {
    

    This should always return \Symfony\Component\Validator\ConstraintViolationListInterface, so no isset() needed.

  3. +++ b/core/modules/edit/lib/Drupal/edit/Form/EditFieldForm.php
    @@ -139,6 +148,17 @@ protected function init(array &$form_state, EntityInterface $entity, $field_name
    +        form_set_error('changed_field', $changed_field_errors[0]->getMessage());
    

    Is [0] reliable? reset($changed_field_errors)->getMessage(); might be better

  4. +++ b/core/modules/edit/lib/Drupal/edit/Form/EditFieldForm.php
    @@ -213,4 +233,23 @@ protected function simplify(array &$form, array &$form_state) {
    +   * Find the field name for the field carrying the changed timestamp, if any.
    

    Finds

  5. +++ b/core/modules/edit/lib/Drupal/edit/Form/EditFieldForm.php
    @@ -213,4 +233,23 @@ protected function simplify(array &$form, array &$form_state) {
    +   * @return string|NULL
    

    In typehints, should be string|null

  6. +++ b/core/modules/edit/lib/Drupal/edit/Form/EditFieldForm.php
    @@ -213,4 +233,23 @@ protected function simplify(array &$form, array &$form_state) {
    +    return NULL;
    

    This is redundant, but fine if you want to be explicit

  7. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php
    @@ -291,4 +291,47 @@ function testUserWithPermission() {
    +  function testConcurrentEdit() {
    

    public function

Wim Leers’s picture

using the tempstore locking system for this to avoid the edit conflict issue, but that the solution proposed by Gábor is enough for now to close this major bug.

I agree that the solution Gábor has been working on is sufficient for now.
I agree that the TempStore locking system is a more complete solution.

The part that I'm missing here though, is that if we're going to apply TempStore locking to in-place editing of entities (nodes or otherwise), we should also apply that to "full editing". That'd prevent the inherent frustration of two people editing the same node at the same time and then one of them having to redo their work.


In typehints, should be string|null

Interesting — I didn't know that, and judging from the typehints in core, many didn't know this. Is this rule documented somewhere?


  1. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php
    @@ -291,4 +291,47 @@ function testUserWithPermission() {
    +   * Tests Edit with concurrent node / Edit use.
    

    Tests Edit on a node that was concurrently edited through the full node form.

  2. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php
    @@ -291,4 +291,47 @@ function testUserWithPermission() {
    +    // Prepare form values for submission. drupalPostAJAX() is not suitable
    +    // for handling pages with JSON responses, so we need our own solution
    +    // here.
    

    80 col wrapping.

Gábor Hojtsy’s picture

Category: bug » task
FileSize
5.94 KB
3.76 KB

Updated patch with *almost* all of @tim.plunkett's and @Wim's comments fixed. The only thing not fixed is the [0] vs. reset(). I'm getting a getMessage() called on non-object when using reset(), getting something else out of the object instead of the first array element. May be an issue with how the array interface is implemented on constraint violation lists. I did not go there to check as it is out of scope for this patch.

I also share Wim's concerns. The overall problem is other entity forms outside of the node form don't do edit concurrency checking at all. Due to our efforts here, we actually introduced changed timestamps to some more entities (custom blocks, taxonomy terms) and then expanded on the node form model with this solution here. Generally it would be great to have edit concurrency checking for entity forms as well. The current lowest common denominator is the changed timestamp and its validator for the entity form and edit module.

Wim Leers’s picture

I'm tempted to RTBC #106, but it's probably better if for example tim.plunkett could RTBC it.

Gábor Hojtsy’s picture

Helps to name the patch a patch :)

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

This looks great, thanks for the fixes.
#105/#106 raise very good points, which will make for a very interesting follow-up :) I look forward to it!

tim.plunkett’s picture

webchick’s picture

Assigned: Gábor Hojtsy » catch

Most likely it'd be best for catch to sign off on this.

catch’s picture

Issue tags: -sprint, -Spark

Status: Reviewed & tested by the community » Needs work
Issue tags: +sprint, +Spark

The last submitted patch, edit-concurrency-fieldapi-106.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
6.01 KB

Rerolled. Marking back to RTBC as per above. Does not hurt to wait for it to be green though :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Wim Leers’s picture

Assigned: catch » Gábor Hojtsy
Issue tags: -sprint

#109: issue created: #2111427: All entity forms should implement (TempStore) locking to protect against concurrent edits. That is not a follow-up for this issue though, since it's a pre-existing problem. It's of course very clearly related to this issue, but not caused by this issue.

Now this issue can finally go and enjoy its much-deserved rest :)

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