- πΊπΈ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 10:56pm 30 April 2023 - Status changed to Needs work
about 1 year ago 12:51am 1 May 2023 - πΊπΈUnited States smustgrave
Only moving back to NW for issue summary update per #97
- Status changed to Needs review
11 months ago 2:08pm 22 July 2023 - 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 The last submitted patch, 104: 2839195-104.patch, failed testing. View results β
- 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 6:50pm 22 July 2023 - πΊπΈ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 9:11am 25 August 2023 - π¨π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 9:49am 25 August 2023 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
10 months ago 8:18am 8 September 2023 - last update
10 months ago Composer error. Unable to continue. - π·πΊRussia kostyashupenko Omsk
Reroll against 11.x, so restoring "Needs review"
- last update
10 months ago Custom Commands Failed - Status changed to Needs work
10 months ago 3:39pm 8 September 2023 - π¦πΊ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.
- Merge request !5850Issue #2839195: Add a method to access the original property β (Open) created by acbramley
- π¦πΊ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!
- πΊπΈ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.
- Status changed to Needs review
about 1 month ago 2:32am 16 May 2024 - πΊπΈ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 2:43pm 16 May 2024 - πΊπΈUnited States smustgrave
Moving to NW for the suggestions made by @longwave.