InstanceInterface: setSources() instead of setNewPresent(), one save per change, the hashes

If it is a RC blocker, it must be moved to next beta.

Proposal analysis

setSources(array $sources, string|\Stringable $log_message = ''): static on InstanceInterface, normalizing and skipping an identical tree as setNewPresent() does with its default flags. setNewPresent() is removed from HistoryInterface, and its seven callers move to setSources(). The mutators share a private method that holds the two flags.

No extra save() after a mutator or setSources(), in the controllers and the submodules. Tests: one change, one revision.

So InstanceInterface::setSources() is becoming simpler replacement of HistoryInterface::setNewPresent() without the check_hash and index flags, and performing the entity save?

It is great to simplify the HistoryInterface, removing the last leftover of the old "steps" system.

⚠️ However HistoryInterface::setNewPresent() is already never called using the check_hash and index flags outside of Instance entity internal calls and the related InstanceHistoryTest.

So, before renaming/moving any public method, let's try to simply move ::setNewPresent() logic into a private ::saveTree() method and remove the flags from the public API.

⚠️ Also, it is a bit "disturbing" to make the entity save itself, especially in a "set field value" method. In Drupal, methods that change an entity normally just change it in memory and return $this; the caller decides when to call save(). So, this proposal go against the convention. I know we already do that 2 or 3 times in thge codebase, but is this the opportunity to remove them instead of consolidating them?

We can also remove comment(s) about https://redux.js.org/usage/implementing-undo-history because it is not the case anymore, since we moved to a revisionable content entity.

isPublishedPresent() returns !$this->hasUnpublishedChanges(). Test: an instance whose published sources lack the draft's node IDs is published present.

👌 OK. But do we still need keep both methods in this case ? I would advocate removing one.

getUniqId() and getContentHash() leave the interface: they are static methods on a contract. They stay on the Instance class, internal, for the module's own callers. The storage alone writes the hash: postCreate() and createDisplayBuilderInstance() stop. getHash() and getPublishedHash() say what each hash counts and when to use it. InstancesPanel::hasUnpublishedChanges() gets a @see to the instance method, and a test for a reference whose instance is gone.

👌 OK for removing InstanceInterface::getUniqId() and InstanceInterface::getContentHash(), and leveraging EntityInterface::postCreate() and/or the Entity Storage. It looks more Drupally.

⚠️ However, ::getContentHash() is also called from buildable plugins and InstanceRowStatus, so how can we do that without extending the storage interface? A SourcesHashTrait ? It may also be the opportunity to remove InstanceRowStatus which is a curiosity only used by InstancesPanel.

ensureCurrentTranslation(): static, with an @return, and a docblock saying it saves and may return another object. Its comment names the translatable fields. Which language it should follow is its own issue, under #3615902: Plan: Translation support.

👌No opinion about that.

It would be nice to remove InstanceInterface::ensureCurrentTranslation() all together, it looks a bit odd and I am surprised there is no existing mechanism in Drupal Core doing this kind of job. But I guess it is not possible.

Instance::$currentUser private. InstanceHistoryTest switches users with the test trait's setCurrentUser().

It is already the case as far as I know.

Every mutator docblock says it saves a revision, which on a translation reaches every language when the structure changes.

👌 Just a docblock change? OK.

Work

I will give a try.

Expected breaking changes:

• Change HistoryInterface::setNewPresent() (or replace it by InstanceInterface::setSources())
• Remove InstanceInterface::getUniqId()
• Remove InstanceInterface::getContentHash()
• Remove InstanceInterface:isPublishedPresent() or :hasUnpublishedChanges()?
• Remove Instance::currentUser
• Change DisplayBuildableInterface::ensureCurrentTranslation()

phpunit changes must be limited to:

• those interface changes
• the removal of the tests dedicated to the removed public methods

Playwright tests must not be changed, not at all.

First commit pushed.

Done

However, ::getContentHash() is also called from buildable plugins and InstanceRowStatus, so how can we do that without extending the storage interface? A SourcesHashTrait ? It may also be the opportunity to remove InstanceRowStatus which is a curiosity only used by InstancesPanel.

Done with SourcesHashTrait used in:

• Instance::hasUnpublishedChanges() entity. Is it something to move to InstanceStorage?
• InstanceStorage, as expected
• InstanceRowStatus, finally kept
• DisplayBuildablePluginBase, as expected

Next steps

Also, it is a bit "disturbing" to make the entity save itself, especially in a "set field value" method. In Drupal, methods that change an entity normally just change it in memory and return $this; the caller decides when to call save(). So, this proposal go against the convention. I know we already do that 2 or 3 times in the codebase, but is this the opportunity to remove them instead of consolidating them?

There are 2 decisions to take:

• Do we confirm we go the mutator way? It is a pretty big decision to take, moving away further from Drupal.
• Do we rename HistoryInterface::setNewPresent() with InstanceInterface::setSources(), knowing it will add around +100/-100 changes in an already heavy MR.

Remove InstanceInterface:isPublishedPresent() or :hasUnpublishedChanges()?

I would advise to remove ::hasUnpublishedChanges(), because less usage in the codebase:

$ grep -r "\->hasUnpublishedChanges(" | wc -l
6
$ grep -r "\->isPublishedPresent(" | wc -l
29

New MR: https://git.drupalcode.org/project/display_builder/-/merge_requests/538

I played it safe, picking the last disruptive naming change options (::setNewPresent() instead of ::setSource(), ::isPublishedPresent() instead of ::hasUnpublishedChanges()) while targeting the goals of the ticket (as far as I understood them):

• going the mutator way for the Instance entity operations on the source tree (alter the tree and save the entity from the same public method)
• simplifying the interfaces

Interface changes:

• Simplify HistoryInterface::setNewPresent()
• Remove InstanceInterface::getUniqId()
• Remove InstanceInterface::getContentHash()
• Remove :hasUnpublishedChanges()
• Remove Instance::currentUser
• Change DisplayBuildableInterface::ensureCurrentTranslation()

✅ Tested and works — rebased MR !538 locally on 1.0.x (no conflicts), applied it, rebuilt caches and re-ran the publication check on an instance whose draft and published sources differ only in node IDs: isPublishedPresent() went from FALSE to TRUE and the status reads "Published". The contradiction between the two publication checks is gone.

Code review - No caller outside the instance still calls save() right after setNewPresent().

Thanks István.

Jean, what do you think of the stance I took in #12 ("I played it safe, picking the last disruptive naming change options")?