I like the predictable UUID option. That seems like it could have the lowest potential for negative performance impact, as it theoretically doesn't even require a save during the get process (assuming other info has already been retrieved and cached properly).
Just thinking out loud... Would there ever be a need for a UUID to be different across translations or would it always be the same? I mean, I suppose a translation could delete an entire section and create a new section, so that would probably cover that case, and otherwise we'd probably want the UUIDs to match. I'm unsure what's done elsewhere in core.
Well that would definitely beg the question of whether or not that's a good design pattern here, which I'm still not 100% convinced of. But, if it was something we decided to do, one possible solution would be to make a separate issue to create a UuidGeneratorTrait that can handle adding the getter/setter methods for any class, allowing the uuid service to get mocked in unit tests. I could see that being a useful trait, not just for layout builder, but anywhere else UUIDs get generated in core/contrib/custom modules, but that could definitely lead to a lot of refactoring across core, so that probably would have to happen over multiple steps.
**edit**:
To clarify, you don't have to use a separate trait here. You could just add getUuidGenerator() and setUuidGenerator() methods directly to the class and then have getUuidGenerator() do a conditional check to see if the service has been previously set, and if not then set it to \Drupal::service('uuid'). I just figure if we're going to establish that as a pattern for UUID generation, having a generic Trait outside of Layout Builder might be an option here.
However, this might be extremely difficult to update for existing overrides (stored in field data) because we'd have to check every entity with LB overrides enabled, every revision of that entity, for every language that entity has been translated to.
Yeah, especially for systems that have an extremely large volume of nodes/revisions. That update hook could take forever to run.
This is why I was kinda suggesting the alternative of allowing Section::getUuid() to generate and set it, if one doesn't already exist, because at least in that case, it's generating it on demand, instead of all at once. But that said, that could probably come with its own problems. Just a couple concerns off the top of my head:
- Are there performance implications if this is happening in a lot of different nodes all at once on busy systems?
- Would we set it for all revisions/languages when called, or just for the current revision? On the one hand, setting for just the current revision would be more performant, but on the other hand, if you roll a node back to and older revision, you'd need to make sure there's logic to copy that value to the revision.
Alternatively, we could just opt to return NULL in these scenarios. And any consumer of this method would need to know that is a possible return value. Then the next time the node is saved, it can generate a UUID if it's missing, and attribute it to the new revision only. There's still a question of what to do if a revision gets reverted, but in theory, you should be able to look at the current revision, grab the uuid value, revert the to the older revision and then update the uuid for that revision when it gets reactivated.
I'm unsure if having a NULL value return would be problematic in other use cases.
WidgetsBurritos → created an issue. See original summary → .