backdrop-contrib / og

The Organic Groups module provides users the ability to create, manage, and delete 'groups' on a site.
GNU General Public License v2.0
1 stars 8 forks source link

og_query_og_membership_alter is defective and affects EntityFieldQuery #67

Closed argiepiano closed 2 years ago

argiepiano commented 2 years ago

I've spent some time (quite a bit actually) troubleshooting testEntityFieldQuery(), specifically the first failing part that attempts to query multiple group audience fields in a single group content type, and fails. This test doesn't fail in D7.

After also digging online, I've come to realize that the query alter that og performs is faulty. See below for a more detailed explanation. This problem (inability to perform an EntityFieldQuery of an entity type that contains group references to two different groups) has been reported here (with a few minor difference, but it's pretty close to the test problem in Backdrop's OG).

The EntityFieldQuery performed in og.test for multiple group audience fields (line 1272 of og.test) translates into the following db query:

SELECT DISTINCT ogm.entity_type AS entity_type, ogm.etid AS entity_id, node.vid AS revision_id, node.type AS bundle
FROM 
`simpletest24657og_membership` ogm
INNER JOIN `simpletest24657node` node ON node.nid = ogm.etid
WHERE  (ogm.gid = :db_condition_placeholder_0) AND (ogm.gid = :db_condition_placeholder_1) AND (node.type = :db_condition_placeholder_2) AND (ogm.entity_type = :db_condition_placeholder_3)

In Backdrop, :db_condition_placeholder_0 = 2 :db_condition_placeholder_1 = 3 :db_condition_placeholder_2 = [the random bundle name of the group content node type] :db_condition_placeholder_3 = 'node'

You can see that this query is wrong in the WHERE statement (first two items there). You cannot query with multiple WHERE for the same column (see this).

So, why does it work in Drupal? By accident. The first two placeholders have the same value of "1", since the entity IDs for the referenced node and the referenced entity_test entity happen to be same.

Proposed action

For the time being, let's comment out this test and focus on more immediate needs. For the future, the D7 issue linked above has a (complex) solution. Let's keep an eye on that to see if it's merged.

Pinging @indigoxela as they worked on the EntityFieldQuery tests and may have additional thoughts.

argiepiano commented 2 years ago

Ultimately, this problem is the result of some "frankenstein coding" courtesy of the authors of OG.

It looks like OG started in Drupal 4, and was gradually "modernized" with Field API and later with Entity API and Entity Reference. The problem is that the module never took full advantage of those APIs - they always move half-way to adopt those. For example, none of the Entity Reference fields that reference groups in OG are used properly. Their tables remain always empty, and instead they use a single table, og_membership, to record all values of ALL group reference fields. Ugh

argiepiano commented 2 years ago

This comment - Comment#12 - is relevant, from that same D7 issue I mentioned above.

That's a real bug. There is no way to make that EFQ work with 2 OG fields for now. You can execute two different EFQ then intersect the results if you don't want to wait for a fix.

As far I remember (that was 2 years ago) OG rewrited the request to replace the fielddata* tables generated by entityreference by the og_membership table instead. The thing was that it only used one join on the og_membership table so we ended with two opposite "where" clauses on the same og_membership join.

argiepiano commented 2 years ago

Rather than commenting out that test, I have come up with an alternative way to test performing an EFQ of entities with multiple audience fields, following the D7 comment linked above.

PR #68 .

indigoxela commented 2 years ago

I've spent some time (quite a bit actually) troubleshooting testEntityFieldQuery()

Yes, I bet on that. :wink:

So, why does it work in Drupal? By accident. The first two placeholders have the same value of "1"...

Ha! Great sleuthing! I think, the change in your PR makes sense. It's a fix for the immediate problem with info about further steps.

laryn commented 2 years ago

Thanks @argiepiano for the analysis and detailed explanation. Merged.