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

[klonos]

RTBC code-wise. @quicksketch still to chime in as to whether this change was in fact required for anything.

[olafgrabienski]

If the revert goes into a release, let's explicitly point to it, so that people who patched their contrib modules know that they can / should revert the patches.

[laryn]

@olafgrabienski We know that it is a fatal error to have core be static and contrib not static. I'm curious if there would be a problem if core was not static (ie. we roll that part back in core) and contrib was static (ie. a patched module didn't get unpatched). If I get a chance I'll test.

[jenlampton]

I was thinking the same thing. In fact, I've created a new PR that only reverts the base class and leaves all the others static: backdrop/backdrop#2699

[klonos]

...sounds like good/safe thinking @jenlampton 👍 ...perhaps add a note in the docblock to explain that this needs to remain non-static (while the other classes have been changed) in order to not break contrib + plus link to this ticket here.

[jenlampton]

okay, my tests disappeared(?) so I had to push an updated PR anyway. Here's what I have for documentation:

* Note: this function cannot be made static in 1.x because doing so will
* break many contrib projects that extend EntityInterface.

[jenlampton]

Well, the experiment reveals that you can't change static/non-static methods in either direction.

 Fatal error: Cannot make non static method EntityInterface::entityType() static in class Node in backdrop/backdrop/core/modules/node/node.entity.inc on line 10

So we're back to the original RTBC PR.

[laryn]

Thanks for testing @jenlampton it was worth a try.

[quicksketch]

Looks good to me. When that change was made I didn't think of inheriting classes in contrib. Or I thought inheriting classes didn't need to also be made static. Having those methods static would be helpful in some situations, but we can get the same information from hook_entity_info() about which classes are which entity types.

I've merged backdrop/backdrop#2698 into 1.x and 1.13.x. We'll want to make a release ASAP to fix this.