Define 'original' as property on the entity object

Re-roll for 10.1.x.

Also including a patchfile for 9.5.x for those of us who haven't yet been able to move to 10.x and are using this on projects.

Fixing that empty diff for 9.5.x 🤦‍♂️

Only moving back to NW for issue summary update per #97

Catching up on the issue. I'm not sure about removing the optimizations in #88. We are rarely doing that optimization, but we've only lost it partially (when you have a non-default revision), I think the comment example is a case where this makes sense, we save multiple entities, we know what we change and skipping that uncached load comes with a considerable performance boost.

Main argument against it would be that there are some rather tricky edge cases with static caching, someone could have already loaded and changed that entity in the same request, but that's already an issue as you might be saving unintended changes.

Some of the other set calls are also workarounds for the relate issues with non-default revisions or in cases where original actually doesn't get set, will need to have a closer look which ones we can remove and which not.

I'm still not 100% sure, but I think getOriginal() makes sense, I now renamed it and changed the implementation to use loadRevision() if are saving a non-default revision in ContentEntityStorageBase. I then also removed the workaround from 🐛 Impossible to update an entity revision if the field value you are updating matches the default revision. Fixed . Doing it first in preSave() means we don't have to load unchanged *and* then the revision. Lets see if anything fails from this.

The last submitted patch, 104: 2839195-104.patch, failed testing. View results →

Ok, so no existing test expects anything different, the clone test is because I added a type to the property. Fixing that and setting the return type to static then. that won't work on ViewUi, but maybe we should set methods that you're not supposed to use on ViewUi to throw an exception instead anyway?

These patches are proudly sponsored by my delayed flight :)

The CR needs to be updated? Doesn't seem to mention the new functions

Also can the deprecation be updated for 10.2.

Didn't have time to do a full search for replacements yet.

