Issue summary: Unable to create or edit a Document Page in Section unless administrator

Background: I've got OpenAtrium 2 freshly installed (2.223 / aka 2.623 ? wierd release tag) and running mostly as I want it except for some permissions, some/most content creation things work for non-admin except for create a Document Page in Section unless that og group administrator also has the 'Administrator' role.

after enabling dblog (watchdog) I found the actual error message: Column not found: 1054 Unknown column ....entity_id

I reviewed the OpenAtrium do••••••••cumentation, and even installed again OpenAtrium in a sandbox and was able to repeat the issue it seems with a fresh sandbox.

Affects versions between 2.619 and 2.623

After extensive debugging I found a working solution

WORKAROUND / WORKING SOLUTION

For openatrium version 2.619 through 2.623:
use this patch for file_entity that fixes this.
here is the patch:
https://www.drupal.org/files/issues/file_entity-REVERT_2067671-EntityFie...
here is t••••••••he comment for the patch:
#2067671-35: Ensure file query alteration is performed when using EntityFieldQuery

put the patch in profiles/openatrium/modules/contrib/file_entity
and apply it.
(if you need help applying the patch, please let us know)

Comments

joseph.olstad created an issue. See original summary.

joseph.olstad’s picture

Title: Unable to create or edit a Document Page in Section unless administrator » Unable to create or edit a Document Page in Section unless administrator
Issue summary: View changes
mikai’s picture

I'm having the same issue. Reverting back to 2.618. Hopefully this gets fixed.

PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'field_data_og_group_ref0.entity_id' in 'where clause': SELECT ogm.entity_type AS entity_type, ogm.etid AS entity_id, node.vid AS revision_id, node.type AS bundle FROM {og_membership} ogm ......

/modules/field/modules/field_sql_storage/field_sql_storage.module).

joseph.olstad’s picture

Mikai, can you please confirm if downgrading resolves this?

joseph.olstad’s picture

Wondering if this has something to do with using the quick install vs the full standard installation.

I just spent about 36 hours creating a site from scratch with 2.223 (2.623) , not sure if I can fix it without data loss.

joseph.olstad’s picture

Category: Support request » Bug report
Status: Active » Needs work

Looks like it might be a bug. Mikai confirms that I'm not inventing this issue.

joseph.olstad’s picture

@mikai, did downgrading to 2.618 fix the problem? Just wondering, I'll try it on my same schema, hopefully it works. Not really excited with the idea of re-doing my site from scratch but if I have to, it's only a day or two worth of work.

I'll try downgrading openatrium code to 2.618 with same schema and see if that helps.

joseph.olstad’s picture

Priority: Normal » Major

I had a look at the mysql file openatrium.mysql 2.618 vs 2.623

2.618 known as working, the file openatrium.mysql is over 5 megabytes
2.623 with the above noted issue openatrium.mysql is about 3 megabytes

so what was removed or 'left out' from openatrium.mysql that caused this to break? if indeed it's the schema.

I checked the code, the og module was unchanged, the entityreference module unchanged, same patches

nothing code wize stuck out except the panopoly_core upgrade , however I'm going to run a few tests, the 2.623 code base with the 2.618 mysql file see what happens.

joseph.olstad’s picture

Ok, have isolated the problem, it is in openatrium core.
I am going to continue to isolate the problem, find out which module and which patch is responsible for this regression

joseph.olstad’s picture

Assigned: Unassigned » joseph.olstad

Solution to follow soon.

joseph.olstad’s picture

Short term solution, replace openatrium core with version 2.618
I will isolate further and find out which exact module is responsible.

joseph.olstad’s picture

Ok, getting closer, file_entity module; run 2.623 , but replace file_entity with the build of file_entity that is found in 2.618
this allows you to use the latest openatrium

Going to dig further into file_entity to find out why this would cause an issue.

joseph.olstad’s picture

Still investigating, I might have messed up my tests, have to try again.

joseph.olstad’s picture

Title: Unable to create or edit a Document Page in Section unless administrator » Unable to create or edit a Document Page in Section unless administrator
Status: Needs work » Needs review

I have a patch for file_entity that fixes this.
here is the patch:
#2067671-35: Ensure file query alteration is performed when using EntityFieldQuery

