Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#114 | edit-concurrency-fieldapi-114.patch | 6.01 KB | Gábor Hojtsy |
#108 | edit-concurrency-fieldapi-106.patch | 5.94 KB | Gábor Hojtsy |
#106 | interdiff.txt | 3.76 KB | Gábor Hojtsy |
#106 | edit-concurrency-fieldapi-106.txt | 5.94 KB | Gábor Hojtsy |
#101 | edit-concurrency-fieldapi-101.patch | 5.42 KB | Gábor Hojtsy |
Comments
Comment #1
Wim LeersNice! :)
Comment #2
Wim LeersTo get this up & running (i.e. actually usable by an end user), I think we need to:
entitySaveURL
toedit_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 exampleEditController::entitySave()
methodEntitySavedCommand.php
AJAX command, which is called byEditController::entitySave()
Comment #3
webchickMarking needs review, since there's a patch. Though it might be a needs work based on #2. :)
Comment #4
larowlanThis 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.
Comment #5
Gábor Hojtsy#1860402: Introduce a tempstore layer to in-place editing, similar to Views, that allows for atomic revisions of multiple page edits turns out was a prior duplicate.
Comment #6
Gábor Hojtsy@Wim: do we have a UI component to hinge the entity save on?
Comment #7
Wim Leers#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).Comment #8
Wim Leers#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 :)
Comment #9
Wim LeersSibling comment posted at #1678002-44: Edit should provide a usable entity-level toolbar for saving fields.
Comment #10
Wim LeersI 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.
Comment #11
Gábor HojtsyHere 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.
Comment #12
Gábor HojtsyMissed two changes (in services.yml and EditFieldForm) from the patch which were in fact in the interdiff. Now putting those changes in as well.
Comment #13
Wim LeersThis works amazingly well. Awesome job, Gábor :)
Bugs
EditController:entitySave()
.Tricky edge cases confirmed to be working correctly
edit.module
and clearing caches:These are indeed only called when the Entity is saved from the TempStore into the DB.
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
TempStore::delete($entity->uuid)
inEditController::entitySave()
.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?)
Comment #14
Gábor HojtsyAs 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.
Comment #15
Gábor HojtsyMinor 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.
Comment #17
Wim Leers#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").Comment #18
Gábor HojtsyIt helps to not add random syntax elements on the middle of lines.
Comment #19
Gábor HojtsyAdded 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.
Comment #21
Gábor Hojtsy#19: edit-tempstore-19.patch queued for re-testing.
Comment #22
Gábor HojtsyI'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.
Comment #23
jessebeach CreditAttribution: jessebeach commentedRerolled on 891f4c5d6928d122bc1b255327f7077599f3eab3.
Comment #24
jessebeach CreditAttribution: jessebeach commentedRerolled on 8.x (50ae3296382669baebee8b9afa909361eb5aa412).
This is the conflict I resolved:
Comment #25
jessebeach CreditAttribution: jessebeach commentedRerolled in anticipation of #1979784: Factor Create.js and VIE.js out of the Edit module being committed.
Had to resolve this conflict:
Comment #26
lightsurge CreditAttribution: lightsurge commented#14
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.
Comment #27
jessebeach CreditAttribution: jessebeach commentedRebased on 3ad81ec0a6b8cba88cabd0b422c3f9cd5f750f91. Rerolled because of this conflict:
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.
Comment #28
jessebeach CreditAttribution: jessebeach commentedYes, 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.
Comment #29
jessebeach CreditAttribution: jessebeach commentedRerolled on e769455e4434ad90572a00aac415648764d9606d after #1979784-36: Factor Create.js and VIE.js out of the Edit module was committed.
Comment #30
Wim LeersThese changes don't belong in this patch, they belong in the entity toolbar patch.
Should use $entity->access(), as Edit module is already doing by now :)
Remove this scaffolding comment :)
Also remove this comment :)
We have output below, delete these two lines.
Comment #31
tim.plunkettYou 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.Comment #32
Gábor HojtsyOnly 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.
Comment #33
Wim LeersNote 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.
Comment #34
Gábor Hojtsy@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.
Comment #35
Wim LeersThe 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.Comment #36
jessebeach CreditAttribution: jessebeach commentedRerolled on 6c1c3b9b96c3c31bb761e6d4dc56a15bb3eaa061 due to #1996378-12: Edit broken because of #1043198 and routing system bug + missing test coverage.
Comment #38
jessebeach CreditAttribution: jessebeach commentedI rolled #36 incorrectly. Here's the update.
Comment #39
Wim LeersThis definitely needs test coverage.
Comment #40
Gábor Hojtsy@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(?).
Comment #41
Wim Leers#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 :)
Comment #42
Gábor HojtsyA quick reroll first since the prior patch did not apply cleanly anymore.
Comment #43
Gábor HojtsyAdding 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!
Comment #44
Wim LeersShould mention this causes the modified field value to be stored in TempStore.
Good :)
Should have a comment that this moves the modified field values from TempStore to the database: it actually saves the entity.
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
Nitpick: Maybe rename this method to just
saveEntity()
?Comment #45
Gábor HojtsyOk, 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.
Comment #46
Wim LeersRight :/ 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 :)
Comment #47
Wim LeersReroll on top of 56f4362ad49ae274dfe6296678f78fb6bf6d7ed3.
Comment #48
jessebeach CreditAttribution: jessebeach commentedReroll on top of 1745bd21e2105f3be03d3a53d299343a525c1cb9
Comment #49
BerdirAre you still having issues with the tempstore or is that working now?
Comment #50
Wim LeersBerdir: 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.
Comment #51
jessebeach CreditAttribution: jessebeach commentedHere's the full stack trace:
Comment #52
BerdirI 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.
Comment #53
jessebeach CreditAttribution: jessebeach commented@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 codeto this, so I could see the value returned by
execute
before the fatal error is thrown.The field save sequence is controlled by
\Drupal\edit\EditController::fieldForm
. InfieldForm()
, the trouble starts when$form = drupal_build_form('edit_field_form', $form_state);
is called.Comment #54
BerdirThat 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 :)
Comment #55
jessebeach CreditAttribution: jessebeach commentedPatch 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.
Comment #56
swentel CreditAttribution: swentel commentedJust out of curiosity, why are we not using Guzzle instead of curl ?
Comment #57
Wim Leers#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. OnceWebTestBase
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.Comment #58
Wim LeersSince #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.
Comment #59
jessebeach CreditAttribution: jessebeach commented#48: edit-tempstore-1901100-48.patch queued for re-testing.
Comment #61
Wim Leers#48: edit-tempstore-1901100-48.patch queued for re-testing.
Comment #62
Gábor HojtsyThe test fail was a fluke.
Comment #63
jessebeach CreditAttribution: jessebeach commentedRaising to major. This should have been committed with #1678002: Edit should provide a usable entity-level toolbar for saving fields.
Comment #64
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #65
dawehnerJust a couple of points I realized when reviewing the patch.
Access checkers should return static::ALLOW and static::DENY.
This information should be better injected.
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.
I don't get why we even need lazy-injection, can't we just implement ControllerInterface and inject the dependency?
$_POST could be easily replaced by using the information on the $request object.
Comment #66
jessebeach CreditAttribution: jessebeach commentedRemoving tags.
Comment #67
Wim Leers.
Comment #68
jessebeach CreditAttribution: jessebeach commentedAdded a followup to address @dawehner's concerns in #65: #2033433: Clean up edit controller
Comment #69
catchJust 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.
Comment #70
Gábor HojtsyWe 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.
Comment #71
Gábor Hojtsy@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.
Comment #72
klonosRegarding 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?
Comment #73
Gábor HojtsyDrupal 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.
Comment #74
klonosI agree. Perhaps we should go with the simple(ier) solution of content locking then.
Comment #75
catchJust 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.
Comment #76
tim.plunkettWe have KeyValueStoreExpirableInterface, which user.tempstore uses.
Also Views has the content locking.
See Drupal\views_ui\Form\BreakLockForm, Drupal\views_ui\ViewUI::isLocked(), etc.
Comment #77
Gábor HojtsyImplementing 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?
Comment #78
Gábor HojtsyTalked with @berdir about this and he provided this great insight:
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).
Comment #79
Wim LeersIndeed, 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 extendedEditLoadingTest
.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?Comment #80
Gábor HojtsyBecause other entities don't have a changed timestamp? See #2074191: [META] Add changed timestamp tracking to content entities.
Comment #81
Wim LeersFair enough, I can see how that could've been premature :)
Comment #82
Gábor HojtsySo 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:
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?
Comment #83
Bojhan CreditAttribution: Bojhan commentedI thought we had some inline edit way to handle errors like that? That it becomes part of the top of the enity bar?
Comment #84
klonostypo in the message text:
Comment #85
Gábor Hojtsy@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.
Comment #86
Gábor HojtsyReuploading #82 with typo "edites" => "edited" fixed. Looking for reviews.
Comment #87
Gábor HojtsyDiscussed this with @berdir:
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.
Comment #88
Gábor HojtsyAlso got actual code review from @berdir:
Not sure if to act on this ATM, will discuss with @Wim Leers.
Comment #89
Gábor HojtsyHere 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!
Comment #91
Gábor HojtsyHere 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!
Comment #92
catchHmm. 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?
Comment #93
Gábor Hojtsy@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.
Comment #95
catch@Gabor
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.
Comment #96
Gábor Hojtsy@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.
Comment #97
Wim Leers#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:
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:
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 theEntityFormDisplay
, 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 — becauseEntityFormController
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 couldunset()
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 ofEditFieldForm
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:
field_attach_*()
for now.hook_field_attach_form()
(which is called for each widget that gets generated) wouldn't be called anymore.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.
Comment #98
Gábor Hojtsy@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.
Comment #99
Gábor HojtsyQuick reroll of the patch. Looking to fix fails.
Comment #101
Gábor HojtsySlightly 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).
Comment #102
webchickcatch, 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.
Comment #103
tim.plunkettYes 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.
Comment #104
tim.plunkettCould be a onliner.
This should always return
\Symfony\Component\Validator\ConstraintViolationListInterface
, so no isset() needed.Is [0] reliable?
reset($changed_field_errors)->getMessage();
might be betterFinds
In typehints, should be
string|null
This is redundant, but fine if you want to be explicit
public function
Comment #105
Wim LeersI 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.
Interesting — I didn't know that, and judging from the typehints in core, many didn't know this. Is this rule documented somewhere?
Tests Edit on a node that was concurrently edited through the full node form.
80 col wrapping.
Comment #106
Gábor HojtsyUpdated 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.
Comment #107
Wim LeersI'm tempted to RTBC #106, but it's probably better if for example tim.plunkett could RTBC it.
Comment #108
Gábor HojtsyHelps to name the patch a patch :)
Comment #109
tim.plunkettThis 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!
Comment #110
tim.plunkettTo address #104/#105 null vs NULL, opened #2105065: Use correct capitalization of NULL for @param/@return/@var
Comment #111
webchickMost likely it'd be best for catch to sign off on this.
Comment #112
catch#108: edit-concurrency-fieldapi-106.patch queued for re-testing.
Comment #114
Gábor HojtsyRerolled. Marking back to RTBC as per above. Does not hurt to wait for it to be green though :)
Comment #115
catchCommitted/pushed to 8.x, thanks!
Comment #116
Wim Leers#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 :)