Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
We usually have a UUID field on each entity type using the 'uuid' field type, but aggregator items have a GUID 'string' field instead. We should convert and rename it to be in line with the rest of core.
Comment | File | Size | Author |
---|---|---|---|
#41 | 2149851-41.patch | 5.83 KB | dcam |
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedForgot to say that I'd prefer this to wait for #2112239: Convert base field and property definitions.
Comment #2
BerdirIt's called GUID because that's what it's called in RSS, it doesn't have to be a UUID nor is it generated by us like our UUID's usually are.
If anything, we should add an additional UUID, if you want to synchronize something internally, but that doesn't make much sense to me?
Comment #3
amateescu CreditAttribution: amateescu commentedI see :) Ok, let's close this one.
Comment #4
amateescu CreditAttribution: amateescu commentedErr.. I meant this.
Comment #5
BerdirWorks for me, updating title accordingly
Comment #6
alexpottHow about actually documenting this here so we're not tempted to change this is the future?
Comment #7
tstoecklerAlso Item::uuid() should override ContentEntityBase::uuid() to always return NULL. On the other hand the latter should really be checking for the UUID key, similar to #2182239: Improve ContentEntityBase::id() for better DX so might just want to leave it as is for now.
Comment #9
timmillwoodComment #10
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedChanged the @todo into a better description of the GUID field.
I would prefer to not do this, just in case someone needs to add a 'real' UUID field to feed items for deployment purposes :)
Comment #14
Wim Leers+1 for #10, because if we use GUIDs as if they are UUIDs, we're very likely to run into two different
Item
entities having the same GUID, which of course defeats the purpose!However, that means that we chose not to add a
uuid
field to theItem
content entity type for no reason at all: we should have done that after all! Right?Attached is a straight reroll of #10.
Comment #15
Wim LeersThis then adds a
uuid
entity key base field. Thanks to\Drupal\Core\Entity\ContentEntityBase::baseFieldDefinitions
, this meansItem
content entities will automatically get a UUID field. However, this of course needs an update path for existingItem
content entities.This is the only content entity type that does not have UUIDs. It prevents e.g. the https://www.drupal.org/project/jsonapi contrib module from being able to work with
Item
content entities.Let's see what fails. (But we'll still need an update path anyway.)
Comment #17
Wim LeersI think the update path test coverage needs to do two things:
Item
entities do not have a UUID before the update, but do so afterwards. Example:system_update_8400()
Item
entity type does not have a UUID key before the update, but does so afterwards. Example:\Drupal\node\Tests\Update\NodeUpdateTest
.Comment #23
JacobSanford@Wim Leers's patch in #15 no longer applied to 8.9.x. A reroll with no further modifications is attached - items mentioned in #17 still need to be addressed.
Comment #28
quietone CreditAttribution: quietone at PreviousNext commentedThe
aggregator
module has been removed from Core in10.0.x-dev
and now lives on as a contrib module. Issues in the Core queue about theaggregator
module, like this one, have been moved to the contrib module queue.Comment #29
larowlanNeeds a reroll for contrib
Comment #30
Arti Anil Pattewar CreditAttribution: Arti Anil Pattewar at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch #23 for contrib Aggregator 1.0 on D9.4.x.
Comment #31
Arti Anil Pattewar CreditAttribution: Arti Anil Pattewar at Srijan | A Material+ Company for Drupal India Association commentedComment #32
dcam CreditAttribution: dcam as a volunteer commentedRerolled #23.
Comment #33
dcam CreditAttribution: dcam as a volunteer commentedComment #34
dcam CreditAttribution: dcam as a volunteer commentedUpdated the test array to try and fix the errors in #32.
Comment #35
dcam CreditAttribution: dcam as a volunteer commentedNW for the update paths. I started work on them, but haven't finished.
Comment #36
larowlanLooks good, just needs update path as you mentioned
Comment #37
dcam CreditAttribution: dcam as a volunteer commentedI added the update path and test.
Comment #38
dcam CreditAttribution: dcam as a volunteer commentedI forgot to update this docblock when I copied it from another test I'd written. But I'm not going to bother changing it right now. We'll probably have to update this class's docblock to change the number of the update function it covers anyway. It can be fixed then.
Comment #39
larowlanGood to go, will need a reroll when one of the other update hook issues goes in, so fix the docblock at the same time
Comment #40
andypostboth places will need to check and fix on commit
Comment #41
dcam CreditAttribution: dcam as a volunteer commentedRerolled #37
Comment #44
dcam CreditAttribution: dcam as a volunteer commentedThank you to everyone who worked on this!