revert entityType() so that it is not static (breaks lots of contrib)

[jenlampton]

It looks like we'll need to revert or partially revert some of #3011 Add entity_access()

This issue had some unforeseen complications that are breaking a lot of contrib.

See:

• Fatal error, need to update EntityInterface entityType to be static backdrop-contrib/multifield#27
• Fatal error, need to update EntityInterface entityType to be static backdrop-contrib/flag#14
• Fatal error with EntityType backdrop-contrib/rules#44
• Fatal error after updating to 1.13.0 ("Cannot make static method EntityInterface::entityType() non static") backdrop-contrib/paragraphs#39
• Static functions backdrop-contrib/ubercart#190
• more?

---

PR: reverts all enityType() functions: backdrop/backdrop#2698
PR: alternate approach that only reverts the base class: backdrop/backdrop#2699 breaks everything!

[docwilmot]

backdrop-contrib/ubercart#190

[klonos]

...adding some excerpts from the related discussions on Gitter:

@herbdool

Can't comment in forum. Anybody else also getting this error? Fatal error: Cannot make static method EntityInterface::entityType() non static in class Flagging in /home/forum/repo/www/modules/contrib/flag/includes/flag.entity.inc on line 0
Was there a recent update to EntityInterface? Or maybe a PHP upgrade
Also get error when trying to flag something
Ah yes, @since 1.13.0 This method is now static.

We may need to check all contrib modules

Looks like @laryn was on the ball with Paragraphs. I noticed Rules needs the fix

@docwilmot

thats bad. as in a mistake. that might break important things.

@herbdool

the contrib modules will have to put a minimum version of core 1.13.0 I think?

@klonos

Yup

@docwilmot

As far as I can see, this method was only overridden to be static in comment entity, right? Was that a mistake?

@docwilmot

@quicksketch should we be fixing this static method issue by fixing contrib or rolling back the core change?

@klonos

I think that if there was no specific reason for the change, then roll back the core change @docwilmot

@herbdool

@docwilmot paragraphs was already updated so a rollback would need to happen there too

@serundeputy

seems like rolling back core is the thing; that is an API change that breaks contrib and our core principles

@klonos

Indeed. Unfortunate that we did not catch that one.

@docwilmot

@herbdool @serundeputy I'm not good at OOP, is that static declaration necessary for the functionality we added though?

@serundeputy

don't know; i'd just take it off and run tests; and specifically install flag and try to comment
if you have a forum site perhaps just do it there as an initial test

@herbdool

@docwilmot I'm not sure

@serundeputy

i'm guessing it will work fine as that is the way it was before this change
backdrop/backdrop@5741961#diff-d3a8d074d6ebaadee7f6ba7cadebe520R94

@docwilmot

Oh, looks like all entityType() declarations were made static, not just on Comment, as I thought previously. Don't know what I was looking at before. But still it's not called anywhere new, so I really don't see the point.

@herbdool

@docwilmot and the change seems unrelated to the issue, which is access control

@docwilmot

@herbdool agreed

@laryn

@herbdool I was only "on the ball" in the sense that someone reported an error: backdrop-contrib/paragraphs#39
I've updated Paragraphs but not made a release yet, so I don't think that change is out in the wild (except by anyone who may have manually patched, like me).
Also unsure if the error runs both ways (ie. making a non static method into a static method, in which case I'd have to reverse the change on Paragraphs).
Actually, I guess I also found the error in Ubercart: backdrop-contrib/ubercart#190

@docwilmot

@jenlampton we discussed that entityType() issue yesterday, and pinged @quicksketch on here. I think the core change needs rolling back. Someone else agreed too.

@jenlampton

too late, I’ve already fixed the problem.
well, I mean, we can change core if necessary - but at least the forum site is up again :)

@docwilmot

yes problem is it breaks a bunch of contrib
and it doesnt seem necessary.

@klonos

It was at least another one who agreed re the roll back ...myself is one; and I believe @herbdool or @serundeputy too

@jenlampton

I haven’t reviewed that issue. I was just fixing the forum site.

@klonos

...but we need to roll this contrib-breaking change back if we are to stay true to our principles/values

@jenlampton

agreed. Is there a PR? I’d be happy to test that on the forum site too if that would help.

@docwilmot

i dont know OOP too good. hence the ping to @quicksketch. not sure the 'why' of that change.

@klonos

Same as @docwilmot ...not good with OOP and unsure why that was changed to begin with.

@jenlampton

has the discussuon been happening on the 1.13 issue or is there a new one for reverting the change?
I just ran into it again on a 3rd site, and am also in agreement that it needs to be reverted.

@jenlampton

oh jeez the git blame says this was my fault :( sorry everyone!

commit 5741961928f43af00c146cede747eb16e1805aa9
Author: Jen Lampton <jenlampton@users.noreply.github.com>
Date:   Wed May 1 17:40:21 2019 -0700
    Issue #3011: Added entity_access and Entity::createAccess() and Entity::access() for all Entities.

I repoepend #3011, @docwilmot @serundeputy @herbdool I’ve listed a few of the issues I ran into there, feel free to add others if you have them (or, point me at any other issues that were opened about this?)

@klonos

...I just added our previous discussions re the entityType() fatal with contrib modules + links to specific issues in #3809 ...lets keep discussions there