Wrong revision can be loaded into original in \Drupal\Core\Entity\EntityStorageBase::doPreSave

Thanks for the cross-reference @Berdir. I looked at it, and it's great to get an official original property. But I'm not seeing where that is covering the issue stated here, where $this->loadUnchanged($id) loads the latests default revision, but not necessarily the current revision that's being pre-saved.

Example: revision 1 is published, revision 2 is draft and gets edited and then saved. In pre-save the $this->loadUnchanged($id) loads revision 1, but original should be revision 2.

I may have missed that in you other issue, is that really covered?

Ah, that's what I missed:

// Use the loaded revision instead of default if this is not the default revision.
if (!$entity->getOriginal() && !$entity->isDefaultRevision()) {
  $entity->setOriginal($this->loadRevision($entity->getLoadedRevisionId()));
}

I wonder if that if-statement shouldn't also verify $entity instanceof RevisionableInterface and if so, that part of the fix would be similar to what I had proposed here. So the fix for loading the correct revision would be this:

// Use the loaded revision instead of default if this is not the default revision.
if ($id_exists && !isset($entity->original) && $entity instanceof RevisionableInterface && !$entity->isDefaultRevision()) {
  $entity->original = $this->loadRevision($entity->getLoadedRevisionId());
}

And #2839195: Add a method to access the original property would be about the property original.

Let me see if that gets tested properly.

@bburch Sorry, I can't reproduce that. The patch from https://git.drupalcode.org/project/drupal/-/merge_requests/3599.diff does apply here for Drupal 10.2.2 - any chance you have another patch in your configuration which gets applied first and therefore may prevent this one from applying as well?

@Anchal_gupta what deprecation did you try to fix with your commit to this MR? I'm asking because that change breaks core entirely as it turns \Drupal\Core\Config\Entity\ConfigEntityStorage into an abstract class because some methods are missing from the RevisionableStorageInterface. Not sure what your intention was?

@makbay as I mentioned before, the approach in this MR will never land in core since @Berdir stated that this issue is being addressed in 2 other issues already. So, as you already contributed to one of the others, it's probably not ideal to modify this MR here as well. So, I'll revert the latest change, close the MR and also this issue as won't fix.

@lukus without further detail it's hard to tell how the patch from the MR could possibly prevent a re-install. But then, you shouldn't use an MR as a patch source either, as this is always subject to changes that may be causing trouble.

In fact, it's not "Won't fix", according to #11 it's a duplicate.

@lukus whether you require this or any patches from one or more of the related issues depends on your use case. For us, we're currently living with this simple "workaround" to correctly recognize state changes when working with ECA and the workflows from Drupal core.