Change to Entity Service GetByKey isn't breaking in 7.15

[KevinJump]

In Umbraco 7.15 there has been a change to the IEntityService interface,

in change 966b07b

IUmbracoEntity GetByKey(Guid key, bool loadBaseType = true);

Has been changed to

IUmbracoEntity GetByKey(Guid key);

and this isn't marked as a breaking change in Umbraco 7.15 release notes.

Also, I think because this is a change removing an optional parameter, it changes the references even if you don't pass the parameter

so :
If you have code compiled against a previous version of Umbraco that calls this method without the optional loadBaseType parameter you still get an error because the base reference has changed (this is what I think is happening)

example in uSync - (compiled against umbraco 7.7.6) - the call to a call to entityService.GetByKey is failing

(here https://github.com/KevinJump/uSync/blob/v4_master/Jumoo.uSync.Core/Serializers/MacroSerializer.cs#L54)

var entity = _entityService.GetByKey(key);

Results in

Method not found: 'Umbraco.Core.Models.EntityBase.IUmbracoEntity Umbraco.Core.Services.IEntityService.GetByKey(System.Guid, Boolean)

Even though we are not calling with the optional parameter.
(tracking usync issue KevinJump/uSync#223)

---

This item has been added to our backlog AB#1700

[kjac]

Yep true that 👍😀

[KevinJump]

yeah, this is unravelling pretty quickly for me :(

any package that calls entityService.GetByKey (or entityService.Get?) will now require a version compiled against 7.15, as other versions will YSOD - equally the code compiled against 7.15 with YSOD if installed on earlier versions of Umbraco.

this means having to maintain two support streams for code, and making sure people are clear about which version they should install :(

ideally, if the original functions could be reinstated but marked as obsolete? this would at least restore the signature of the IEntityService and things could continue to function?

[kjac]

I have no idea when and why the param was removed. But all the same it's definitely a breaking change. If indeed the param is without meaning as of 7.15, creating an overload + obsoleting the original method seems like an excellent approach.

Question then is how does that leave package devs for 7.15, given that it's out there in the wild? Simply "unsupported"?

[nul800sebastiaan]

Hey @KevinJump - sorry about this, we definitely should have been more careful when removing this parameter and introduced an overload instead. We can add it back but that means you'll have to wait until 7.15.1 for backwards compatibility support. Unfortunately, I'm afraid it will also take a while to get 7.15.1 out so I'm not sure if some kind of hack would be possible?

As background info: the param was removed for performance reasons, loading the baseType would cause the loading of all properties. This was bad in itself, but it would also mean that the feature "Ignore user start nodes option in pickers" (#2441) would be open up a loophole for people to easily fetch all data from nodes in the CMS instead of just showing the node name, the id and the image thumbnail. That would be a security risk, so we don't want people to be able to get all that data this easily. (hope that made sense, there's a long version of this in #2441).

[KevinJump]

yeah :(

for me, I am just wondering if replacing calls to Get/GetByKey with ones to GetAll, passing a single ID might work because that method hasn't been overloaded?

need to do some testing, see if that's possible where I am using it, etc..

[ronaldbarendse]

This is indeed a binary breaking change: https://docs.microsoft.com/en-us/dotnet/standard/library-guidance/breaking-changes#binary-breaking-change. As Umbraco 7.15.0 is already released, there's nothing you can do now besides re-compiling your own code/package (or release a new version with a work-around).

Releasing betas or using additional build tooling (e.g. NDepend or SonarQube) could have avoided this 🤷‍♂

[nul800sebastiaan]

Yep, we are always aware that we should not change method signatures even if the parameter has a default and is optional.

Again, apologies for that, we're contemplating what to do.

[ronaldbarendse]

I just noticed this breaks Umbraco Deploy (version 2.0.16) on Umbraco 7.15.0 with the following exception: Method not found: 'Umbraco.Core.Models.EntityBase.IUmbracoEntity Umbraco.Core.Services.IEntityService.GetByKey(System.Guid, Umbraco.Core.Models.UmbracoObjectTypes, Boolean)'.

[nul800sebastiaan]

@ronaldbarendse You'll need Deploy 2.1.0, which is what the Cloud upgrader should give you.

[vaskinhu]

Same problem for courier,

umbraco/Umbraco.Courier.Issues#11