Problem/Motivation
Currently, nodes are the only entity that support revisions. While there is support for loading revisions in the generic entity storage controller, there is no support for saving/deleting revisionable entities nor a way to delete revisions
Proposed resolution
The provided patch refactors and generalizes the node specific revision code, extends the entity storage controller and entity interfaces with the necessary methods and adds a default implementation for it.
Remaining tasks
Patch needs review.
User interface changes
None.
API changes
- EntityStorageControllerInterface::deleteRevision($revision_id)
- EntityInterface::isNewRevision()
- EntityInterface::setNewRevision($value = TRUE)
- $entity->revision, which is currently used as the flag for writing a new revions is removed and can no longer be used, isNewRevision() should be used instead.
Comment | File | Size | Author |
---|---|---|---|
#99 | entity-revision-save-delete-1723892-99.patch | 393 bytes | mitchell |
#84 | entity-revision-save-delete-1723892-83.patch | 38.28 KB | Pancho |
#84 | interdiff.txt | 22.73 KB | Pancho |
#82 | entity-revision-save-delete-1723892-82.patch | 34.28 KB | Pancho |
#82 | interdiff.txt | 22.73 KB | Pancho |
Comments
Comment #1
BerdirAdding tags
Comment #2
BerdirHere is a first try, let's see what the tests have to say.
We're not really saving that much code (other than being able to remove a lot of duplicated code in the node storage controller), but this will for example help #1374030: Remove entity_extract_ids() now that we have proper classed entity objects as I can completely remove the controller there once this is in.
Comment #3
BerdirComment #5
BerdirForgot to remove the additional argument in the node storage controller.
Comment #7
BerdirFixed the node tests fails and removed TestEntityController.
Comment #9
BerdirFixed test_entity test failures.
Comment #11
BerdirOk. Added support for saving revisions with predefined keys, this also allows for more simplifications in EntityFieldQueryTest.
Let's see if this passes...
Comment #13
BerdirOh wow, the last patch was nonsense. It was too late to write code.
Here's a better approach that adds a test entity specific flag and re-adds the controller to revert the ftvid if that is set. I have not found a way to figure that out without any flags, the previous patch broke re-saving/reverting old revisions. Not sure if that should be standardized.
I think we can remove the _old thing in favor of $entity->original->getRevisionId() but I left it there for now.
Interdiff is against #7.
Comment #15
klonos...coming from #1082292: Clean up/refactor revisions
Comment #16
BerdirOk, revamped the convertToPartialEntities(), with all those additional properties that need to be set now it's actually easier to use the from_ids() functions as well.
Comment #17
fagoHaving a clone here looks awkward. If the object is just for preparing the storage of the revision, it should be named like that and probably be a plain array of values ora stdclass object. E.g. $record or $storage_record?
I don't think we still need this old_revisionkey. For that $entity->original should work, shouldn't it? Or doesn't that contain the right revision? If so, I'd say that's what we want to fix: $original should be the previous version of whatever we are saving.
Why is that enforcing something? Isn't it just setting the new revision flag? So maybe, it could be
just $entity->setNewRevision(); ?
I do think this should be part of the form code. If the revision checkbox is set, use the method. But I don't think we should still have $node->revision as the API is the method.
Comment #18
BerdirHaven't touched the clone yet, not sure about what to do. We can do that, but then we need to pass $entity to prepareRevision() I think.
Renamed enforceNewRevision() to setNewRevision(), removed the $node->revision check from the storage controller, doing it outside now.
Comment #20
BerdirFixed the weird empty( thing.
Comment #22
BerdirOk, the original change breaks the predefined test_entity revision id's, changed that pattern a bit. We do want to get rid of it in the long term I think but there are various tests that rely on it, especially the EFQ Tests.
Comment #24
BerdirFixed the forum tests, they need a new way to figure out if a new revision was saved.
Comment #26
Berdir#24: entity-revision-save-delete-1723892-24.patch queued for re-testing.
Comment #27
corvus_ch CreditAttribution: corvus_ch commentedSeems to me as an unintentional deletion.
Comment #28
BerdirRe-rolled my branch against 8.x, this should remove the accidental removal of that code.
Comment #31
Berdir#28: entity-revision-save-delete-1723892-28.patch queued for re-testing.
Comment #33
BerdirRe-roll.
Comment #34
corvus_ch CreditAttribution: corvus_ch commentedUse $form_state['values'] instead
/modules/file/file.field.inc file_field_update() still uses
!empty($entity->revision)
instead of$entity->isNewRevision()
.Comment #35
BerdirFixed those two things.
Note that the first point about the clone $entity from #17 is still open. Not sure what to do about it, need to discuss it with fago.
Comment #37
BerdirWeird typo that netbeans did not recognize as a PHP syntax error.
Comment #39
BerdirOk, here we go, fixed the test fails and changed $revision to a $record array.
Note that the change there is fragile and will break the issue that attempts to separate $original from $entity.
We discussed several things on how this could be fixed in the long-term:
- Pass original through that hook as well
- Remove hook_field_update() and similar in favor of hook_entity_update(), which would eliminates quite a few hooks.
- For this specific issue, we could add proper revision support to the file usage API, and actually register the revision id too. Right now, this is a problem anyway, because we have no clue what exactly happens when you have multiple revisions where some have a file and some don't. We have no idea what to do when we start deleting those revisions...
Comment #40
BerdirErm, patch would help.
Comment #41
das-peter CreditAttribution: das-peter commentedNeeds a re-roll, I'm working on that right now.
Comment #42
das-peter CreditAttribution: das-peter commentedOkay, here we go. I re-rolled the patch, let's see if it passes the tests.
I fixed some coding standard violations at the same time.
For now the only part I'm a bit concerned about is this:
At first it looks like the condition could be adjusted to simply use
empty()
, even the comment below the condition implicitly says that. But according to Sascha, who I bothered with this, usingempty()
would lead to failing tests.Shall we improve the comment or just hope that nobody has the idea to clean this up?
Comment #43
fagoPatch does not apply any more :/
Comment #44
fagoAlso, this does not cover deleting yet. Looks like this does not even use the controller yet:
http://api.drupal.org/api/drupal/core!modules!node!node.module/function/...
Comment #45
fagoI've worked on the entity api module's revision support today, patch over at http://drupal.org/node/996696#comment-6433746. This incorporates already #218755: Support revisions in different states as well as code of this patch - maybe it's useful for this patch as well.
Comment #46
BerdirOk, re-rolled and added deleteRevision(). Let's see if this passes.
Comment #48
BerdirHm, 8.x is moving fast ;) New try.
Comment #50
BerdirisCurrentRevision is now protected.
Comment #52
BerdirAdded a dummy implementation for deleteRevision() to the config storage controller.
Comment #54
BerdirRe-rolled after the current/default revision changes. Let's see if this will pass, hope that issue has added proper tests ;) Also fixed the TestEntitycontroller exceptions.
Comment #56
BerdirHad to fix one of these new tests..
Comment #57
moshe weitzman CreditAttribution: moshe weitzman commentedPlease add a Summary. Thanks
Comment #58
das-peter CreditAttribution: das-peter commentedComment #59
BerdirAdded an issue summary.
Comment #60
moshe weitzman CreditAttribution: moshe weitzman commentedIf this is a new hook, it needs docs:
$this->invokeHook('revision_delete', $revision);
// When saving a new revision, set any existing revision ID to NULL
This is not good for data migration between Drupal sites (e.g. D7 => D8). Can we think of a way to honor a revision_id if caller provided one? Nodes and Users currently let you specify a nid/uid so I'm hoping that we can do similar here. This is probably worthy of a follow-up, and shouldn't block this patch.
$entity->use_provided_revision_id
. Looks like this is a new, magic key :). We should document ittypo:
deleeted
Comment #61
BerdirAbout the forceful revision ID override, agreed that this is not optimal but that's how it currently works in node.module. I tried to improve that, but it's quite complicated.
The only thing that I was able to come up with was an explicit flag that is currently only used by the test entity, which requires exactly this: fixed, predefined revision id's because they are then checked in test assertions. The implementation there is just a very ugly hack to get the tests working. We could easily standardize this with a method, but we already have quite a few of those methods and I'm not too fond of adding even more of them.
Ideas?
Comment #62
moshe weitzman CreditAttribution: moshe weitzman commentedI just looked for the equivalent logic when saving nodes/users with specific IDs and it might have been removed in D8 (including the tests for it) which really sucks. Anyway, what D7 does is honor a specified id when an id is passed AND an is_new flag is set. I don't know if it makes sense to do that here. If it doesn't, your approach looks fine to me.
Comment #63
BerdirConfirmed, this is the code snippet from 7.x:
And it looks like the check exists in 8.x as well:
But here I somehow messed up up :) Comment is outdated as well. Probably needs a test for this. Will try to update this soon. Back from a 3 week absence, lots of things to catch up...
Comment #64
BerdirOk, here is the promised re-roll.
- Re-added the new entity check. The reason I removed it is that isNew() doesn't work in there, just like postSave(). Added the same workaround, a $update argument to saveRevision().
- Added documentation for hook_entity_revision_delete().
- The is new check actually made most of the usage of use_provided_revision_id unecessary, the only exception was the EntityFieldQuery tests that try to create new revisions with specific ids of an existing entity. I refactored that to use dynamic id's instead. This allowed me to remove that flag again and also the whole test entity controller class. Win!
- I'm unable to find the deleeted typo?
This means that we actually save code in the end, yay!:
22 files changed, 243 insertions(+), 284 deletions(-)
Let's see if this passed the tests.
Comment #65
moshe weitzman CreditAttribution: moshe weitzman commentedLooks like all my concerns were addressed, and bot is happy.
Comment #66
Berdir#64: entity-revision-save-delete-1723892-64.patch queued for re-testing.
Comment #68
BerdirRe-rolled after the entity property api patch went in.
Comment #69
moshe weitzman CreditAttribution: moshe weitzman commentedthx
Comment #70
Berdir#68: entity-revision-save-delete-1723892-68.patch queued for re-testing.
Comment #72
BerdirRe-roll.
Wondering if this isn't actually a task instead of a feature. Would be really nice to have this in so that #1498674: Refactor node properties to multilingual can build on this, instead of having to do the work twice.
Comment #73
BerdirErm. with patch.
Comment #74
xjmI agree that this is more of a task, since we're generalizing existing functionality from one entity type to all entities. The diffstat illustrates this. :)
Comment #75
xjmComment #76
moshe weitzman CreditAttribution: moshe weitzman commentedBack to RTBC. This sat at RTBC for a week. Lets please get it in soon.
Comment #77
plachComment #78
catchI'm leaving this RTBC but it'd be good to understand the following (apart from the initial nitpick) a bit better since it's not explained well in the patch.
s/entity/node/ ?
I don't understand this comment tbh. Why won't it have the same properties as the entity? If it's got a different structure to the Entity then what about an EntityRevisionInterface or similar.
So we have the revision being passed as an array, and the entity being passed as an entity. Why would those be different at all? Also I have no idea what the structure of $record might be reading the documentation here.
The diffstat on this file is amazing.
Comment #79
BerdirThanks for reviewing.
I've discussed the record thing in Munich with @fago. The reason for using an array there is that a revision usually has additional stuff on it that the entity doesn't. An interface doesn't help because this isn't about methods. We just need a data structure to be able to save it.
It is *similar*, although not equal the way the new DatabaseStorageControllerNG works. The entity object is converted to a dumb record in mapToStorageRecord(). The difference there is it is a class and it has only the properties defined that actually exist on the table. What about doing the same here, then we can document it as "has the properties of the revision table" ?
Not sure if we want to do that now or as a follow-up/while converting to the properties based structure.
Comment #80
PanchoDid I miss the point, why the abstracted entity_revision_delete() - which was there in patch #56 - disappeared again, so node_revision_delete() and other entity types still need to do a "manual" db_delete()?
Otherwise a nice enabler!
Comment #81
BerdirLooks like I re-rolled this on an old patch.
Comment #82
PanchoHmm, seems we need to take a few steps back.
Here's a straight re-roll of #56 against HEAD, with just a few obvious typos and glitches fixed.
Now we'd need to catch up with the changes since #56. Wasn't completely sure about what was on purpose and what was erroneous. Probably Berdir knows best.
The supplied interdiff is against #73.
Comment #84
PanchoHmm, #82 applied cleanly here. Another try with some more minor doc fixes (+ stats)...
If the testbot can't apply, I don't know.
The supplied interdiff is still against #73.
Comment #85
fubhy CreditAttribution: fubhy commentedDeletes an 'entity revision'
Let's change this to 'delete_revision'.
See comment above.
"Indicates whether this is a new revision."
That comment refers to what we are using it for, not what it actually does. What it does is, it returns whether an instance is a new revision of an entity.
Same here basically.
Comment #86
fubhy CreditAttribution: fubhy commentedComment #87
Berdir> Let's change this to 'delete_revision'.
Disagree. the function is called entity_revision_delete() (which is consistent with entity_revision_load()), so the hook should be hook_entity_revision_delete(). It's the field attach function that's named wrong, not the other way round :)
> That comment refers to what we are using it for, not what it actually does. What it does is, it returns whether an instance is a new revision of an entity.
Hm. I get your point, but not sure. This flag is only relevant between being set and save, and setting it usually happens immediately before saving it. What is "a new revision of an entity"? At this moment, it's just a (possibly) modified entity. There will only be a new revision with a new revision id after it has been saved, at which point this function will actually return FALSE. Maybe "Returns if the entity will be saved as a new revision"?
Comment #88
moshe weitzman CreditAttribution: moshe weitzman commentedStill Needs Work?
Comment #89
fubhy CreditAttribution: fubhy commentedAh whatever. I guess I was just nit picking. I can live with it the way it is :)
Comment #90
mitchell CreditAttribution: mitchell commentedMarked #120967: Separate revisions from node.module as a duplicate.
Comment #91
webchickJust to clear up any misconceptions about #76, this sat for a week at RTBC because we were over thresholds during that time, and this was classified as a feature request. And we're currently over thresholds again. I agree with this issue's re-classification as a task, however, because as xjm mentions in #74, this is more refactoring than a new feature. And what a lovely refactoring indeed! :D
Committed and pushed to 8.x, along with a CHANGELOG.txt update. WOOHOO. This'll need a change notice.
Comment #92
jhodgdonde-tagging
Comment #93
chx CreditAttribution: chx commentedThis bitten me during EFQ v2 although figured out fairly quick:
Comment #94
webchickWe're once again over the critical task threshold. Please try and find time to write up documentation for your API change ASAP so your fellow contributors' features aren't blocked. Thanks.
Comment #95
moshe weitzman CreditAttribution: moshe weitzman commentedYes, please write the Change Notice
But it is over the top to use Critical priority for this. IMO, the Needs change notification tag is sufficient. We will remember to close those out before release.
Comment #96
BerdirSorry for taking so long, here is the change record, please review: http://drupal.org/node/1818376.
Render controller is next, will try to find time to write that one today.
Comment #97
webchickLooks good to me; thanks Berdir!
Comment #98
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #99
mitchell CreditAttribution: mitchell commentedThis is a small fix to
entity_revision_delete()
's comment. (I thought better here than a new issue.)Comment #101
fubhy CreditAttribution: fubhy commented#99: entity-revision-save-delete-1723892-99.patch queued for re-testing.
Comment #102
BerdirUpsie.
Comment #103
webchickCommitted and pushed to 8.x, but let's get follow-up issues for minor things like this, ok? :)
Comment #104.0
(not verified) CreditAttribution: commentedUpdated issue summary.