Problem/Motivation

Child 3 of #3624396: Plan: Harden InstanceInterface, IslandInterface and DisplayBuildableInterface as public API before RC1, split in two: this one is InstanceInterface, child 3b the other interfaces. Needs #3627028: InstanceInterface: one failure style for the tree methods.

  • Setting a whole tree goes through setNewPresent(), a history term with two boolean flags, which returns nothing. Seven callers outside the instance, the submodules among them, use it, and each one calls save() right after, though it already saved.
  • The controllers save again after a mutator, 9 times: each mutator already saves.
  • isPublishedPresent() compares hashes that count node IDs, so it can answer "not published" where hasUnpublishedChanges() finds no change.
  • getUniqId() and getContentHash() are static methods on the interface. The hash is written in three places, and the storage recomputes it on every save anyway.
  • ensureCurrentTranslation() saves and may return another object, and says neither. Its comment names one translatable field, there are three.
  • Instance::$currentUser is public, for one test.
  • Two mutators do not say that they save a revision, nor that on a translation a structural change reaches every language.

Proposed resolution

  • 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.
  • isPublishedPresent() returns !$this->hasUnpublishedChanges(). Test: an instance whose published sources lack the draft's node IDs is published present.
  • 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.
  • 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.
  • Instance::$currentUser private. InstanceHistoryTest switches users with the test trait's setCurrentUser().
  • Every mutator docblock says it saves a revision, which on a translation reaches every language when the structure changes.

Remaining tasks

User interface changes

None.

API changes

  • New InstanceInterface::setSources(). HistoryInterface::setNewPresent() removed.
  • getUniqId() and getContentHash() removed from InstanceInterface.
  • ensureCurrentTranslation() returns static.
  • Instance::$currentUser is private.
  • isPublishedPresent() ignores node IDs.

Data model changes

None.

Comments

mogtofu33 created an issue. See original summary.

mogtofu33’s picture

Issue summary: View changes