+++ b/core/lib/Drupal/Core/Entity/EntityBase.php
@@ -47,6 +47,21 @@ abstract class EntityBase implements EntityInterface {
+   * @deprecated in drupal:9.5.0 and is removed from drupal:11.0.0. Use
+   *   \Drupal\Core\Entity\EntityInterface::getOriginal()
+   *   instead.
+   *
+   * @see https://www.drupal.org/node/3295826
+   */
+  public ?EntityInterface $original = NULL;

it needs deprecation test and modification of __get/__set() to throw deprecation

It's a defined property, we can't do a deprecation for that as it is now. So it wouldn't get removed, it would become protected and not meant for direct access. I did consider to introduce a new property with a separate name and make that protected from the start, then we could have __get()/__set() magic for it. That would even allow for BC for the small behaviour change for non-default revision saving, but it would also come at a performance cost possibly.

I started reworking the change record as well.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Reroll against 11.x, so restoring "Needs review"

acbramley → changed the visibility of the branch 2839195-define-original-as to hidden.

acbramley → changed the visibility of the branch 2839195-9.5.x to hidden.

Rolling #112 into a new branch on the MR. Will fix up CS issues next.

So the fails are due to me attempting to fix the stan issues here https://git.drupalcode.org/project/drupal/-/merge_requests/5850/diffs?co...

I don't know if we just need to revert that and ignore those stan errors too? It seems we were intentionally using ->original considering it's added by this patch.

Unfortunately #112 is failing against 10.2 :/

Thanks @codesquatch but 112 is out of date anyway. Patches should not be used anymore, https://git.drupalcode.org/project/drupal/-/merge_requests/5850 is now the canonical source for this issue.

Ah, thanks for the response. Oh of curiosity, how would one use this forked version on a production environment? I’m used to using cweagans patches in composer to maintain things of this nature.

@codesquatch you need to download the MR diff as a patch file and then you can use cweagans patches to reference the local file.

i.e, my workflow is something like this:

wget https://git.drupalcode.org/project/drupal/-/merge_requests/5850.diff
mv 5850.diff ./PATCHES/2839195-mr5850.patch

where PATCHES is a directory in my project's root that contains all patches like this.

Then in your composer.json/composer.patches.json:

"Add a method to access the original property": "./PATCHES/2839195-mr5850.patch",

Ahhh, thank you! I see now, that makes sense. Appreciate you dropping the code example too, you da bomb!

OK, a few hours of my life gone into the depths of the meaning of "original" in various contexts around Drupal.

The genesis of this issue appears to be getting away from a dynamic property, but it's morphed into a larger problem space occupied by related issues such as 🐛 Impossible to update an entity revision if the field value you are updating matches the default revision. Fixed . There's also a heavy dose of translations weirdness here.

Overall, the patch is pretty simple in that it replaces gets and sets of $entity->original with new getOriginal/setOriginal methods accessing what will soon be a protected property of the entity. Simple enough to understand that.

Even though that feels like it should go very smoothly with no regressions, test failures cropped up in code that must use the now-deprecated original property to do all sorts of magic like deciding when a translation has changed. I'm not entirely sure what broke this in every instance but I think it's related to the particulars of cloning the "parent" entity into the translations array.

This feels overall like the right cleanup to do, but the collateral damage is gnarly (as mentioned way back in #5. As you can see from the recent commit history, I went back and forth on various hacks and had myself questioning "how does any of this even work right now?" but we're down to a few failing tests which is pretty manageable.

One failing test in particular has an ominous link to a recently-closed issue relating to the behavior of the original property with revisions. @Berdir mentioned at #2859042-54: Impossible to update an entity revision if the field value you are updating matches the default revision. → that the method introduced in this issue is the correct path forward in smoothing out these special cases. So let's do that?

This one is pretty gnarly but I think this is a move in the right direction and I'm happy to see mostly green with a limited amount of hack.

Marking this NR - I haven't chased my tail like this in quite a while but I think this ended up in a decent place with a minimum amount of change to logic. Helps reveal where we were depending on cloned properties rather than being explicit.

Added some questions and suggestions.

Moving to NW for the suggestions made by @longwave.

Rebased and minor fixes.

Back in #31, it was questioned if this should be adding a public property just to deprecate it. I have the same concern.

It would appear the patch fails to work on any version of 10.3.*. I didn't bother testing 10.2 and below.

arousseau → changed the visibility of the branch 2839195-11.x to hidden.

arousseau → changed the visibility of the branch 2839195-11.x to active.

arousseau → changed the visibility of the branch 10.3.x to hidden.

arousseau → changed the visibility of the branch 2839195-10.3.x to hidden.

arousseau → changed the visibility of the branch 2839195-10.3.x to active.

arousseau → changed the visibility of the branch 2839195-10.3.x to hidden.

Re-roll for 10.3.x

I pushed a change to see if my idea about loading the correct origin from the start works, didn't address the other parts of my review yet but plan to have a look later.

Look like only 1 test (LanguageNegotiationInfoTest) failed! Hope to see it in 11.1

Lets temper our expectations a bit. This is a complex change, it's not complete and 11.1 beta is in a few days.

Couldn't reproduce the fail on LanguageNegotiationInfoTest::testInfoAlterations locally, the last few fails here were all in different tests and none of them seem related. So ignoring that for now.

Next step I plan to do is reintroduce the clone/initialize logic, see if that causes any test failures and if not, see if it allows to remove some of the workarounds.

Pushed a proposal for the BC layer by renaming the property and adding/extending __get()/__set() including a test.

Also added to phpstan baseline about the missing return type. Adding it would be a BC break for subclasses.

The remaining question about the BC layer is whether we should ensure the original behavior of what it returned when saving a non-default revision as mentioned in https://git.drupalcode.org/project/drupal/-/merge_requests/5850#note_401766. I'm not sure, it would make things more complex and slower (fwiw, not really slower than HEAD, which already does both loads). But there are 5 or so open issues around this bug and people expect it to be more consistent around revisions and hopefully there aren't too many cases where the opposite is expected (famous last words I guess).

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Squashed to a single commit and then rebased because this had a _lot_ of conflicts after the #Hook changes.

Couple comments on the MR.

Replied.

This RTBC +1 from me. There are outstanding comments from @longwave that @berdir replied to, and I agree those can be pushed to a follow up, but I'll defer as to whether those have been satisfactorily addressed to him.

I've gone through the open threads and closed those that were awnsered. Only one open are all about the removed 'efficiency' optimization for setting original. I agree this could be a followup if we really want it. See 📌 [Follow-up] Investigate if we need to optimize EntityInterface::setOriginal() to only clone when not set Active .

Update the Issue summary to reflect the state of the MR. Also did a small tweak on the title of the change record.

Think this can be RTBC.

Adding needs developer tooling, we could have a rector rule for this. That should not be a blocker, but will help manage the work.

Ignore the part about the follow up, I switched around some thing mentally there.

dubois → changed the visibility of the branch 10.3.x to active.

larowlan → changed the visibility of the branch 10.3.x to hidden.

Committed this to 11.x - nice and early in the 11.2 cycle so we can shake out any issues (should they exist).

Huge effort folks, congratulations.

Published the change record.

larowlan → closed merge request !5850