Add a method to access the original property

Created on 25 December 2016, over 7 years ago
Updated 16 May 2024, about 1 month ago

Problem/Motivation

Currently we set $entity->original during the saving process however the entity object does not have such a property and therefor on content entities the magic methods __isset, __get and __set will be involved. In order to make the original property official and do not involve the magic methods it makes sense to create a property on the entity object for it.

Proposed resolution

Create a property $original on \Drupal\Core\Entity\Entity.

Remaining tasks

Review & Commit.

User interface changes

None.

API changes

None.

Data model changes

None.

πŸ“Œ Task
Status

Needs work

Version

11.0 πŸ”₯

Component
EntityΒ  β†’

Last updated 1 day ago

Created by

πŸ‡©πŸ‡ͺGermany hchonov πŸ‡ͺπŸ‡ΊπŸ‡©πŸ‡ͺπŸ‡§πŸ‡¬

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life

    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.

  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life

    Fixing that empty diff for 9.5.x πŸ€¦β€β™‚οΈ

  • Status changed to Needs review about 1 year ago
  • Status changed to Needs work about 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

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

  • Status changed to Needs review 11 months ago
  • last update 11 months ago
    Custom Commands Failed
  • πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

    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.

  • last update 11 months ago
    29,869 pass, 2 fail
  • πŸ‡¨πŸ‡­Switzerland Berdir Switzerland
  • last update 11 months ago
    29,879 pass
  • πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

    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 :)

  • Status changed to Needs work 11 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    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.

  • πŸ‡«πŸ‡·France andypost
    +++ 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

  • Status changed to Needs review 10 months ago
  • πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

    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.

  • Status changed to Needs work 10 months ago
  • 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.

  • Status changed to Needs review 9 months ago
  • last update 9 months ago
    Composer error. Unable to continue.
  • πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

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

  • last update 9 months ago
    Custom Commands Failed
  • Status changed to Needs work 9 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    acbramley β†’ changed the visibility of the branch 2839195-define-original-as to hidden.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    acbramley β†’ changed the visibility of the branch 2839195-9.5.x to hidden.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

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

  • Pipeline finished with Failed
    6 months ago
    Total: 186s
    #65029
  • Pipeline finished with Failed
    6 months ago
    Total: 672s
    #65030
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    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.

  • πŸ‡ΊπŸ‡ΈUnited States codesquatch

    Unfortunately #112 is failing against 10.2 :/

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    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.

  • πŸ‡ΊπŸ‡ΈUnited States codesquatch

    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.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    @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",
    
  • πŸ‡ΊπŸ‡ΈUnited States codesquatch

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

  • Pipeline finished with Failed
    about 1 month ago
    Total: 609s
    #172854
  • Pipeline finished with Failed
    about 1 month ago
    Total: 584s
    #172864
  • Pipeline finished with Failed
    about 1 month ago
    Total: 602s
    #172870
  • Pipeline finished with Failed
    about 1 month ago
    Total: 664s
    #172896
  • Pipeline finished with Failed
    about 1 month ago
    Total: 693s
    #172905
  • Pipeline finished with Failed
    about 1 month ago
    Total: 693s
    #172926
  • Pipeline finished with Failed
    about 1 month ago
    Total: 526s
    #173001
  • Pipeline finished with Failed
    about 1 month ago
    Total: 606s
    #173059
  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life

    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.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 974s
    #173069
  • Pipeline finished with Failed
    about 1 month ago
    Total: 715s
    #173865
  • Pipeline finished with Failed
    about 1 month ago
    Total: 692s
    #173872
  • Pipeline finished with Success
    about 1 month ago
    Total: 652s
    #173883
  • Status changed to Needs review about 1 month ago
  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life

    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.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    Added some questions and suggestions.

  • Status changed to Needs work about 1 month ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Moving to NW for the suggestions made by @longwave.

Production build 0.69.0 2024