_access tag can leading to fatal error

Facing this bug on the views data export displays on the 3.0.0-beta6 and the patch fixes the problem for us.

Feels like a major really.

Re-roll for the latest 3.x.

Tested on the latest 3.0.0-rc1 - issue persist.

And back to RTBC for greens!

If a query is tagged as an entity query, it needs to have an entity type set as its metadata.

If some modules (VBO, core_view_facets, ...) decide to maim a query by removing parts of the metadata while keeping others, that's their issue. This has nothing to do with "sane coding suggestions", as it's a perfectly valid expectation for there to be metadata present. If anything, it's good that Group doesn't check and crashes the page because now you know some module you're using is doing some really funky stuff.

This bug has been reported with VBO+Group and it turned out to be a bug in VBO, now it's reported with core_view_facets and voila: #3177272-5: core_views_facets module can crash in combination to other modules →

Not planning on "fixing" this as there is nothing to fix unless proven otherwise.

If a query is tagged as an entity query

this is not completely true, well not true at all. group adds 'entity_type' to metadata in group_views_query_alter() , together with 'views_entity_query' tag based on which group_query_views_entity_query_alter() the query is then sent to group_query_entity_query_alter(), but the problem is that \Drupal\views\Plugin\views\query\Sql::query() passes on tags to the queries generated with it, but it regenerates the query every time it's called. And queries generated by views doesn't have entity query tag on them.

Agree that original patch doesn't really addresses the issue, but there is definitely an issue here. I created 🐛 Query metadata get lost when $view->query->query() is called multiple times during page build Active get some feedback from the core on this subject.

Maybe the implementation of the group_views_query_alter() could be improved to pass around entity type somehow? In my case views_data_export calls $view->query->query()->countQuery()->execute(); to get total rows count and that doesn't look terribly wrong, but that call $view->query->query() generates a new SQL query and the entity type set in group_views_query_alter() is not preserved.

group_views_query_alter() has several checks to make sure it does not add the 'entity_type' metadata willy-nilly. When it does add it, it's because we're dealing with a View that we found represents a given entity type. At that point we do not tag the entity as an entity query, but we do use a custom tag to make sure we can invoke our logic that would otherwise run on an entity query.

core_views_facets, however, along with other modules that have so far broken on this piece of Group code, all either have an entity query or a View where they run custom code that strips important data from the query.

So again: There are two routes to group_query_entity_query_alter():

1. An entity query, or a query which pretends to be one - via core directly
2. A view that deals with an entity type or pretends to do so - via group_views_query_alter()

Both scenarios work, provided no-one stripped valuable metadata along the way.

So, respectfully, @RoSk0 I do not see where you find my previous statement not to be true at all. Group is right to expect the metadata to be set and, in case it's not, the problem always lies where the metadata is destroyed. If this happens to be in core, then it should be fixed as a bug in core.

Setting back to "Won't fix" as there is nothing I can do here and working around a bug in someone else's code (even if core) is something I'd rather not do. The right way ahead is to fix that bug elsewhere.