Add a method to access the original property

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.