Originally from https://www.drupal.org/node/2656742#comment-10795610

CommentFileSizeAuthor
#7 2658836-null-entity-access-check.patch2.71 KBCrell
#6 2658836-null-entity-access-check.patch2.05 KBCrell
no-entity.patch995 bytestimmillwood
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

timmillwood created an issue. See original summary.

Crell’s picture

Crell CreditAttribution: Crell at Palantir.net for Acquia commented

The linked comment(s) suggest this issue occurs when the links block is undefined in an entity. I... do not understand how that would happen, since if there's no links block the route shouldn't even be defined in the first place and this code should be unreachable.

I'd like to understand how that happens before committing this, as it may suggest a better alternative approach. (I generally assume that if you need to protect against null, it means there's a logic error elsewhere to begin with.)

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

I understand it's a bit of an edge case, but Multiversion has been working fine without all of the links defined, and I'm sure there will be other modules that think they're OK.

This patch is just a prevention.

Also it seems odd that the one method knowingly returns null and there is nothing preventing that being passed through to something that doesn't accept null.

Crell’s picture

Crell CreditAttribution: Crell at Palantir.net for Acquia commented

But how does links being empty result in this issue? That's what I don't understand. I don't get how that connection happens.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

Ah, it causes the parameters from RouteMatch to be empty

Crell’s picture

Crell CreditAttribution: Crell at Palantir.net for Acquia commented
FileSize
2658836-null-entity-access-check.patch2.05 KB

Tim and I talked this through earlier today. The issue isn't links per se; it's that the current code in loadEntity() presumes an _entity_access requirement, which isn't guaranteed to be there in all cases. It then returns NULL rather than properly handling that error case, and things blow up.

This patch changes loadEntity() to use a hopefully more robust approach, and throws an exception in case it doesn't work. Because returning "something useful or null" is rude: http://www.garfieldtech.com/blog/empty-return-values

Let's see if the bot approves...

Crell’s picture

Crell CreditAttribution: Crell at Palantir.net for Acquia commented
FileSize
2658836-null-entity-access-check.patch2.71 KB

And now thanks to EclipseGc's input, here's an even better approach. (No interdiff as it would basically be the entire patch.)

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc at Acquia commented
Status: Needs review » Reviewed & tested by the community

Looks solidly better than what's there!

Eclipse

  • Crell committed 4e26d7f on 
    Issue #2658836 by Crell, timmillwood, EclipseGc: If entity is null, deny...
Crell’s picture

Crell CreditAttribution: Crell at Palantir.net for Acquia commented
Status: Reviewed & tested by the community » Fixed

Went ahead and merged. Thanks, Tim and Kris!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.