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

Seems the change is causing failures so may need to relook at that.

But this will need a test case also.

As commented in slack, 📌 Define 'original' as property on the entity object Needs work now includes a fix for this. The logic for this shouldn't be in EntityStorageBase but ContentEntityStorageBase as EntityStorageBase won't include revision methods anymore with D11. My implementation also avoids to load the original twice by doing it first.

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?

See #103 there and the changes in ContentEntityStorageBase and SqlContentEntityStorageBase.

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 📌 Define 'original' as property on the entity object Needs work would be about the property original.

Let me see if that gets tested properly.

It's not necessary to check revisionable because all content entities implement that.

This issue is still a duplicate of the two existing ones and I'd prefer to fix them together, that also allows us to document the changed behavior.

I tried to apply a diff based on the current MR to Drupal 10.2.2, and the patch doesn’t apply.

Where can I get a patch that will apply to the current release version of Drupal core?

@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?

Got it working -- our mistake:

In the “extra” “patches” section of the composer.json file, we had the array key as “drupal/eca”.
But we aren’t patching the ECA module, we’re patching core. So, it should be:

"drupal/core": {
"fix ECA workflow transitions": "./patches/eca-3599.patch"
}

@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?

I think it's about this:

Call to deprecated method loadRevision() of class               
         Drupal\Core\Entity\EntityStorageInterface:                      
         in drupal:10.1.0 and is removed from drupal:11.0.0. Use         
         \Drupal\Core\Entity\RevisionableStorageInterface::loadRevision  
         instead.   

I had on tests for a similar issue here: https://www.drupal.org/project/drupal/issues/3201209 🐛 Hook presave does not set original correctly when editing non-default revision entities Needs work

https://git.drupalcode.org/issue/drupal-3201209/-/pipelines/97456

This patch is now causing out build test runner to fail when trying to install our site from scratch.

@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.

jurgenhaas → closed merge request !3599

> @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.

@jurgenhaas - I totally agree. Moving this into a patch file and adding to our codebase for now, while I work out why the patch was included in the first place.

You mention it's being addressed elsewhere, so maybe it's not required?

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.

janpongos → changed the visibility of the branch 3346430-wrong-revision-can to active.

ok so this is a dangerous update - MR 9272
I misinterpreted the method as if it's a query.
Please ignore that MR.

janpongos → closed merge request !9272

janpongos → reopened merge request !9272

janpongos → closed merge request !9272