More robust query altering for line items

There is no option for opting in/out of public listings, #1673000: Define Required and Optional Data for User Profiles would add this to COD. We are proceeding without this.

(Also #1672922: Recommended Changes to Attendee List (/community))

Would be great to resolve this in the next week as we're going to begin heavy promotion for attendees and want to use this page to generate buzz.
This is an issue for Sydney.
This view needs to be added to Portland so we can test for this there as well.

The anonymous query actually errors out. The cause is an extra INNER JOIN {commerce_order} co ON co.order_id = users.order_id. I'm still investigating this.

I traced this back to a bug in commerce line item. For non-admin users, this view's query gets to commerce_line_item_query_commerce_line_item_access_alter() for access checking. Our view's first table is users, which doesn't have an order_id column, causing

$order_alias = $query->innerJoin('commerce_order', 'co', '%alias.order_id = ' . $base_table . '.order_id');

to add an invalid join.

Our query was already joined with commerce_order, so this patch first searches for that table and uses the first existing alias it can find. If it does not find one, it falls back to the current behavior.

The table search moved the array's internal pointer, causing key() to return the last key instead of the first.

I'm not sure that

$tables = &$query->getTables();

Guarantees that the internal pointer is at the first table or not. If not, this patch also makes that case a bit more robust.

That internal array pointer is shared everywhere the tables array is used in that way, and caused the same problem in commerce_entity_access_query_alter().

I faced this issue for Commerce file license module.

function commerce_line_item_query_commerce_line_item_access_alter() adds this bad join:

INNER JOIN commerce_order co ON co.order_id = commerce_file_license.order_id

Pacufist - any luck with using the patch in #12 to fix this?

Looked this over and decided to extend the same treatment to the payment transaction query alter as well. However, I'm actually a bit stumped about how you guys end up with this join being made in the first place.

As best I can tell, commerce_line_item_query_commerce_line_item_access_alter() is never being called in my install. This is because we actually don't define an access tag in the entity info for line items (or payment transactions), relying instead on the entity type specific access callbacks (e.g. commerce_line_item_access()) that use the fallback on the order access.

Is there some module you're using that adds in the access tag that commerce_entity_access() is looking for before creating and tagging the query? If not, can I get a backtrace showing how the function was actually called?

Attached a patch for review that includes the payment transaction fix and that tweaks the reset() usage just a little bit to pass strict reporting (the function expects an array by reference, which we can't send as the return value of array_keys()).

Ahh, wait, I see. It's coming in through Views. Testing now.

Alrighty, one more tweak. In my View I was using to test query altering, I noticed that I was getting an extra condition on commerce_line_item.uid - a column that doesn't exist but that was being added because commerce_order_query_commerce_order_access_alter() was being invoked when the query was being altered. I haven't sorted through exactly how or why that was happening (is line item access altering itself to pass through to order access somehow resulting in the order access tag being applied? or is order access happening separately through some Views magic?).

In any event, when that function got invoked, it was repeating the same query access altering that line item already did to the query, and in so doing was resorting to using the first table as the base table - i.e. commerce_line_item. We even say in commerce_entity_access_query_alter() that a caller must ensure it isn't running an alter for an entity type whose base table isn't the first table without explicitly supplying the base table alias:

    // Assume that the base table is the first table if not set. It will result
    // in an invalid query if the first table is not the table we expect,
    // forcing the caller to actually properly pass a base table in that case.

So I copied over the logic to loop over tables and find a matching base table. If commerce_order_query_commerce_order_access_alter() can't find a commerce_order table, it'll just do nothing.

Thanks for kicking this off, Neil. : )

Commit: http://drupalcode.org/project/commerce.git/commitdiff/db61d45

I started tracing through the access tags and query alteration, but don't remember how exactly commerce_line_item_query_commerce_line_item_access_alter () got called. I think I may have switched to searching for that join without fully understanding.

The commit looks okay. I noticed the same code pattern, so I'm not surprised the commerce_order and commerce_payment changes were needed too. key() is used in a few other places, but those haven't caused trouble that I know of yet.