Add a method to access the original property

Thanks for opening this.

Ok, those errors are pretty scary, did you already look into what's going on? Do we need some special handling again on cloning/translationing (yes, I just invented a new word, cool no?) We possibly get that implicitly right now as i is in values.

While touching this, I think we should actually add a method/some methods for this, as I think we will need at least two different objects, as being discussed in #2700747: loadUnchanged loads default revision, not latest revision. As already commented somewhere/talked about with @timmillwood, we could then add two useful method names that make sense and add the old property as a deprecated BC thing?

If we add $original as public, we have to deprecate it and add also a protected one + setter/getter.

Re-tested now that the referenced issue is in.

Reroll.

Based on #5, the IS should be clarified.

Don't have those test fails locally on PHP 7, but possibly this helps.

Ha, that was one magic fix for sure ;)

Now that this is passing, we need to figure out how this relates to #2833084: $entity->original doesn't adequately address intentions when saving a revision, possibly letting that figure out the desired API and then implement those methods and deprecate ->original. Or we get this in and then deprecate it when that is ready.

Rerolled the patch.

IMHO, as mentioned above, I would make it public, deprecated and add a method instead to use it. Also one to set the value.

This was never really documented, so I think we can live with some hidden BC changes as the use case you described above. I think it's very unlikely that there a lot of custom/contrib code doing something like that.

