$entity->original doesn't adequately address intentions when saving a revision

So before loading $original we need to check through an entity query if there is an existing forward revision or we should add this information to the entity when building/loading it from the storage? We would need this for points 1. and 3..

1. No existing forward revision exists, saving a new non-default revision based on the current default. In this case you should compare against the default (which is simultaneously the most recent and the one you loaded)

if ($entity->wasDefaultRevision()) {$original = $storage->loadUnchanged($entity->id());}
So we need wasDefaultRevision as in this case we would have called $entity->isDefaultRevision(FALSE) and do not know anymore of the current entity was a default revision or not.

2. Load and edit an existing forward revision, then save it as a new forward revision with a new workflow state. In this case you should compare against the loaded revision (which is simultaneously the most recent, but not the default).

$original = $storage->loadRevisionUnchanged($entity->getOriginalRevisionId());
This is blocked on #2809227: Store revision id in originalRevisionId property to use after revision id is updated where we introduce ::getOriginalRevisionId and probably on #2620980: Add static and persistent caching to ContentEntityStorageBase::loadRevision() where we introduce a static cache for entity revisions and ::loadRevisionUnchanged.

3. You're reverting a forward revision back to a previous one - i.e. loading revision 1, saving revision 3. In this case the comparison ought to be against revision 2 (which is neither the revision that was loaded nor the default, but it is the most recent)

What if revision 2 is the default revision? This, I guess, is then Point 1?

$original = $storage->loadRevisionUnchanged($storage->getNewestRevisionId());

Perhaps I'm misunderstanding the problem, but it seems to me that in every case when you save a new revision you are comparing it to the most current revision, whether or not that also happens to be the default revision. Under what circumstances would that not be true?

Whenever you want to do someting where only the actually published (=default) revision matters, then when a new default revision is saved you want to compare against the previous default revision to see if you need to do something.

But yes, I suppose in most other cases, you want to compare against the latest revision.

@Birdir: Do you have an example or two of when that might be the case, to help me understand the issue?

For example if you integrate with some external service and send data if the value changed, You probably want to do that only if this is the default revision and the it has changed to the previous default revision. A variant of that could be sending out an e-mail if a node is published.. you don't want to send that if it's just the draft that is published.

We discussed this at the Entity Field API triage meeting with core and Entity API maintainers and we agreed on the current classification as "major".

Whenever you want to do someting where only the actually published (=default) revision matters, then when a new default revision is saved you want to compare against the previous default revision to see if you need to do something.

I believe that everyone who cares exactly about that, should explicitly load that revision. In almost all cases we implicit expect the corresponding loaded revisionn, and a lot of code out their doesn't handle that correctly. Yes there might be some problems introduced by changing that, but I believe there are way more problems out there at the moment.

Let's see how many test failures we might have with this.

@dawehner - some of the test fails might be related to defining original as property - there is an issue for this which has tests fails as well - #2839195: Add a method to access the original property .

I worry about contrib or custom modules which are depending on the incorrect way $entity->original works. Therefore if we make it work correctly are we breaking backwards compatibility (BC), but then how do we get around this?

Do we deprecate 'original' in D8 and add 'correctOriginal', then deprecate 'correctOriginal' in D9 and add 'original' back.

See #2839195: Add a method to access the original property , my suggestion is to add explicit methods for both/all original-y entities that you might want to use.

I agree with #14 that the proper fix would be to add methods to explicitly retrieve the original source and default revisions. I'm going to provide a quick fix in #2859042: Impossible to update an entity revision if the field value you are updating matches the default revision., but feel free to close that as a duplicated of this one, if the BC considerations Tim has outlined in #13 wouldn't allow that.

#2839195: Add a method to access the original property fixes this now I think.