The current UUID implementation for nodes doesn't support revision UUIDs. This means that when an entity supports revisions, those revisions can't be tracked across environments. There are situations where it is reasonable to expect to be able to update a revision of a node in one environment with the contents from another.
The UUID contrib module supports UUIDs for both the entity (and where supported) revisions. It would be good to see core support this in D8.
Adding the Revision UUID column as a not null required column poses a problem with setting the default values. Previously we've got around this problem by using initial
or initial_from_field
but that is a little trickier with UUID which need to be unique per row. We have two options:
- Use
initial_from_field
as is shown in #48 and then provide a UUID() function for each database layer. - Solve #2346019: Handle initial values when creating a new field storage definition so that we can support many different schema migrations.
Comment | File | Size | Author |
---|---|---|---|
#148 | 1812202-148.patch | 29.05 KB | thursday_bw |
#140 | 1812202-140.patch | 32.94 KB | timmillwood |
Comments
Comment #1
mitchell CreditAttribution: mitchell commented(tagging)
Comment #2
skwashd CreditAttribution: skwashd commentedThis is a first cut. I want some feedback to make sure I am heading in the right direction with this. If people are happy with it I'll add some tests.
Comment #4
skwashd CreditAttribution: skwashd commented#2: drupal-1812202-revision-uuids.patch queued for re-testing.
Comment #6
effulgentsia CreditAttribution: effulgentsia at Acquia commented#2304849: Stop excluding local ID and revision fields from HAL output was committed recently, and in the course of that issue, we lamented not having revision UUIDs.
Bumping this to 8.1 for now, but per https://groups.drupal.org/node/508968, this will likely need to be bumped to 8.2 after March 2.
Comment #7
timmillwoodRequeuing patch from #2 with DrupalCI, just for fun!
Comment #9
dawehnerThank you for the reroll, but of course in the meantime some thing changed.
Another thing which would be nice to use autogenerate the base fields, see
ContentEntityBase::baseFieldDefinitions()
What about just using 'revision_uuid', to not shortcut but rather be really explicit.
We should be able to skip this change completly
I'm wondering whether we could generalize this entire update function to make it available for other ones as well
This function is missing
Comment #11
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedRe-starting the patch from scratch, a lot of things moved around in the meantime :) Leaving at NW because the update function is not there yet.
Comment #12
Anishnirmal CreditAttribution: Anishnirmal as a volunteer and at UniMity Solutions Pvt Limited commentedMoving it to needs review to take the automated test for patch at #11.
Comment #14
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedSome progress.
This question is still outstanding.. the current patch makes it kinda mandatory to have a
revision_uuid
key set ifrevision_id
is set.Comment #16
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis is the problem outlined in #14.
Does anyone have an opinion on this matter? :)
Comment #17
timmillwoodI think revision_uuid should be populated be default if revision key is set.
When will this ever return true?
Comment #18
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedWhen someone sets
$entity->original
and/or$entity->original->revision_uuid
manually. See the code comment just above that line :)Comment #20
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedDiscussed with @timmillwood on IRC and I finally got the problem mentioned in #17. However, I cannot easily think of a way that an external source might pre-set the revision uuid, so I took out that part for now.
Comment #22
timmillwoodComment #23
pwolanin CreditAttribution: pwolanin as a volunteer commentedListening to the core conversation, I feel like the parent UUID + revision hash is possibly more useful than a revision UIID, though I understand that will not uniquely identify a revision since you could have multiple revisions with exactly the same content
Comment #24
markdorisonIs there a compelling reason to not have both a UUID and a revision hash? It seems that they serve two distinct (and valuable) purposes.
Comment #25
dixon_As part of #2721129: Workflow Initiative I would like to propose a change in direction of this issue. Since 2012 (when skwashd posted this issue) the Multiversion has successfully evolved in contrib. And in Multiversion we are borrowing heavily from how revisions are done in other systems, namely Git and CouchDB.
A very important design decision in both Git and CouchDB is that if exactly the same change (time, content etc) is made in two different environments to the same entity it should result in the same revision hash. The point is that this hash should NOT be unique in the case just described. It's better if we can identify exactly the same change in two environments as the same revision hash to avoid having the same change marked as conflicts (once these two changes make it onto the same environment).
Having a revision UUID alongside this proposed revision hash would cause the hash to always be unique (which is not good), unless we exclude it from the hash algorithm. So I would propose we only move forward with implementing this revision hash since that is what #2721129: Workflow Initiative needs.
Structurally I think this patch can stay the same pretty much, we just need to change
revision_uuid
intorevision_hash
and use a different field type thanuuid
.Stub code for the bit that generates the hash:
Comment #26
dixon_Work in progress patch...
Comment #27
markdorison@dixon_: You show in your stub code removing
id
andrevision_id
before hashing. If the goal is for identical changes in two different environments to have the same hash then we would also need to excludechanged
(possibly others?). I only bring this up because I think getting the right value for the revision hash and whether or not we want to add a revision UUID are two different topics. Your code demonstrates that no matter what, we are going to need to exclude some data to get the revision hash to function as desired. With that in mind:Comment #28
simeThere is no way to define for every use case what constitutes a 'unique change" between two environments. Whatever fields are excluded from the hash, it will require making assumptions, and assumptions need to be overridable.
Comment #29
acbramley CreditAttribution: acbramley commentedI agree with @sime here, I also think that hijacking this issue and focusing on the revision hash should be avoided. Or if that is the roadmap that is taken, this issue should be closed and a new one opened since a hash and a uuid are achieving slightly different things.
Comment #30
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI also agree with that and I believe that a revision UUID is still useful for REST purposes, as discussed in #2304849-25: Stop excluding local ID and revision fields from HAL output and the comments below.
So I propose to keep the original purpose of this issue and open a new one for adding the revision hash.
Comment #31
dixon_Yep, no problem. There are ways to work around having both revision UUID and revision hash. So I've moved the hash part into #2727511: WI: Add revision hash base field to all revisionable entities and instead made that issue part of #2721129: Workflow Initiative.
Comment #32
larowlanDefault content et-al would use that feature if we had a normalizer for revisions.
Comment #33
benjy CreditAttribution: benjy at PreviousNext for Department of Justice & Community Safety, Victoria commentedre-roll.
Comment #35
benjy CreditAttribution: benjy at PreviousNext for Department of Justice & Community Safety, Victoria commentedCouple of test fixes, added the update hooks and fixed a bug in SqlContentEntityStorageSchema
Comment #37
benjy CreditAttribution: benjy at PreviousNext for Department of Justice & Community Safety, Victoria commentedOK, i've fixed a couple of tests but we have an issue with setting the default value. The core issue for that is #2346019: Handle initial values when creating a new field storage definition which probably needs to leverage a Migrate based solution although this has promise #2721313: Upgrade path between revisionable / non-revisionable entities
I came up with a workaround using
initial_from_field
which I think works for MySQL and chx tells me we can solve it for the other database engines. That is now a schema change for all UUID fields though so I've left some TODO's to clean-up the reflection I was using and bake it into SqlContentEntityStorageSchema which will allow the entity updates to run.Comment #38
benjy CreditAttribution: benjy at PreviousNext for Department of Justice & Community Safety, Victoria commentedHere's the interdiff
Comment #40
benjy CreditAttribution: benjy at PreviousNext for Department of Justice & Community Safety, Victoria commentedThe change in entity_schema_test.module shows an API break, if other modules are enabling revisions via hook_entity_type_alter() then they'll also need to add revision_uuid otherwise getEntitySchema() will fail trying to get the field storage definition for revision_uuid. We could fix this by enforcing the entity key defaults logic in __set for entity_keys?
A couple more test fixes but still not managed to get rid of the reflection.
I've also removed the schema check in
requiresFieldStorageSchemaChanges
to see what breaks on the bot, I feel like the schema comparison is being handled below with$this->getSchemaFromStorageDefinition($storage_definition) != $this->loadFieldSchemaData($original)
Comment #42
benjy CreditAttribution: benjy at PreviousNext for Department of Justice & Community Safety, Victoria commentedPatch is green, removing the reflection. Still need to try get this green on postgres and sqlite.
Comment #43
dawehnerIMHO we should also check whether 'revision_uuid' was not set yet. Users might override it, for whatever reason
From a reader of this documentation I would be excited if this method would not just tell the information, which is already given by the function name, but rather also some actual additional information, for example an example where a revision UUID could be useful or so.
Note: The return value is string, even getEntityKey returns NULL, if it doesn't exist.
One thing which could be interesting to document is also whether this UUID is unique across translations or not.
I kinda dislike this variable name. What about name it $uuidGenerator, as this is exactly what this is doing? Note: This new property is also not documented yet.
Maybe some documentation could be added why this is needed. Why should keying by revision ID not be unique enough already?
Comment #45
benjy CreditAttribution: benjy at PreviousNext for Department of Justice & Community Safety, Victoria commentedFixed all of dawehner's feedback, I think the only real way to solve the Postgres and SQLite fails is going to be a proper schema migration API from #2346019: Handle initial values when creating a new field storage definition
Comment #46
dawehnerThank you @benjy
Comment #47
tstoecklerThis is a novelty in core, we currently do not provide any default entity keys, not even for
uuid
itself. Also there is no way to opt-out from this as far as I can tell if you want to avoid this for some reason.Therefore, I would propose dropping that. Out entity annotations are already very verbose which independently is a problem in its own right, but one more line shouldn't be a problem I think.
Comment #48
benjy CreditAttribution: benjy at PreviousNext for Department of Justice & Community Safety, Victoria commented@tstoeckler, will take a look at that tomorrow.
Here's a fix for SQLite.
Comment #49
benjy CreditAttribution: benjy at PreviousNext for Department of Justice & Community Safety, Victoria commented@tstoeckler, I looked into your suggestion but the outcome was, it's more effort than just that one line to make it optional and in the end become much messier. I think, if you have revisions enabled then you get revision_uuid as well and that's it. There is a small API break for those who don't call ContentEntityBase::baseFieldDefinitions() that will need documenting in the change record.
Comment #50
benjy CreditAttribution: benjy at PreviousNext for Department of Justice & Community Safety, Victoria commentedAdding tags for review and updating IS.
Comment #51
timmillwoodVery nitty nit picks.
Not sure I see the need of changing this property name.
Sneaking in a typo fix? ;)
uuid generator, or uuid service?
I think service everywhere would reduce the amount of changes in this patch.
Comment #52
larowlanAs mentioned on IRC - this is an API break. We need to make it optional and put all access behind a protected accessor that falls back to the container singleton if not set. Joys of BC
Comment #53
larowlanGah, not that one, this one :)
Comment #55
acbramley CreditAttribution: acbramley commentedThis code block is causing issues when importing normalized entities.
I'm trying to develop normalization for entity_reference_revisions using revision_uuids over at #2780395: ERR fields are not deployable but my revision_uuids were always being overwritten on import which I tracked down to this point.
Comment #56
benjy CreditAttribution: benjy at PreviousNext for Department of Justice & Community Safety, Victoria commentedStraight re-roll against 8.3.x, looking at some of the feedback now.
Comment #57
benjy CreditAttribution: benjy at PreviousNext for Department of Justice & Community Safety, Victoria commentedThis patch tries to detect is the previous revision has the same Revision UUID as the one we're creating, this show allow imports although I haven't tested as suggested in #55
I've renamed uuid_generator back to uuid_service in all existing code to try and reduce the amount of noise and made the UUID Service optional to preserve BC as per #52
Comment #58
benjy CreditAttribution: benjy at PreviousNext for Department of Justice & Community Safety, Victoria commentedInterdiff
Comment #60
benjy CreditAttribution: benjy at PreviousNext for Department of Justice & Community Safety, Victoria commentedComment #61
benjy CreditAttribution: benjy at PreviousNext for Department of Justice & Community Safety, Victoria commentedLooks like the tests are failing for reverting revisions, also this won't work for importing the very first revision.
We probably need a flag to indicate that we're doing an import, or a way to force the use of the existing UUID, thoughts on that? Could even be a new issue given core doesn't have a way currently to import revisions.
Comment #63
dixon_Just had a quick chat with dawehner and timmillwood on IRC regarding UUID vs revision hashes (e.g. #2727511: WI: Add revision hash base field to all revisionable entities) as part of #2721129: Workflow Initiative...
Personally I see use cases for having both UUID and revision hashes. But if we want to keep the number of fields down, one possible solution would be to first commit this UUID issue as-is and then later down the line change how we generate revision UUID so that the UUID is deterministic (like described in #2727511: WI: Add revision hash base field to all revisionable entities).
If I recall correctly version 5 of the UUID spec allows for UUIDs to be deterministic based on namespaces and user data. This could be something we look into in a follow-up.
Comment #64
skwashd CreditAttribution: skwashd at Dave Hall Consulting for Pfizer, Inc. commentedUUIDv5 allows you to generate UUIDs using a URI. While it would be possible for Drupal to generate a UUID from a data URI, there would be quite a bit to the logic.
Firstly Drupal would need to convert the entity object to a "simple" object that contains the properties to be used for generating the data URI. For example the revision id, update timestamp and some other properties would need to be removed. These properties prevent the same change being made on different systems at different times from having the same UUID.
In order to keep the data consistent across platforms the simple object keys would need to be sorted. Some platforms order keys inconsistently. An example of this is the common
dict
object in Python.The "simple" object would need to be serialised, using a cross platform serialisation format. JSON is probably the most portable choice here. A decision would need to be made about base64 encoding and the mime type for the uri. I propose we use
data:application/vnd.drupal+json;base64,[base64-encoded-data]
with base64 encoding being mandatory.A namespace UUID would need to be generated for Drupal to use when generating the UUIDv5 UUIDs.
Any client that wanted to interoperate with Drupal would need to implement this logic if deterministic UUIDs was a requirement for that implementation. Others could use UUIDv4 knowing that revision matching couldn't be used.
This is my long way of saying that I think revision hashes and revision UUIDs should be decoupled, at least for now. Such an approach would allow work on this issue continue. The feature could be implemented as originally proposed with v4 UUID, like it is done today for the entity UUIDs. The value of data uri v5 UUIDs can be compared to continuing to use their v4 counterparts in a later stage of the workflow initiative.
Comment #65
BerdirReroll and moved generating the uuid to setNewRevision(), because the current place had a second problem. It actually only generated the new uuid on saveRevision(), at that point, it had already saved the old uuid to the {node} table (do we actually need the revision_uuid there, I'm not sure..)
Comment #67
dawehnerJust a question: is adding a field and by that changing the schema fast enough to be executed for every entity type, or would adding a sandbox make sense?
Comment #68
benjy CreditAttribution: benjy at PreviousNext for Department of Justice & Community Safety, Victoria commentedI did run that upgrade hook a few times locally from the UI, it has been a few weeks so I can't remember how long it took but it never timed out. I'll give it another test if I can get the patch green.
Comment #69
BerdirOne problem with the update function that I wanted to bring up is that now that we don't have a default value for revision_uuid, it doesn't make too much sense to run it on all entity types, as we can't assume that they have one. It might make more sense to have a helper function or so that each module that adds it to its entity types can call in its own update function. That would also solve the "timeout" problem.
You might not have problems on small sites, but I have no idea what will happen if you try to run that on on a site with 600k or so nodes.
Comment #70
benjy CreditAttribution: benjy at PreviousNext for Department of Justice & Community Safety, Victoria commentedComment #71
Berdirare we sure that the uuid method of the databases creates UUID's that are compatible with ours?
Also makes me wonder if that could possibly be seen as a API change, as we now rely on a method that we didn't before, for backends not supported by core..
using the service is not an option here?
shoudn't the field be marked as revisionable or something?
Hum, I actually thought we'd removed that already.
I'm not sure if we can do this. We need to be as BC compatible as possible, so I think entity types need to opt-in to this feature, possibly with a @todo for D9. Also we might end up adding this for entity types that aren't even revisionable and that's wrong too.
for other revision metadata, we added separate interfaces, we might need to do that here as well. will be annoying, but this is a fairly generic interface that might be implemented by something that isn't a content entity.
We actually did not inject this service so far on purpose. \Drupal\Core\Entity\EntityStorageBase::create() generates a uuid if the service is inject, content entities currently do that through the default value.
might not be needed anymore now, now that I moved it to the entity class.
Comment #72
acbramley CreditAttribution: acbramley commentedThe latest patch seems to fix my issues described in #55, nice work!
Comment #73
timmillwoodThis should fix broken tests in #65.
Comment #74
timmillwoodTrying to address items from #71.
1) Not sure
2) Updated
3) Aren't entity keys assumed revisionable? For example revision_id isn't even a revisionable base field, but still, we should make this a revisionable field.
4) Removed
5) Added RevisionUuidInterface
6) Not really sure I understand the issue here, will ping @Berdir.
Comment #76
timmillwoodForgot to add the RevisionUuidInterface.
This also removes the injected service as mentioned in #71.6.
Comment #78
timmillwoodIt seems like #71.4 started causing issues because we assume revisionable entities types have a revision_uuid entity key but the core revisionable entity types (Node and BlockContent) don't. This patch fixes that, but we need to be backwards compatible for those that don't have a revision_uuid entity key (and tests for that).
Comment #80
timmillwoodThis seems to have blown up many entity types. Think we need more checks in place, and to allow any entity type (revisionable or not) to not need a revision_uuid.
Comment #81
timmillwoodRemoving the special case of revision_uuid from SqlContentEntityStorage and SqlContentEntityStorageTest.
Comment #82
timmillwoodAfter looking at the changes in #81 I'm thinking that maybe we should make revision_uuid a revision_metadata_key, which will mean committing #2248983: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table first.
Comment #84
timmillwoodMaking revision_uuid a revision metadata field.
Comment #86
benjy CreditAttribution: benjy at PreviousNext for Department of Justice & Community Safety, Victoria commentedFixed the two remaining files which were changes in the test made earlier in this issue that I've reverted.
Comment #87
timmillwood@amateescu, @dixon_, and I discussed which table the revision_uuid field should be in. We came to the conclusion it should be the revision table (eg node_revision) and not the revision data table (eg node_field_revision).
This is what happens when revision_uuid is set as revision metadata, but it shouldn't really be revision metadata. So we need to rework the storage schema to make sure it is in the right place.
Comment #88
timmillwoodHere's an update which makes revision_uuid a "key" field but not a "metadata" field.
Comment #90
timmillwoodThink we need to change the key on the revision_uuid column, or special case it so it doesn't appear in data tables.
Comment #91
timmillwoodLooking at the option of non-unique UUIDs. Using the revisionable flag to manage this. So revisionable UUIDs are not unique, and non-revisionable ones are unique.
Comment #93
timmillwoodWell... that was harder to track down that I thought it would be.
In
\Drupal\Core\Entity\Sql\SqlContentEntityStorage::getTableMapping
revision_uuid was getting added twice. Once as a revision key field, and once as a revisionable field. Wrapping the arrage_merge() for these in array_unique() seemed to do the trick.Comment #94
acbramley CreditAttribution: acbramley at Department of Justice & Community Safety, Victoria commentedI'm trying to get this integrated with the Paragraphs and using Entity Pilot to export and import a node's full history.
Other patches in play are:
#2826237: Add support for revision_uuids
#2780395: ERR fields are not deployable
#2774463: Allow transportation of revisions
We were having issues with importing existing content, where it was trying to insert an existing revision (I tracked this down to SqlContentEntityStorage::saveRevision) which resulted in an Integrity constraint violation due to the revision_uuid existing already.
The latest patch seems to have fixed that for nodes but I'm still getting the same error with the Paragraph entities.
The interesting thing is the same (or similar?) issue seems to be causing my paragraphs patch to fail some tests (https://www.drupal.org/pift-ci-job/527174) so I'm not sure if this is due to the core patch or something I'm missing in the paragraphs patch. Any ideas?
Comment #95
acbramley CreditAttribution: acbramley at Department of Justice & Community Safety, Victoria commentedIt seems that I missed the change to set the revision_uuid field as revisionable.
The bi-product of that now seems to be that existing revisions are duplicated on import instead of updating the existing revision.
In other words, there is no integrity constraint on the revision_uuid any more. I can continue importing existing content and generating duplicate revisions with the same revision_uuid.
EDIT: SQL Output:
I have 2 nodes with several revisions each.
I then import those same nodes again expecting to see no change. But:
Comment #96
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis is not true anymore :)
-> of the entity revision.
This change is out of scope :)
Why do we need to add revision_uuid to $revision_key_fields?
Same question here.
We shouldn't remove this check.
Why is this change needed?
Also, using a db-level function as 'initial_from_field' is kind of a hack IMO :)
I don't see anything in the comment storage that uses the new service.
- we need to hardcode the definition of the revision_uuid field instead of getting it from the field manager.
- we need individual update functions for each entity type
- the same individual update functions need to set the value of the revision_uuid field in the database instead of relying on the 'initial_from_field' stuff
Comment #97
timmillwood#96.4 & #96.5 - So the column is added to the revision table (as well as the revision data tables).
#96.7 - So that revision_uuid isn't set as unique. A unique revision_uuid in the data tables causes issues for multilingual sites.
Comment #98
timmillwoodAddressing items from #96 apart form 7 & 9.
Comment #100
timmillwood- Moved the update hook to node.install and block_content.install.
- Prevented the uuid field update by only adding the initial_from_field on revisionable uuid fields.
Can't think of a good solution to removing the use of initial_from_field.
Comment #102
timmillwoodFixing REST test failures.
Comment #103
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedIs there anything wrong with generating the uuids in PHP and populating the reivision_uuid table column in update functions?
Comment #104
timmillwoodAfter speaking to @amateescu we agreed that UUID() in SQL is not the best option for anything, let alone initial_from_field. The biggest realization for me was that we need the same revision_uuid for each language of a revision.
The solution we are looking at now is:
Comment #105
BerdirDoing that in hook_update_N() would mean hours of downtime for a site with 100k's of nodes. That's a serious problem.
We once discussed in IRC the idea of doing this as some kind of background job. file_entity has a similar thing where it converts files to have a bundle. That doesn't scale well either though as it saves each ID into a queue.
Comment #106
acbramley CreditAttribution: acbramley at Department of Justice & Community Safety, Victoria commentedCan I get a response to #95, the idea of having non unique revision_uuids is going to completely break the use case for us.
Comment #107
timmillwoodJust a progress update
- Revision uuid is unique again
- Revision uuid is only in the revision table and not the data table
- The initial revision uuid is the revision id
Todo
- Update the revision uuid field with a uuid
- Run the update on cron
- Add a status page to show update progress and manually run it
- Write tests
Comment #109
timmillwoodBefore I sign off for the week
- Implemented EntityFieldValueUpdater class to be a general place we can put updaters in the future.
- updateRevisionUuid class loops through all [revision_id => entity_id] items in the array and updates the revision_uuid
- if there are less than 100 to update this is called from hook_update, otherwise it's stored in state
- hook_cron gets the array from state and loops through 100 at a time.
Todo:
- Add a status page to show update progress and manually run it
- Write tests
Comment #111
timmillwood- Added a hacky fix for the views issue
- Setup upgrade tests for node and block_content
- Introduced a status report item to show if there are "field value updates"
Todo:
- Add a route / controller to run updates manually
- Add a test with more than 100 node / block_content entities
#2721313: Upgrade path between revisionable / non-revisionable entities adds a database dump with more entities, which would be good to test this.
Comment #113
timmillwoodRe-rolling due to conflicts with #2789315: Create EntityPublishedInterface and use for Node and Comment.
Comment #114
timmillwoodComment #115
timmillwoodTodo:
- Add a route / controller to run updates manually
- Add a test with more than 100 node / block_content entities
Comment #116
BerdirThis also won't scale to 600k entries. We can query revisions without a uuid, all we need is a list of entity_types to process IMHO. Then we fetch N revisions without a UUID and if we're done (less than 100 found), we remove that key.
Also doesn't need to be entity type specific, could be in system_cron().
Comment #117
hchonovSo if I've disabled cron my entities would never be updated or if I do the update and want to make use of the feature it would be not possible right away but I might need to wait days depending on how many revisions I have and how often my cron is running? Why not doing this instead in a batch job during the update?
Comment #118
timmillwood@hchonov - @berdir suggested that running all this in an update hook, even as a batch job will be slow for hundreds of thousands or millions of entities, therefore taking a live site down for a while. I'm planning on adding a route linked from the status report to run the update on all entity.
Comment #119
BerdirBecause a site is in maintenance mode while updates are running and *down*.
We can always add a drush command or even a UI to force-execute it.
Comment #120
timmillwoodReworked based on #116.
Still need to build a manual way of running the update.
(Don't think it works, but need to head out, so kinda a work in progress)
Comment #122
timmillwoodFixed the tests, and added a manual way to run the updates.
Not fully happy with the implementation, also need to switch the manual updater to run as a batch.
Could do with a test of more than 100 entities.
Comment #124
timmillwoodMissed some file from the patch.
Comment #125
timmillwoodBasic re-roll.
Comment #126
timmillwoodBeen looking into the best way to batch process the field value updates. I guess in the controller, but then there are dependency injection issue. Are there any good examples of how to batch process things like this?
Comment #128
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedIf you need to run a method in a batch process, it's usually easier if you make it static and access services through
\Drupal::
and friends :)Comment #129
larowlan<plug class="shameless">
I wrote a bit about that https://www.previousnext.com.au/blog/refactoring-drupal-batch-api-callba...</plug>
Comment #130
BerdirThat's what I did in simplenews:
Could be a static method too. But of course, we could just simply add support for the controller resolver in batch API, should be easy enough :)
Comment #132
timmillwoodre-roll of #125.
Comment #134
dagmarRerrolled.
Comment #136
lammensj CreditAttribution: lammensj at iO commentedI could not apply the latest patch due to a faulty path to FilterUidRevisionTest.
Comment #138
timmillwoodRerolling
Comment #140
timmillwoodHelps if I properly resolve conflicts.
Comment #142
dawehnerJust a early in the morning review ...
Here is a question:
Both times this call happens a new UUID is generated.
It would be great to not document what is happening, but rather why.
Here its using $duplicate->{}, the other method above is using $this->set() ... maybe make those two lines more consistent.
Let's be safe and use a triple equal
So the revision uuid key can be NULL|bool|string ;) Just imagine if PHP would actually have generics, what a nice world this could be.
The documenation seems to document something which is not really visible in the code (anymore)?
Why can we not mock the entity type manager?
Comment #143
hchonovIn order to cover what Daniel has mentioned in #142.1 and don't regenerate the value each time setNewRevision is called and additionally don't loose it on
1. setNewRevision(TRUE)
2. setNewRevision(FALSE)
we have to move this piece of code to the storage and create the revision UUID before the entity is saved. We should not make irreversible changes in setNewRevision() and this is why we've moved the logic for the revision translation affected flag away to the storage - see next point.
We've recently moved this piece of logic to the storage. Is there a specific reason for bringing it back or something went wrong during re-rolling the patch? :)
What about just setting it to null and letting the storage take care of it on entity save? I think that this way it will be consistent with 1.).
If we put the revision uuid key to the revision metadata keys in the entity annotation it will get automatically excluded from
CEB::hasTranslationChanges()
. This happens inCEB::getFieldsToSkipFromTranslationChangesCheck()
.I think we could consider this more of a revision metadata key instead of an entity key - see the previous point for advantages.
Comment #146
Wim LeersIs this still intended to happen? This would have a huge impact on #2992833: Add a version negotiation to revisionable resource types.
Comment #148
thursday_bw CreditAttribution: thursday_bw at Catalyst IT commentedI've had a go at updating this patch to work with 8.8.x-dev
I have a patch that applies, but it's buggy and definitely needs work. It's a step in the right direction though.