for openatrium build 2.623 put the patch in profiles/openatrium/modules/contrib/file_entity
and apply it.

joseph.olstad’s picture

Assigned: joseph.olstad » Unassigned

Please see panopoly queue for the panopoly_widgets make file patch
#2473495-24: Patches used in Open Atrium for Panopoly Widgets

joseph.olstad’s picture

Version: 7.x-2.223 » 7.x-2.x-dev
Priority: Major » Critical

I'd say this is a critical issue.

as the main functionality of openatrium revolves around group permissions.
Fix is available. see my latest comments above.

mpotter’s picture

I need to understand more about this issue.

It seems the patch #35 in #2067671: Ensure file query alteration is performed when using EntityFieldQuery simply reverts a commit made there. I don't want to "undo" something that has been commited to another module release if at all possible. If the update to file_entity caused Atrium to break, I'd like to know more about what I might do in Atrium to fix that.

Reverting the file_entity would be the last resort. Especially since file_entity is coming via Panopoly and the Panopoly tests are passing with it.

mpotter’s picture

Status: Needs review » Needs work
joseph.olstad’s picture

Prior to and including Atrium v2.618 openatrium was using file_entity 7.x-2.0-beta3 , beta3 did NOT have the file query alteration support for EntityFieldQuery , however it was added in 7.x-2.0 of file_entity (7.x-2.0 was released 7 months ago, and I haven't seen any complaints related to this commit until Atrium.

The latest atrium has file_entity 2.12 , I created a patch for that version to revert the file query alteration support for EntityFieldQuery. beta4 did not have this commit, atrium works with that version too, every file_entity module release since 7.x-2.0 has the query alteration support for EntityFieldQuery , all those versions do not work unless patched.
n the wild
I recommend including the patch that I created for file_entity 7.x-2.12 . We can open an issue for figuring out the 'why' and 'how' , meanwhile we can release a new build that includes the fix.

this is critical to the functionality of Atrium, we have a working solution, we can use the patch now and make a new release and then follow up with the issue to discover why.

I'm not that familiar with organic groups or the custom open atrium modules, and only started recently using Atrium 2, but I'll follow up with the why and how but for now I really think we should have a fully working release.

Not sure why we'd want to leave the current tagged release in such a state, cause others a bunch of trouble when we have a known fix that has no known issues.

I also checked the panopoly queue, nothIdeally we'd know why and how everything works in all casesing in there related to this EntityFieldQuery support. Nothing in file_entity queue either.

usually if there's a problem someone will report it.

joseph.olstad’s picture

I tested all versions of file_entity since
7.x-2.0-beta3
beta3 works because it does not have the EntityFieldQuery alter query support
beta4 works because it does not have the EntityFieldQuery alter query support
2.0 not compatible unless revert the EntityFieldQuery alter query support
2.1 not compatible unless revert the EntityFieldQuery alter query support
2.2 not compatible unless revert the EntityFieldQuery alter query support
2.3 not compatible unless revert the EntityFieldQuery alter query support
all subsequent releases fail
2.12 not compatible unless revert the EntityFieldQuery alter query support (must use the patch I created as mentioned)

solution is to revert EntityFieldQuery alworks because it does not have the EntityFieldQuery alter query supportter query support using the patch I suppworks because it does not have the EntityFieldQuery alter query supportlied.

As to how and why , we can open a followup issue for how and why. What is more critical however is fixing this so that others don't spend as much time as I already have debugging and resolving this issue. So that others don't have to downgrade or run a site with a security flaw , as the recent atrium release was a security release based on the panopoly security release.

So for the sake of security (allowing others to run the latest security fixed release) and to help others not pull their hair out we should make a new release that includes the fix as mentioned.

mpotter’s picture

I fully understand the need to get a new release out. However, reverting functionality in file_entity that was added 7 months ago still isn't a good solution. Remember that file_entity comes from Panopoly not from Atrium directly and it was Panopoly that recently updated it from beta3 to the 2.12 version. My concern is that other modules that were updated, such as media, might depend on that new functionality.

What I need is an understanding of what the purpose of "EntityFieldQuery alter query support" is and how Atrium can support that. I don't want to rush out yet another release and then have to do another release to "fix it right" if possible.

At this point I'm launching a client site this week so I'm not likely going to have time to do an Atrium release. So this gives us about a week to figure out a better solution if possible.

joseph.olstad’s picture

I'm also the main active maintainer of media, file_entity and the media_dev distribution , I can confirm that media requires a recent version of file_entity (2.12 to get the full optimal functionality) but it (media) does not require alter query EntityFieldQuery commit.

I could revert the EntityFieldQuery commit and release file_entity 2.13 , however no one else has reported problems so I thought the easiest way to fix openatrium is to patch file_entity 2.12 with the previously mentioned patch that reverts that commit.

However if I release file_entity 2.13 as described, there's no guarantee that panopoly will start using it right away.

joseph.olstad’s picture

So ya, I appologize for the inconvenience, however that commit was done 7 months ago, tens of thousands of installations of file_entity 2.0+ later and only now I find an issue.

joseph.olstad’s picture

There's approximately 97000+ installations of file_entity >= 7.x-2.0 that have the file alter query EntityFieldQuery support.

after 7 months, the first reported issue was brought up by me, when using openatrium .

I'm not sure how many sites might be affected by this issue, but I did not find any reported issues in the file_entity queue , nor did I find any reported issues in the panopoly queue regarding this.

that does not mean that there aren't any other reported issues, but it wasn't obvious and I haven't seen any yet except the openatrium one issue I created.

joseph.olstad’s picture

just FYI, all my related testing for this issue was done on both php 5.4.x (CentOS 7) and php 7.0.22 (Ubuntu 16.04.3 LTS)
same results on both

mpotter’s picture

Sorry I didn't realize you were a media & file_entity maintainer.

But your comments above are exactly right...this shouldn't be something we need to "fix" in file_entity. You don't need to release a 2.13 version. It's working for lots of people, and being used by media, etc. That's why I don't want to just revert the commit you made or have you release a new version.

The problem here is with Atrium. Specifically something in the oa_core.access.inc that handles the file_entity access control. That's where I want to fix this. I just need to understand what the EntityFieldQuery change in file_entity did. Fixing oa_core to support this change shouldn't be too hard, but it will take me time to look through the code and figure it out.

As somebody more familiar with the change in file_entity, I was hoping you might have an idea what I might need to change in oa_core.

My guess is that the reason only Atrium has this issue is that Atrium is rather unique in it's access control model and amount of code handling access control. For file_entities it specifically looks up the node(s) that the file is attached to and tried to grant file access based on the parent node access. My guess is that's where the problem lies.

joseph.olstad’s picture

I had a quick peek at the oa_core modules and code.
To debug this, I'd start by looking at :
og_membership_load_multiple

whether from the caller (oa_core / oa_worktracker) or from where it is defined (og.module), is possibly related to:

PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'field_data_og_group_ref0.entity_id' in 'where clause': SELECT ogm.entity_type AS entity_type, ogm.etid AS entity_id, node.vid AS revision_id, node.type AS bundle FROM {og_membership} ogm ......

Mitigated/avoided by reverting support for file alter query EntityFieldQuery code in file_entity
#2067671-35: Ensure file query alteration is performed when using EntityFieldQuery

by looking at the PDOException , I saw og_membership in the query.
og_membership is a parameter for og_membership_load_multiple

oa_core/oa_core.module:

return !empty($result['og_membership']) ? og_membership_load_multiple(array_keys($result['og_membership'])) : array();

oa_worktracker/plugins/entityreference_selection/OaWorkTracker_SelectionHandler_Assigned.class.php:

$og_memberships = og_membership_load_multiple(array_keys($result['og_membership']));

og/og.module:

function og_membership_load_multiple($ids = array(), $conditions = array(), $reset = FALSE) {

og_subgroups/og_subgroups.common.inc:

$og_memberships = og_membership_load_multiple(array_keys($result['og_membership']));

og_subgroups/og_subgroups.common.inc:

$memberships = og_membership_load_multiple($membership_ids);
joseph.olstad’s picture

Issue summary: View changes

update issue summary, include the working solution.

jeffm2001’s picture

So I looked at this for a bit, and I think I have an idea of what's going on, though honestly I got kind of lost, so I'm not 100% sure.

The query that causes the error starts off as something like:

SELECT field_data_og_group_ref0.entity_type AS entity_type, field_data_og_group_ref0.entity_id AS entity_id ...
FROM  {field_data_og_group_ref} field_data_og_group_ref0 ...

_file_entity_query_file_entity_access_alter() does a bunch of stuff, and ultimately adds a field_data_og_group_ref0.entity_id = fm_access.fid into the WHERE clause of a subquery.

Later, og_query_og_membership_alter() seems to be changing the table name/alias in the outer query to {og_membership} ogm, but the subquery condition stays the same, causing the Column Not Found error.

At least that's my sense of things.

Assuming this is correct, I'm not sure the best way to go about fixing it.

One option would be to add this to the file_entity alter:

if ($query->hasTag('og_membership')) {
  return;
}

@joseph.olstad if that sounds right to you, I'm happy to upload a patch in the file_entity issue queue.

joseph.olstad’s picture

Sure put a patch in file_entity for that.
Pragmatic solution until we figure out a better solution.

joseph.olstad’s picture

Thanks @JeffM2001

at first glance it appears that this file_entity issue might affect more than just openatrium, thinking it also affects og organic groups.

So, therefore, I'm thinking we should expedite the fix.
It'd be nice to have a test case for this in the file_entity module as well, if you can do it great, if not, we'll probably have to do without.

the next file_entity module release will be 2.13

I'm giving 7 days maximum to sit on this.

joseph.olstad’s picture

jeffm2001’s picture

Yeah, I think it affects any query which is tagged with 'entity_field_access' and 'og_membership' and whose base table is a group audience field. Unfortunately, I'm not sure how to write a test for that.

mpotter’s picture

Thanks for all the work looking into this. I did some quick looking at code myself, and am wondering if the real solution to this is a patch for OG, rather than a release of file_entity that contains this special case?

Trying to understand the og_query_og_membership_alter() function in OG and it's certainly quite complex.

If you want to fix it in file_entity, rather than checking for the existence of OG etc, I wonder if you can check to see if the base table has been modified. Because maybe some other module (other than OG) could do a similar query_alter and cause the same kind of issue?

I guess I can see that it might be an call-order problem...if OG is altering the query *after* file_entity, then file_entity can't really detect that in a general way. This is definitely trickier than I original realized.

joseph.olstad’s picture

Hmm, ya , I'm not sure what the module weights are (haven't yet looked, FYI: it is in the system table), but probably f comes before o in the alphabet, so it's likely that file_entity runs first.
In this case the patch that JeffM2001 wrote is probably the best short term solution.

I'm not sure how much that alter EntityFieldQuery in file_entity is used in the wild. Removing it completely might not actually do much harm.

On the other hand, the patch that JeffM2001 wrote is dealing with this case and has a tighter scope. Less likely to ruffle feathers in the wild.

I have not yet had the chance to run my own tests on his patch, I know that my patch that removes the alter FieldEntityQuery from file_entity does work. Before I action this in file_entity I will make sure to test it on my own openatrium 2.623 environment.

I'll have a look on the weekend.

mpotter’s picture

@joseph I think there is something that can be done better in the file_entity code.

I was able to trace this via xDebug and what it's doing on the node/add form is trying to create the drop-down list of allowed Sections for the current user and current space. So it ends up doing an entity_field_access query via og_membership as mentioned above.

However, there is nothing about this query that has anything to do with file_entity. So I'm not sure why file_entity is trying to alter the query.

I think there is some additional conditionals needed in _file_entity_query_file_entity_access_alter() for this. Going to investigate it a bit more to see if I can suggest something. But looking over the code I think your query alter is handling more queries than it should and could be a problem beyond anything in OG.

mpotter’s picture

Specifically, it is this condition that is causing the alter to run:

          elseif (isset($schema['fields']['entity_id']) && isset($schema['fields']['entity_type']) && isset($schema['fields']['deleted']) && isset($schema['fields']['delta'])) {
            // The table is a field data table, use it as fallback.
            $base_table = $table;
          }

which sets $base_table to "field_data_og_group_ref"

mpotter’s picture

It looks like you are allowing any sort of "entity reference" field as a possible reference to a media entity, without checking the entity type. Maybe you can check the $query->getMetaData('entity_field_query') and get the entityConditions and check that the entity_type is "media" or something. In this Atrium query, the entity_type is "node" (because it's trying to query a list of allowed oa_section nodes).

jeffm2001’s picture

@Mike, yeah, I found the same thing. Which makes some sense, because that's what was added by the patch committed in #2067671: Ensure file query alteration is performed when using EntityFieldQuery.

It's altering anything that looks like a field data table, and this code is supposed to restrict the effect to only file fields:

    // All the file access conditions are only for field values belonging to
    // files.
    if ($base_alias !== 'file_managed') {
      $file_conditions->condition("$base_alias.entity_type", 'file');
      $or = db_or();
      $or->condition($file_conditions);
      // If the field value belongs to a non-file entity type then this function
      // does not do anything with it.
      $or->condition("$base_alias.entity_type", 'file', '<>');
      // Add the compiled set of rules to the query.
      $query->condition($or);
    }

But maybe there's a way to do the same thing by looking at the query/field metadata and only alter the query when needed.

Another key line is $subquery->where("$falias.$field = fm_access.fid");

That WHERE clause is the one that triggers the error. Possibly if that can be rewritten as a JOIN instead that would solve the problem. (I'm not sure that it can be though).

Finally, and maybe the simplest, this is definitely a call-order problem — file_entity is trying to use a value from the outer query (field_data_og_group_ref0.entity_id) in its subquery, but then OG comes along and changes the base table. If OG's alter runs first the issue goes away.

I initially thought this wouldn't work, but I just tried it, and adding this to file_entity_module_implements_alter() eliminates the error:

if ($hook == 'query_alter' && isset($implementations['file_entity'])) {
  $group = $implementations['file_entity'];
  unset($implementations['file_entity']);
  $implementations['file_entity'] = $group;
}
mpotter’s picture

That might not be a bad approach. While the code looks odd (and would need comments), what it basically does is move the file_entity alter to the end of the list, so it runs last. Seems like there is also a way to control the module "weight".

Although I'd still like to see a better fix that doesn't actually alter the query unless it really has a file entity reference. The idea of making all the entity queries more complex (and probably slower) even when they don't have anything to do with file_entity somewhat bothers me.

joseph.olstad’s picture

Just FYI, here's the original issue that brought in alter EntityFieldQuery into file_entity
it was worked on by Dave Reid and some other heavy weights. It sat in the queue for a long while, they wrote a simpletest for it, and 7 months ago or so I decided to put it in because I didn't see why it shouldn't go in, and it was marked as a 2.0 release blocker.

However today, I'm wondering if it should just be removed completely. I've never used alter EntityFieldQuery for file_entity myself, not sure who is using it or what contrib modules "might" be using it. The fact that it causes a pretty serious regression is alarming and so I'm willing to do whatever is most expedient to resolve this. If it involves removing it altogether, that would presumably resolve organic groups regressions , however there's a couple ideas up here and patches to review.

Probably least disruptive to make this fix for organic groups a specific case. However I could be convinced to revert file_entity alter EntityFieldQuery , anyone who complains about that we can easily justify why we reverted and then ask the community to chip in for resolving the known issues if they want it back. For my projects I cannot recall ever having used it specifically. I do not know of any modules that are. However that doesn't guarantee that there aren't any.

joseph.olstad’s picture

Weigh the pros and cons
is out of the box alter EFQ support really necessary for file_entity?

I'm thinking no, probably just extra bloat that we don't need. If someone needs it, they can revert the revert patch.

I'm leaning towards reverting it completely. Something with it seems broken to me.

jeffm2001’s picture

Having read through the issue you linked, and the parent issue it links to (#1959260: Ensure a 'file_access' tag is used on all queries of outputted files => alpha1 breaks EntityFieldQuery not releated to files), I'm inclined to agree that the patch should be reverted.

It's really unclear to me exactly what they were trying to do with the EFQ alter in the first place. It makes total sense that file_entity would alter queries for files based on file permissions, but I'm not sure you would want it to alter queries for other entities that just reference files.

joseph.olstad’s picture

Ok, so ya I'll revert file_entity alter EFQ this weekend, unless someone can convince me otherwise.
I'll publish a new release 7.x-2.13 immediately afterwards.

joseph.olstad’s picture

Issue summary: View changes

Ok, I decided to go with the small patch that @JeffM2001 published
I tested it myself using openatrium 2.623 and applying his fix to that after reverting the other and it works.

file_entity 7.x-2.14 is released with his patch.

I did not revert the entire EntityFieldQuery support from file_entity , decided to go with @JeffM2001 patch as it was small and smaller in scope.

I'll keep an eye out for related issues.

For now, we need to get the panopoly folks to do a release with file_entity 7.x-2.14

joseph.olstad’s picture

Status: Needs work » Reviewed & tested by the community
joseph.olstad’s picture

joseph.olstad’s picture

I also created an issue upstream in panopoly queue
#2934959: upgrade file_entity, fix for organic groups og regression

mpotter’s picture

Hmm, I was a bit sad to see you go with the OG-specific approach. I think you were right on in #43. I'm still very concerned by the fact that the query_alter affects *all* field entity reference queries and not just file entity queries. Having read more of the discussions, I honestly would have just reverted the EFQ alter until more work can be done on it to ensure it only affects media queries. No module should be altering queries outside the scope of that module.

I'm doing a client deployment today, but am scheduling work on Atrium tomorrow morning and my inclination right now is to go back to the original suggestion in this thread and apply the patch to file_entity 2.13 that reverts the EFQ alter.

joseph.olstad’s picture

There was a reported issue with 7.x-2.14 for those that don't have the token module installed.
So I cut a new release that doesn't have the opengraph video thumbnail metatag support.

@mpotter, yes I see your point, I was going to revert however there's a lot of installations out there and they don't have a reported issue. I opted for the smallest code change possible under the time constraints. This was due to the fact that the related code has already been in the wild for over 7 months.

What I can do howeverr, is for the next release 7.x-2.16 I can remove the alter EntityFieldQuery support completely
but leave it in the 7.x-3.x branch , so that way if someone needs it, they can use the 7.x-3.x branch instead which has a release.

I'm totally in agreement with you on this. What we can do is remove this alter EntityFieldQuery for 7.x-2.16 and leave it in the 7.x-3.0-beta2 release.

The idea is to push the riskier stuff into 7.x-3.x , that is why I created a new branch.

Meanwhile, feel free to release openatrium using either file_entity 7.x-2.14 or 7.x-2.15 .

see the release notes of 7.x-2.14 and 7.x-2.15 basically 2.15 I removed video thumbnail open graph metatag support , it was in dev after 2.12 was released, we forgot to add a dependency for the token module , the video thumbnail functionality requires the token module, I reverted that so that there's more time to review, possibly just putting this functionality into the 3.x branch.

basically I'm planning on a feature freeze on the 7.x-2.x branch and put all new features into 7.x-3.x , right now the two branches are virtually the same.

would like to focus on stability for the 7.x-2.x branch.

I tested 7.x-2.15 with openatrium2 and it's good.

joseph.olstad’s picture

BTW: if you want to stay in alignment with the plan for 7.x-2.16
the revert patch still applies cleanly to 7.x-2.15

so, you can specify 2.15 for the build, add the revert patch
and you'll be close to what 2.16 will eventually look like.
https://www.drupal.org/files/issues/file_entity-REVERT_2067671-EntityFie...

meanwhile right now, openatrium will work with 2.15

joseph.olstad’s picture

m_potter, JeffM2001 and I have granted you both full maintainer role for file_entity.

feel free to publish a new release 2.16
I have created a plan for it. Feel free to edit it and discuss here:
#2935147: Plan for file_entity release 7.x-2.16

so yes, I apologize for the inconvenience
however we have a working solution right now. We have several working options.

mpotter’s picture

Status: Reviewed & tested by the community » Fixed

Should be fixed in the new 2.624 release. I updated to file_entity 2.15 and added the patch to revert the original issue as described in the issue description.

Status: Fixed » Closed (fixed)

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