Playing with that a bit. I added the method as getOriginalDefaultRevision() because I want to document that it is the default revision. But the problem is that this is on EntityInterface which doesn't have revisions. Suggestions welcome, maybe getOriginalEntity() and add additional documentation on ContentEntityInterface. And we can then add getUnchangedLoadedEntity() in #2833084: $entity->original doesn't adequately address intentions when saving a revision or something like that.

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -630,4 +643,11 @@ public function getConfigTarget() {
+  public function getOriginalDefaultRevision() {
+    return $this->original;
+  }

Since this will not always be set, should we throw a proper exception here with an explanation such as "Will only be set in presave()" or something?

unrelated testbot error

Why not create a clone on loading the entity and using it to set the original directly saving us a second DB load when the original is needed?

While that sounds nice in practice, I'm not sure it's a good idea. Currently you can do isset($this->original) to check whether an entity is being saved, that would then break. So it is kind of a behavior change. Not sure, though, maybe other people have more experience with this.

@berdir - why do we have to define the property public? Instead we could define it protected and use CEB::__set and CEB::__get to access it as currently we are doing, but then trigger there a deprecation notice?

Yes we could. My reason for making it public is that it gives us a nice way to mark it as deprecated in IDE's.. a runtime deprecation notice would work too but isn't as clear when looking at code.

Maybe we can ask @catch, @xjm or some other release or framework maintainer for an opinion?

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -47,6 +47,19 @@
+   * This property will be set and used during the saving process.

Is there anything stopping us from setting it during the load process so we can use it in entity validation for example (#2826101: Add a ReadOnly constraint validation and use it automatically for read-only entity fields)?

Is it just performance/memory considerations?

I don't think we should make $original a public property just for deprecation reasons. At the moment we're using the non field path in CEB::__set() etc. If we want to make it a protected property we'll need to add cases to the magic methods for $name === 'original'. And do the @trigger_error in them. Yes it won't be a nice as an IDE catching it but there's less behaviour change.

I was actually starting this comment with "Works for me.", but then started thinking (that's a dangerous thing, I know :)):

One thing that I only just thought about is that __get()/__set() is a content entity thing, but $entity->original also exists for config entities. So right now, setting $entity->original on config entities actually defines it as a public on-the-fly property (not sure if there's a name for doing something like that).

Meaning, to be able to define it as protected, we would have to *add* completely new __get/__set() to the entity base class. Which could be problematic because if someone has implemented that now on a specific entity class that is not a content entity, then he won't be calling the currently non-existing parent method, so original would possibly not work for those entities anymore.

We also can't make it public on Entity and protected on ContentEntityBase.

Given that, I think that public $original is still the least likely approach to cause BC problems?

That needs work was also part of the initial "works for me" not-actually-posted comment.

@amateescu: That might be a non-trivial behavior change (think loading entities and then serializing them, do we serialize ->original or not? and many other "interesting" questions). We could open a follow-up to discuss that but I'd prefer to keep this change as small as possible.

@berdir: Does this mean, that we cannot implement PHP magic methods on existing classes anymore? This also sounds like that we even cannot introduce new methods, because it might happen that in custom/contrib the same name is already being used in a class extending from a core class.

I think magic methods are a bit different from normal ones, but yes, it can be a problem to add those, depends on the use case I think. But since original would break without special support in _get() this is a bit more problematic than e.g. adding something else.

I think my main point is that it is a (minor) change either way, just in a different direction. Either for content entities or config entities, so there is no perfect, 100%-absolute-guaranteee-BC-compatibility. And if we go there, then it might even theoretically be possible that someone managed to access $this->values directly for original, I've seen people doing crazy things when they look at entity objects in debugger/dpm().

We actually had cases where we used a _ prefix for new methods in patch releases but didn't do that in $next-minor.

Couldn't we make sure that the optimization works if the new API is properly used? If modules are still somehow relying on ->original then it won't and we'll need some kind of fallback.

But we can't make an optimization that only works because of a BC layer and would stop working when core doesn't actually use that BC layer anymore?

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -47,6 +47,19 @@
+   * This property will be set and used during the saving process.

That's not completely true.

When saving a new entity, $original is NULL.

I would argue that it when saving a new entity, $original should be identical to the entity itself, so that checks for a changed value are simplified.

Eg:

public function preSave(EntityStorageInterface $storage) {
  parent::preSave($storage);
  
  if ($this->myfield->value != $this->original->myfield->value && $this->myfield->value = 'foo') {
    // myfield has been set to 'foo'
  }
}

This code becomes much more complicated if you also need to check for a value of 'foo' on a new entity.

Just a reroll for now.

@joachim: I don't want to change the behavior of it here, happy to discuss that in a separate issue, @amateescu also suggested something like that above.

The way we handle deprecations has changed quite a bit since we discussed this, and I think I agree now that it would be better to not define $this->original so that we can add a trigger_error() later on in __get/__set. That said, that will make the patch more complicated, because I assume we *do* want an explicit protected property for this like $originalDefaultRevision or something. But that means we need to keep the two properties in sync through __get/__set().

Short and simple.

Also, not sure how to deal with #2859042: Impossible to update an entity revision if the field value you are updating matches the default revision. as that basically changes the meaning of $this->original under our feet, so the method we add here suddenly doesn't make sense anymore and we'd need to name it differently.

@jibran: Short and simple yes, but quite possibly no longer correct, but lets wait for the issue above or even merge the two together, not sure yet.

> @joachim: I don't want to change the behavior of it here, happy to discuss that in a separate issue, @amateescu also suggested something like that above.

I can completely understand not wanting to add complexity to this issue, but if that change is left to a follow-up, we have to commit to getting it in during 8.5 development, otherwise we'll have an API change on our hands.

This is "property" as in "PHP object property", not as in "Typed Data property". Which means it doesn't affect HTTP APIs (e.g. core's REST module). Great.

I ran into something related to this a few days ago.
I knew of the ->original entity thanks to Google, but would be nice to have it documented.
Would also be useful if it is mentioned that $entity->original fetches the original language and requires
$entity->original->getTranslation($entity->language()->getId())
to get the original version in the current language.

Bump, this still is valuable.

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -47,6 +47,19 @@
+   * @deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0.

we are can't deprecate in 9.5 no longer but it makes no sense to wait for 10.1

My gut reaction was that this is not a child of #3275851: [META] Fix PHP 8.2 dynamic property deprecations because ContentEntityBase::__get()/__set() handles this. But it is in fact a problem for config entities which don't have that, confirmed by running for example:

  32x: Creation of dynamic property Drupal\config_test\Entity\ConfigTest::$original is deprecated
    4x in ConfigEntityAdapterTest::testEntityDeriver from Drupal\KernelTests\Core\Entity
    4x in ConfigEntityAdapterTest::testValidate from Drupal\KernelTests\Core\Entity
    4x in ConfigEntityAdapterTest::testGetProperties from Drupal\KernelTests\Core\Entity
    4x in ConfigEntityAdapterTest::testGetValue from Drupal\KernelTests\Core\Entity
    4x in ConfigEntityAdapterTest::testSet from Drupal\KernelTests\Core\Entity
    ...

No ifea how to re-roll last patch

Phew. This issue hasn't been rerolled in a _long_ time. And yeah, know deprecation messages and so on will need to be updated, first we'll need to figure out what to do about it. Might want to go with a __get()/__set() based approach for deprecation anyway so we can trigger deprecation messages.

Also added a setter and replaced most usages that I could find.

Also, method name is still a problem. The current one is weird because it's for all entities, getting a default revision for a config entity makes no sense. Making it the current revision for revisionable entities by default and naming it getOriginal or maybe getUnchanged() is probably the way forward.

+++ b/core/lib/Drupal/Core/Entity/EntityBase.php
@@ -47,6 +47,19 @@ abstract class EntityBase implements EntityInterface {
+   * @deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0.
+   *   Use \Drupal\Core\Entity\EntityInterface::getOriginalDefaultRevision()
+   *   instead.

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
@@ -442,4 +442,25 @@ public function getConfigDependencyName();
+   * Returns the unchanged entity for the default revision.
...
+  public function getOriginalDefaultRevision();
...
+   * Set the unchanged entity for the default revision.
...
+  public function setOriginalDefaultRevision(?EntityInterface $original);

it needs CR and improve deprecation + needs follow-up to remove the property in 11.0.x

Filed stub CR https://www.drupal.org/node/3295826 and fixed CS

> And yeah, know deprecation messages and so on will need to be updated, first we'll need to figure out what to do about it

Fixing CS makes sense, but please hold on CR and stuff. There's a good chance that this will change a lot before it's done, including completely different method names.

patch makes fatal error, so needs work

Fixing that, also the explicitly defined public properties on specific subclasses that conflict with the current definition.

Fixed Views::postSave()

Fixed editor_editor_update() and some more updates.

Presumably the type hints for the original property and getOriginalDefaultRevision() method should allow null.

Fix #73

fix one more place

Thanks, but just so that we are clear, literally everything about needs to change to have a proper BC deprecation, changing the method name and implementation (merge in [#43] and change the behavior of it.)

I like how Laravel models just have "getOriginal" and "getChanges": https://laravel.com/api/9.x/Illuminate/Database/Eloquent/Model.html

For Drupal, \Drupal\Core\Entity\ContentEntityBase::hasTranslationChanges is basically `getChanges`. I prefer getOriginal over getUnchanged

Started reviewing this, but ran out of time before I got through the whole patch. Noticed this:

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1221,6 +1222,16 @@ public function __clone() {
+    // Ensure the original property is actually cloned by overwriting the
+    // original reference with one pointing to a copy of it.
+    $original = $this->original;
+    $this->original = &$original;
+
+    // Ensure the original property is actually cloned by overwriting the
+    // original reference with one pointing to a copy of it.
+    $original = $this->original;
+    $this->original = &$original;
+

Duplicate code block. Re-roll mistake?

@bradjones1 the issue is for 10.0.x but your MR is vs 9.5 would be great to see interdiff vs #75

Patch fixes #78 (As MR doing but for 9.5)

Yeah sorry I typed out a comment but didn't submit it... needed to apply this to a 9.5.x project and figured I'd at least make it an MR if others needed it. There's very little difference.

I strongly advice against using this in projects, I don't see why you would need to. The API and behavior is not final and you will have to adjust your code again.

Re: #83, Understood and am doing so understanding that the API is not final. That said, I ran into a bit of a conundrum where I needed to test code that accessed $entity->original, which didn't make Phpstan happy because it basically can't be typed. You can't really mock __get() in PhpUnit 9, ergo I went down this rabbit hole. I had, before finding this issue, created my own trait that I could add to a new interface that defined a getter... yadda yadda yadda. This seems like it has traction and some version of it will land in Core... and if the behavior changes I have a unit test that will break.

Thanks for the words of caution!

Been going through this. I wonder. Why not use the new method here?

Drupal\Core\Field\Plugin\Field\FieldType::ChangedItem:45

      /** @var \Drupal\Core\Entity\ContentEntityInterface $entity */
      $entity = $this->getEntity();
      /** @var \Drupal\Core\Entity\ContentEntityInterface $original */
      $original = $entity->original;
      $langcode = $entity->language()->getId();

Is that because original might be set to something else before that? But that would mean the property is just returned. Im confused :)

To be complete, this are the places i found which still access original (and are not by reference or added by the patch).

core/modules/content_translation/content_translation.module:207
core/modules/comment/comment.module:466 (could be set?)
core/modules/file/src/Plugin/Field/FieldType/FileFieldItemList.php:53
core/modules/image/image.module:380 and 421

Also i see entity.api.php still with a reference?

core/lib/Drupal/Core/Entity/entity.api.php:1183 and 1207

Addressed #85. Simplified the code in some places where we check or set ->original, as far as I can see it must always be set (or set elsewhere, in the case of comment.module).

+++ b/core/modules/comment/comment.module
@@ -461,9 +461,6 @@ function comment_user_cancel($edit, UserInterface $account, $method) {
-        // For efficiency manually save the original comment before applying any
-        // changes.
-        $comment->original = clone $comment;

This was added in #3166561: Comment being deleted instead of reassigned to Anonymous user but was not questioned anywhere in the issue, I'm not sure why it's necessary.

+++ b/core/modules/node/node.admin.inc
@@ -82,7 +82,7 @@ function node_mass_update(array $nodes, array $updates, $langcode = NULL, $load
   // For efficiency manually save the original node before applying any changes.
-  $node->original = clone $node;

I guess it was copied from here?

This "for efficiency" comment dates back to #651240: Allow modules to react to changes to an entity - when the $entity->original feature was first introduced! This is because of code like the following:

    // Load the stored entity, if any.
    if (!empty($node->nid) && !isset($node->original)) {
      $node->original = entity_load_unchanged('node', $node->nid);
    }

ie. an optimisation where $entity->original was not loaded if already set, and so callers could set the original entity if they already had it. But we seem to have lost that optimisation nowadays (as far as I can see), so I'm not sure this is needed any more? Let's see how the comment.module changes fares on testbot, and if so whether we can remove the rest of the similar setOriginalDefaultRevision() calls.

I've got a working Drupal 10 on a working PHP 8.2 environment, but cannot for the life of me reproduce

  32x: Creation of dynamic property Drupal\config_test\Entity\ConfigTest::$original is deprecated
    4x in ConfigEntityAdapterTest::testEntityDeriver from Drupal\KernelTests\Core\Entity
    4x in ConfigEntityAdapterTest::testValidate from Drupal\KernelTests\Core\Entity
    4x in ConfigEntityAdapterTest::testGetProperties from Drupal\KernelTests\Core\Entity
    4x in ConfigEntityAdapterTest::testGetValue from Drupal\KernelTests\Core\Entity
    4x in ConfigEntityAdapterTest::testSet from Drupal\KernelTests\Core\Entity
    ...

I did update my core/phpunit.xml for #3272779: [Symfony 6.1] Replace deprecationListenerTrait with PHPUnitBridge ignoreFile. I can see deprecations I trigger myself. But I cannot see any of these. Did something change?

My version:

$ php -v
PHP 8.2.0-dev (cli) (built: Oct  2 2022 00:49:17) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.2.0-dev, Copyright (c) Zend Technologies
    with Zend OPcache v8.2.0-dev, Copyright (c), by Zend Technologies

This is not a PHP 8.2 blocker because we have the dynamic properties allowed annotation on ConfigEntityBase, and it's only an issue for those. It's only a blocker for getting rid of that annotation (and having sane DX around this).

The only remaining blocker to having green tests on 8.2 is #3309748: Define missing object properties on non-testing classes for PHP 8.2, please help review that :)

Ah, so this is about being able to change

#[\AllowDynamicProperties]
abstract class ConfigEntityBase extends EntityBase implements ConfigEntityInterface {

to

abstract class ConfigEntityBase extends EntityBase implements ConfigEntityInterface {

Right? (I'm still getting familiar with all things PHP 8.2!) I was asked to assess whether this issue is truly essential for PHP 8.2 compatibility. You say it's not, but you do say that it is required for sane DX around this. So it's required to be able to do #3299857: [PP-1] Remove AllowDynamicProperties attribute from ConfigEntityBase? But you linked to #3309748: Define missing object properties on non-testing classes for PHP 8.2? And not #3299855: [META] Get rid of #[\AllowDynamicProperties] attribute? It's not clear to me what all the issue relationships are 🙈

I think we need 3309748 first, and then get rid of attributes

The primary issue for 8.2 is #3309748: Define missing object properties on non-testing classes for PHP 8.2. To help with PHP 8.2 compatibility, focus on that.

This is a child issue of #3299857: [PP-1] Remove AllowDynamicProperties attribute from ConfigEntityBase which is a child issue of #3299855: [META] Get rid of #[\AllowDynamicProperties] attribute. Both are PHP 8.2 compatibility follow-up issues, not blockers. They might end up being PHP 9 blockers, but that is quite a bit in the future and still not final AFAIK.

But that is a secondary and recent addition to this issue (which did help to revitalize this issue). This issue has been open since 2016 and the main focus was always to improve DX around the barely documented original property. As well as fixing a pretty serious bug with it if we include #2859042: Impossible to update an entity revision if the field value you are updating matches the default revision., which is what I plan to do.

And help to update those issues and update references and issue summaries is definitely welcome and probably the most useful thing to do right now.

Did you mean to link to #2859042: Impossible to update an entity revision if the field value you are updating matches the default revision., @Berdir, because that issue has not seen a substantial update in well over a year!

Yes I did, I wish it was just a year. See #43, that's how long this has been stuck before the recent activity.

it's blocker for #3299857: [PP-1] Remove AllowDynamicProperties attribute from ConfigEntityBase

Summary needs to be updated with current approach

@berdir any reason to put fix here from #2859042: Impossible to update an entity revision if the field value you are updating matches the default revision. it looks related but not required, what's left here to polish about API?

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 #2859042: Impossible to update an entity revision if the field value you are updating matches the default revision.. Doing it first in preSave() means we don't have to load unchanged *and* then the revision. Lets see if anything fails from this.

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"

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!