There's currently no relationship in Views to allow a view of nodes to join on to their group.

I've made a start on this, however, I think before I go further there's a potential pitfall to consider.

The group_content table has an entity_id field for the ID of the entity that is a member of the group. However, if there are two different types of entity in the group with the same ID, say user 1 and node 1, the only way to differentiate between them is by the type field, which will be, say:

- mygrouptype-group_node-page
- mygrouptype-group_membership

This column is essential for the join in the Views relationship. However, this means that we have to define the Views relationship from node -> group content to be per group type. That seems like overkill, and it also is potentially restrictive, as it means you can't easily list nodes and groups where the groups are of different types.

It might be better to add an entity_type column to the group_content entity table. This is a little redundant, but it would make the Views relationships much simpler. (IIRC we've taken a similar approach with Flag module.)

CommentFileSizeAuthor
#56 group-relationship-better.jpg182.46 KBalan.upstone
#51 interdiff-51-44.txt18.98 KBkristiaanvandeneynde
#51 group-2706495-51.patch27.51 KBkristiaanvandeneynde
#50 group-relationship.jpeg273.42 KBalan.upstone
#49 group-relationship-used .jpeg201.07 KBalan.upstone
#48 core-patch.jpg374.36 KBalan.upstone
#44 interdiff-37-44.txt28.7 KBkristiaanvandeneynde
#44 group-2706495-44.patch30.52 KBkristiaanvandeneynde
#37 interdiff-37-34.txt2.69 KBkristiaanvandeneynde
#37 group-2706495-37.patch11.52 KBkristiaanvandeneynde
#36 memory exception.jpg113.86 KBalan.upstone
#35 views error.jpg180.86 KBalan.upstone
#35 views - apply.jpg204.56 KBalan.upstone
#34 group-reverse-views.png104.39 KBkristiaanvandeneynde
#34 interdiff-34-16.txt2.71 KBkristiaanvandeneynde
#34 group-2706495-34.patch11.08 KBkristiaanvandeneynde
#33 view relationships.jpg173.87 KBalan.upstone
#16 group-2706495-16.patch9.55 KBkristiaanvandeneynde
#8 2706495-8.group_.views-reverse-relationships.patch5.29 KBjoachim
#6 2706495-6.group_.views-reverse-relationships.patch5.26 KBjoachim
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

joachim’s picture

joachim CreditAttribution: joachim commented
Issue summary: View changes

Here's the code I've got so far:

function group_views_data_alter(array &$data) {
  // Relate entities to group content.
  $entity_types = \Drupal::entityManager()->getDefinitions();
  $group_types = GroupType::loadMultiple();
  
  // We have to create a relationship for each group type, rather than just one
  // relationship from entity type -> group content, because only the group type
  // column on the group_content table allows us to limit by entity type.
  foreach ($group_types as $group_type_id => $group_type) {
    $group_type_content_plugins = $group_type->getInstalledContentPlugins();
    foreach ($group_type_content_plugins as $plugin) {  
      $entity_type = $entity_types[$p->getEntityTypeId()];
      $base_table = $entity_type->getDataTable() ?: $entity_type->getBaseTable();
     
      $t_args = [
        '@entity_type' => $entity_type->getLabel(),
        // TODO: group type
      ];
      
      $data[$base_table]['group_content_' . $group_type_id] = array(
        'title' => t('@entity_type Group content', $t_args),
        'help' => t('Relate @entity_type to the Group Content that represents its membership of a Group.', $t_args),
        'relationship' => array(
          'group' => t('Group'),
          'base' => 'group_content',
          'base field' => 'entity_id',
          'relationship field' => $entity_type->getKey('id'),
          'id' => 'standard',
          'extra' => array(
            array(
              'field' => 'type',
              'value' => $plugin->getContentTypeConfigId(),
            ),
          ),
        ),
      );
    }
  }
}
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

So far I've always started from a GroupContent view and went from there (linking to the nodes). So I've not encountered this issue before, but it does make sense to have an easy way of linking nodes to their groups from a node view.

I was thinking of adding filter and relationship plugins that allow you to:

  • Filter a GroupContent view on entity_type, deducted from the plugin info. For instance: Node, User, ...
  • Filter a GroupContent view on plugin, regardless of which group type it's on. For instance: Group Node, Group Membership, ...
  • Link from a content entity to its GroupContent entity specifying the plugin: Group Node, Group Membership, ...
  • Link from a content entity to any GroupContent entity for it, regardless of plugin.

All of these are possible because we can query which GroupContentType entities could serve that content entity and then add an IN() clause to the query listing all of the valid GCTs.

What do you think?

joachim’s picture

joachim CreditAttribution: joachim commented

An IN clause could potentially get quite unwieldy if you have lots of group types for an entity type. I'd opt for redundant data.

joachim’s picture

joachim CreditAttribution: joachim commented
Assigned: Unassigned » joachim

Just had a look at Comment module, and that has an entity_type string field field on the comment entity table:

    $fields['entity_type'] = BaseFieldDefinition::create('string')
      ->setLabel(t('Entity type'))
      ->setDescription(t('The entity type to which this comment is attached.'))
      ->setSetting('is_ascii', TRUE)
      ->setSetting('max_length', EntityTypeInterface::ID_MAX_LENGTH);

Going to start on a patch for this.

joachim’s picture

joachim CreditAttribution: joachim at Torchbox commented
Status: Active » Needs review
FileSize
2706495-6.group_.views-reverse-relationships.patch5.26 KB

Here's a patch that adds:

- the entity_type field to Group Content entities
- hook_update to populate this field on existing entities (not strictly needed in alpha release, but my project needed it, and this module does have ~150 D8 installs...)
- views relationships

All working great for me :)

joachim’s picture

joachim CreditAttribution: joachim at Torchbox commented

Note from a colleague: the update in this patch will crash if there are orphaned Group Content entities. Those should get dealt with by #2693219: GroupContent: Delete all occurrences along with the entity.

joachim’s picture

joachim CreditAttribution: joachim at Torchbox commented
FileSize
2706495-8.group_.views-reverse-relationships.patch5.29 KB

Updated patch -- I missed off the line that executes the actual query to get the GC entities and they were all loading, which crashes on larger sites :/

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

I just pushed an update hook that removes all of the orphaned GroupContent entities. Looking into your patch now, but I'm not so big on redundant data. I'll investigate what other options we have and then get back to this.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

While reviewing this patch I uncovered a really poorly documented aspect of the Entity API that we should have used for Group and GroupContent entities. This patch is now blocked by that one, sadly :(

#2715011: Missing data_table for Group and GroupContent

joachim’s picture

joachim CreditAttribution: joachim at Torchbox commented

Regarding the redundant data, core's Comment module does the same thing for comment entities, so I feel we'd be in good company.
Back in the days of D7, we'd just link to the table for the Group Content bundles to go from Group Content type to the entity type, but now that bundles are config, we can't involve them in SQL queries any more. As I see it, having redundant fields on entity tables like this is just a cost of this change.

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

@joachim this patch completely replaces group.install. Is that what you planned? I find the resulting run of update.php crashes.

Updating
An unrecoverable error has occurred. You can find the error message below. It is advised to copy it to the clipboard for reference.
Please continue to the error page

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: /update.php/start?id=154&op=do_nojs&op=do
StatusText: OK
ResponseText: Fatal error: Allowed memory size of 268435456 bytes exhausted (tried to allocate 250609723 bytes) in /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/modules/contrib/group/src/Entity/GroupContent.php on line 18

joachim’s picture

joachim CreditAttribution: joachim at Torchbox commented
Status: Needs review » Needs work

> this patch completely replaces group.install

Must be that there wasn't a group.install file when I made the patch!

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Hey Alan, there wasn't a group.install file when joachim created this patch. I would also advise to not go ahead with this patch until #2715011: Missing data_table for Group and GroupContent lands as it will change the configuration of your views (different base tables). The other patch should land soon, I just need to find some time.

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

Thanks, Kristiaan; I'll wait. I see the issue list is growing substantially with requests for fixes and new features. It's your own fault for publishing a D8 alpha before OG!

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented
Status: Needs work » Needs review
FileSize
group-2706495-16.patch9.55 KB

Right, so here's a patch based off of today's dev release. It doesn't add a base field, but uses a rather intuitive relationship handler to do the heavy lifting. Please test. If this works out well, I'll add another relationship handler to go from GroupContent to entities with the same UI.

Note, I stumbled upon the very cryptic, yet annoying core bug with joins having an IN condition. This patch will only work if you patch core with #2658438-10: Array valued 'extra' values generate invalid SQL in Views joins.

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

I'll try this out ASAP.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Great, thanks!

P.S.: Did you manage to get your update errors fixed?

joachim’s picture

joachim CreditAttribution: joachim at Torchbox commented

A few remarks from an eyeball review, before I try it out:

  1. +++ b/src/Plugin/views/relationship/GroupContentToEntityReverse.php
@@ -0,0 +1,180 @@
+        '#description' => $this->t('Refine the result by plugin. Leave empty to fetch all results.'),

    You can't allow that, because you'll join to group content entities that are possibly not actually related.

    Eg, if your base table is node, and one row is node ID 42, then without the plugin condition, you'd end up joining to the group content entity for node 42 in group 1, but also the GC for user 42 in group 1. 

  2. +++ b/src/Plugin/views/relationship/GroupContentToEntityReverse.php
@@ -0,0 +1,180 @@
+   * We save the group content types this handler will filter on based off of
+   * which plugins were selected in the options form. This will allow us to
+   * easily filter the query on group content types without having to run an
+   * extra query or add a complicated join to the view query.
+   *
+   * {@inheritdoc}

    @inherit doc has to be the only thing in the docblock.

Also, the views data code should be in hook_views_data_alter() rather than the Group Content views data handler, because it is adding to tables from other entity types.

joachim’s picture

joachim CreditAttribution: joachim at Torchbox commented

> You can't allow that, because you'll join to group content entities that are possibly not actually related.

Ah right, I see, you're handling that:

    // Set default values that mimic those submitted by checkboxes to set which
    // group content types we're actually filtering on. This defaults to all of
    // the ones that could be used by the entity type set on the handler.
    $group_content_type_ids = array_keys(GroupContentType::loadByEntityTypeId($this->definition['entity_type']));
    $options['group_content_types']['default'] = array_combine($group_content_type_ids, $group_content_type_ids);

    // Create an array containing all options as unselected so we can add it to
    // the array containing the selected options. This will result in an array
    // which only has the actually selected options with non-zero values.
    $unselected = array_fill_keys($this->options['group_content_types'], 0);

    // Save the options as if they were coming from a checkboxes element.
    $options['group_content_types'] = $selected + $unselected;
    $form_state->setValue('options', $options);

This seems really convoluted. There's a group_content_plugins option, but the one that's actually used in the query is group_content_types, which is hidden in the UI.

Also, the filtering by plugin in the relationship options seems to me to be unnecessary, as it's just reproducing the bundle filter. From the POV of a site builder, it's violating DRY.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Thanks for the review.

Your first point is actually taken care of. It only loads the group content types that can serve the entity type the relationship uses. I think I've seen doxygen blocks in core that have both text and inheritdoc in there. I'll have another look and move it into the function.

Your final remark is also valid, even though I'm not a fan of procedural code in a D8 module. I'll check the API and if it does recommend adding it in an alter, we can move it.

joachim’s picture

joachim CreditAttribution: joachim at Torchbox commented

> I think I've seen doxygen blocks in core that have both text and inheritdoc in there.

Indeed, though they shouldn't be there. See #1994890: Allow {@inheritdoc} and additional documentation for the discussion about it. Basically, API module doesn't and can't support it.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Also, the filtering by plugin in the relationship options seems to me to be unnecessary, as it's just reproducing the bundle filter. From the POV of a site builder, it's violating DRY.

At first it looks like that, but when you realize that multiple plugins could serve the same entity type, then it makes sense. For instance: group_membership serves users as members, but a sandbox module I know of (guser) serves users as entities a group manager has edit access over.

So ideally, you want to be able to filter the relationship per plugin or even per group content type, but seeing as that's too much of a pain, we allow you to filter on the plugin and then manually populate the group content types in the query for you.

The only thing I just realized is that the cached results won't do if someone enables a plugin on a group type after the view has been saved. So we need to retrieve the group content types "live" (performance hit) or add in an extra join to the group content types, but seeing as they are configuration entities I don't think that's possible.

Perhaps we can have a cached mapper that maps plugin IDs to group content types and update that cache whenever a plugin gets enabled? If we call that from the views handler, it would reduce the performance hit significantly.

Edit: I am still inclined to commit this as an alpha thing because at least that way we have a working handler in Group and it would remain working if we optimize the underlying logic in beta (i.e.: the cached group content type ID map needs to be moved to a service).

joachim’s picture

joachim CreditAttribution: joachim at Torchbox commented

> For instance: group_membership serves users as members, but a sandbox module I know of (guser) serves users as entities a group manager has edit access over.

Ah, right. I hadn't thought of that.

> So we need to retrieve the group content types "live" (performance hit)

You could cache a lookup array when the view is saved, and clear that cache whenever group content configuration is changed.
EDIT: no, cache the lookup whenever group content configuration is saved. The view relationship handler would just consume the cache data, not store it.

> or add in an extra join to the group content types, but seeing as they are configuration entities I don't think that's possible.

... hence the need for an entity_type field on the content entity, like comment module has. It's necessary because we can no longer do query joins to configuration entities like we used to pre-D8.

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

Well, I tried this out.
The core patch updated OK except for the test. The patch was trying to update core/modules/views/src/Tests/Plugin/JoinTest.php, which I don't have in 8.1.1 core.
The patch at #16 creates WSOD. I have reverted and re-committed a few times and this seems to toggle the WSOD.
I can see in New Relic that index.php runs v slow but I think I am out of knowledge now,

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Could you turn on error reporting in Drupal and tell us what the WSOD says?

Also, when does the WSOD happen? When rendering a view which uses the plugins from the patch?

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

I have turned on error reporting but it does not seem to get that far. Just WSOD on any page, including home. Is there a place in the base I that might store errors?

On the server, I see:

php-fpm-error.log

098ef2e93dbe42319a5e0c2aa08cfc1a/code//index.php' (request: "GET /index.php") executing too slow (5.483428 sec), logging
[23-May-2016 15:31:20] NOTICE: child 25496 stopped for tracing
[23-May-2016 15:31:20] NOTICE: about to trace 25496
[23-May-2016 15:31:20] NOTICE: finished trace of 25496

php-slow.log

[23-May-2016 15:31:20] [pool www] pid 25496
script_filename = /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code//index.php
[0x00007f6449c9cf60] createFromRenderArray() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/core/lib/Drupal/Core/Render/BubbleableMetadata.php:65
[0x00007f6449c9ce40] createFromRenderArray() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/core/lib/Drupal/Core/Render/RenderContext.php:31
[0x00007f6449c9cbb0] update() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/core/lib/Drupal/Core/Render/Renderer.php:517
[0x00007f6449c9ca28] doRender() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/core/lib/Drupal/Core/Render/Renderer.php:195
[0x00007f6449c9c910] render() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/core/includes/common.inc:871
[0x00007f6449c9c650] drupal_render() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/core/lib/Drupal/Core/Theme/ThemeManager.php:306
[0x00007f6449c9c378] render() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/core/lib/Drupal/Core/Render/Renderer.php:491
[0x00007f6449c9c1f0] doRender() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/core/lib/Drupal/Core/Render/Renderer.php:195
[0x00007f6449c9c080] render() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/core/lib/Drupal/Core/Template/TwigExtension.php:462
[0x00007f6449c9beb0] escapeFilter() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/files/php/twig/ffab12a8_page.html.twig_8e4684e780ccaef71c039b616824501d65fa0d24eccc583e5457933f88f236f8/0ca57761b92795d58cd984c212845146a7672b71a5efff609595857ac97723c3.php:60
[0x00007f6449c9bd68] doDisplay() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/vendor/twig/twig/lib/Twig/Template.php:381
[0x00007f6449c9bc30] displayWithErrorHandling() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/vendor/twig/twig/lib/Twig/Template.php:355
[0x00007f6449c9bb30] display() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/vendor/twig/twig/lib/Twig/Template.php:366
[0x00007f6449c9b920] render() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/core/themes/engines/twig/twig.engine:64
[0x00007f6449c9b598] twig_render_template() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/core/lib/Drupal/Core/Theme/ThemeManager.php:384
[0x00007f6449c9b2c0] render() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/core/lib/Drupal/Core/Render/Renderer.php:435
[0x00007f6449c9b138] doRender() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/core/lib/Drupal/Core/Render/Renderer.php:195
[0x00007f6449c9afc8] render() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/core/lib/Drupal/Core/Template/TwigExtension.php:462
[0x00007f6449c9adf8] escapeFilter() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/files/php/twig/ffab12a8_html.html.twig_6748c0119b14a320949c097fbbd9ddeaa8fc04ac59619e0ca3a326d125ebabc8/20dc9a43e7714f1bb638cf1d82597db66464c8da9769557ef3d69f06e7dd81f3.php:88
[0x00007f6449c9ac70] doDisplay() /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/vendor/twig/twig/lib/Twig/Template.php:381

newrelic.log
2016-05-23 15:33:24.815 (3374) info: New Relic 4.9.0 (build 54 - "trex" - "4b297ef65da097e3976e9f36a305aa3919410f29") [daemon=3145 php='5.5.24' zts=no sapi='cli' pid=3374 ppid=3373 uid=12276 euid=12276 gid=12276 egid=12276 backtrace=yes startup=init os='Linux' rel='4.3.4-200.fc22.x86_64' mach='x86_64' ver='#1 SMP Mon Jan 25 13' node='endpoint6c226846.chios.panth.io']
2016-05-23 15:33:25.494 (3419) info: New Relic 4.9.0 (build 54 - "trex" - "4b297ef65da097e3976e9f36a305aa3919410f29") [daemon=3145 php='5.5.24' zts=no sapi='cli' pid=3419 ppid=3418 uid=12276 euid=12276 gid=12276 egid=12276 backtrace=yes startup=init os='Linux' rel='4.3.4-200.fc22.x86_64' mach='x86_64' ver='#1 SMP Mon Jan 25 13' node='endpoint6c226846.chios.panth.io']
2016-05-23 15:33:27.541 (3493) info: New Relic 4.9.0 (build 54 - "trex" - "4b297ef65da097e3976e9f36a305aa3919410f29") [daemon=3145 php='5.5.24' zts=no sapi='cli' pid=3493 ppid=3491 uid=12276 euid=12276 gid=12276 egid=12276 backtrace=yes startup=init os='Linux' rel='4.3.4-200.fc22.x86_64' mach='x86_64' ver='#1 SMP Mon Jan 25 13' node='endpoint6c226846.chios.panth.io']
2016-05-23 15:33:28.254 (3555) info: New Relic 4.9.0 (build 54 - "trex" - "4b297ef65da097e3976e9f36a305aa3919410f29") [daemon=3145 php='5.5.24' zts=no sapi='cli' pid=3555 ppid=3554 uid=12276 euid=12276 gid=12276 egid=12276 backtrace=yes startup=init os='Linux' rel='4.3.4-200.fc22.x86_64' mach='x86_64' ver='#1 SMP Mon Jan 25 13' node='endpoint6c226846.chios.panth.io']

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Unless you have a view on every page, this patch should not be the culprit. The code contained inside of it should only run when working with Views.

Perhaps the core patch is causing issues, although that one should have only affected rare JOIN queries. Your site is working fine without this patch and the core patch?

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

Yes, and fine with the core patch too.

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

Yes, and fine with the core patch too.

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

I think I have got this working by reverting some other group patches and turning themes off and on a few times (more art than science). The reverse plugin is in the codebase, but what does it do?

I was hoping it would allow me to add a relationship, say, from an article view to the group content entity which has that article's id in the "content to add" database column - to get a view that tells me the group for an article. In /admin/structure/views, I mean. What am I missing?

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

With this patch you can get from any Node/User/etc. view to a GroupContent Entity using a relationship. You can optionally filter that relationship by group content plugin.

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented
FileSize
view relationships.jpg173.87 KB

Kristiaan
I'd be grateful for a screen grab or two showing this. I am creating views of nodes or users and trying to add a relationship as in my screen grab. But I am not offered anything to do with groups.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented
FileSize
group-2706495-34.patch11.08 KB
interdiff-34-16.txt2.71 KB
group-reverse-views.png104.39 KB

Patch needed a reroll. Note that this still doesn't address the caching issues mentioned above. I need a bit more time for that. Also: make sure you have the core patch and that you clear your caches after applying this one.

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented
FileSize
views - apply.jpg204.56 KB
views error.jpg180.86 KB

Thanks, Kristiaan
That's working better. I can now find the relationship, but clicking the "Add and configure relationships" button to confirm it does not work. If I close the dialogue using the X, the relationship is listed, but it can't be configured. The fields from the group_content entity are available for adding to the view display.

This is the error report:

Type php
Date Tuesday, May 24, 2016 - 13:57
User Alan Upstone
Location http://dev-mobilized.pantheonsite.io/admin/structure/views/view/groups_f...
Referrer http://dev-mobilized.pantheonsite.io/admin/structure/views/view/groups_f...
Message Warning: Illegal offset type in isset or empty in views_query_views_alter() (line 663 of /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/core/modules/views/views.module).
Severity Warning

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented
FileSize
memory exception.jpg113.86 KB

Here's the memory exception

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented
FileSize
group-2706495-37.patch11.52 KB
interdiff-37-34.txt2.69 KB

Still had some alpha3 code in there, try now. Same conditions apply as the patch in #34

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

That's stopped the block on saving the relationship, but I still get errors...

Summary

Type Date Sort ascending Message User Operations
php 05/24/2016 - 17:13 Warning: Illegal offset type in isset or empty in views… Alan Upstone
php 05/24/2016 - 17:13 Warning: Illegal offset type in isset or empty in views… Alan Upstone
php 05/24/2016 - 17:13 Warning: Illegal offset type in isset or empty in views… Alan Upstone
php 05/24/2016 - 17:13 Warning: Illegal offset type in isset or empty in views… Alan Upstone
php 05/24/2016 - 17:13 Warning: array_filter() expects parameter 1 to be array… Alan Upstone
php 05/24/2016 - 17:13 Notice: Undefined index: group_content_plugins in… Alan Upstone
php 05/24/2016 - 17:13 Warning: Illegal offset type in isset or empty in views… Alan Upstone
php 05/24/2016 - 17:13 Warning: Illegal offset type in isset or empty in views… Alan Upstone

Details

Type php
Date Tuesday, May 24, 2016 - 17:13
User Alan Upstone
Location http://dev-mobilized.pantheonsite.io/admin/structure/views/ajax/handler/...
Referrer http://dev-mobilized.pantheonsite.io/admin/structure/views/view/user_sum...
Message Notice: Undefined index: group_content_plugins in Drupal\group\Plugin\views\relationship\GroupContentToEntityReverse->validateOptionsForm() (line 123 of /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/modules/contrib/group/src/Plugin/views/relationship/GroupContentToEntityReverse.php).
Severity Notice

Type php
Date Tuesday, May 24, 2016 - 17:13
User Alan Upstone
Location http://dev-mobilized.pantheonsite.io/admin/structure/views/ajax/handler/...
Referrer http://dev-mobilized.pantheonsite.io/admin/structure/views/view/user_sum...
Message Warning: array_filter() expects parameter 1 to be array, null given in Drupal\group\Plugin\views\relationship\GroupContentToEntityReverse->validateOptionsForm() (line 123 of /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/modules/contrib/group/src/Plugin/views/relationship/GroupContentToEntityReverse.php).
Severity Warning
Hostname 81.107.208.35

Type php
Date Tuesday, May 24, 2016 - 17:13
User Alan Upstone
Location http://dev-mobilized.pantheonsite.io/admin/structure/views/view/user_sum...
Referrer http://dev-mobilized.pantheonsite.io/admin/structure/views/view/user_sum...
Message Warning: Illegal offset type in isset or empty in views_query_views_alter() (line 663 of /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/core/modules/views/views.module).

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

I'm not properly checking for $options['group_content_plugins'] as it may not be set. Good catch. I'll roll an updated patch on the train to London tomorrow.

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

Any luck with this, Kristiaan? Is that train delayed? :)

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Yeah I had some other critical Group issues while I was over here :)

I did manage to reroll the other patch for core 8.1, though. Try out the latest copy in #2658438: Array valued 'extra' values generate invalid SQL in Views joins

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

Hello Kristiaan
While I am waiting for #39, is there anything I can do to make sure $options['group_content_plugins'] is always set, so I can avoid the bug?
I need to go live.
Thanks

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Hi Alan, I should be able to work on Group 8 tomorrow. I'll post an updated patch then.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented
FileSize
group-2706495-44.patch30.52 KB
interdiff-37-44.txt28.7 KB

Right so here's the finished patch, it:

  • Caches the plugin info the handler needs on the plugin manager
  • Adds a relationship from every content entity type that is handled by a plugin to group content entities
  • Adds a relationship from group content entities to every content entity type that is handled by a plugin
  • Removes the original group content to content entity relationship as it was much weaker than the above

If you had any GroupContent -> Entity relationships, those need to be replaced with the new ones. This still needs the patch from #2658438: Array valued 'extra' values generate invalid SQL in Views joins

P.S.: The interdiff is huge, feel free to ignore it.

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

Fab. I will try this today and report back.

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

I have not managed to get this working yet. I get:

Type php
Date Friday, June 3, 2016 - 17:31
User Alan Upstone
Location http://xxxxx/admin/structure/views/view/topics_by_group/preview/block_1?...
Referrer http://xxxxx/admin/structure/views/view/topics_by_group
Message InvalidArgumentException: Placeholders must have a trailing [] if they are to be expanded with an array of values. in Drupal\Core\Database\Connection->expandArguments() (line 714 of /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/core/lib/Drupal/Core/Database/Connection.php).
Severity Error

and

Type php
Date Friday, June 3, 2016 - 17:31
User Alan Upstone
Location http://xxxxx/admin/structure/views/view/topics_by_group/preview/block_1?...
Referrer http://xxxxx/admin/structure/views/view/topics_by_group
Message Warning: Illegal offset type in isset or empty in views_query_views_alter() (line 663 of /srv/bindings/098ef2e93dbe42319a5e0c2aa08cfc1a/code/core/modules/views/views.module).
Severity Warning

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Have you patched core with the other issue's patch?

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented
FileSize
core-patch.jpg374.36 KB

Yes - please see attached.

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented
FileSize
group-relationship-used .jpeg201.07 KB
alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented
FileSize
group-relationship.jpeg273.42 KB
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented
FileSize
group-2706495-51.patch27.51 KB
interdiff-51-44.txt18.98 KB

Seems like the latest patch in the other issue was broken, use issue-2658438-bad-join-sql-19.patch instead along with the cleaned up patch attached to this comment.

Make sure you re-save any relationships you had so that they are properly updated.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

I also need to add config schemas for the 2 relationship handlers introduced here, but that's an easy fix.

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

Thanks for looking at this on your weekend, Kristiaan!
Do I try it now to do I need to wait for the config schemas?

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

You can try it now, the config schema isn't that critical.

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

Will do!

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented
FileSize
group-relationship-better.jpg182.46 KB

Same errors as at #46, I'm afraid. Using the -51.patch here plus issue-2658438-bad-join-sql-19.patch as instructed.
There is one difference, though.
The image group-relationship at #50 shows how I was offered all content types, even those for which I had not enabled any group content plugins.
The image attached here, group-relationship-better, shows how I am now offered only those content types for which I have enabled group content plugins, which seems better to me.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Hi Alan,

I've tested the combination of this patch and patch 19 from the other issue and it works fine here. Keep in mind that when patching core twice the latter patch may not have applied at all. Can you verify that the JoinPluginBase class has the following code?

$use_parenthesis = FALSE;
if (is_array($info['value'])) {
  // We use an SA-CORE-2014-005 conformant placeholder for our array
  // of values. Also, note that the 'IN' operator is implicit.
  // @see https://www.drupal.org/node/2401615.
  $placeholder = ':views_join_condition_' . $select_query->nextPlaceholder() . '[]';
  $operator = !empty($info['operator']) ? $info['operator'] : 'IN';
  $arguments[$placeholder] = $info['value'];
  $use_parenthesis = TRUE;
}
else {
  // With a single value, the '=' operator is implicit.
  $operator = !empty($info['operator']) ? $info['operator'] : '=';
  $placeholder = ':views_join_condition_' . $select_query->nextPlaceholder();
}
// Set 'field' as join table field if available or set 'left field' as
// join table field is not set.
if (isset($info['field'])) {
  $join_table_field = "$join_table$info[field]";
  // Allow the value to be set either with the 'value' element or
  // with 'left_field'.
  if (isset($info['left_field'])) {
    $placeholder = "$left[alias].$info[left_field]";
  }
  else {
    $arguments[$placeholder] = $info['value'];
  }
}
alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

Well, I have been resetting the views script to core (8.1.2) and your June 4th build of group then applying the patches, to avoid any reverse patch malarkey.
However, the code I end up with in JoinPluginBase looks different to what you posted in #57, e.g. I have:

if (is_array($info['value'])) {
// We use an SA-CORE-2014-005 conformant placeholder for our array
// of values. Also, note that the 'IN' operator is implicit.
// @see https://www.drupal.org/node/2401615.
$placeholder = '( :views_join_condition_' . $select_query->nextPlaceholder() . '[] )';
$operator = !empty($info['operator']) ? $info['operator'] : 'IN';
$arguments[$placeholder] = $info['value'];
}
else {
// With a single value, the '=' operator is implicit.
$operator = !empty($info['operator']) ? $info['operator'] : '=';
$placeholder = ':views_join_condition_' . $select_query->nextPlaceholder();
}
// Set 'field' as join table field if available or set 'left field' as
// join table field is not set.
if (isset($info['field'])) {
$join_table_field = "$join_table$info[field]";
// Allow the value to be set either with the 'value' element or
// with 'left_field'.
if (isset($info['left_field'])) {
$placeholder = "$left[alias].$info[left_field]";
}
else {
$arguments[$placeholder] = $info['value'];
}
}

i.e. no "$use_parenthesis = FALSE;'' inserted before the IF.

That suggests I am doing something wrong with my patching. I'll investigate and report back.

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

For the craic, I pasted code from #57 in directly instead of patching. I am not sure the things in {} are OK in the generated SQL! I will try the patches again on a fresh install and report back.

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near ''group_content_type_d07c0dea387bb', 'mlab-group_node-app_toc_topics', 'mw-group_' at line 1: SELECT node_field_data.langcode AS node_field_data_langcode, group_content_field_data_node_field_data.langcode AS group_content_field_data_node_field_data_langcode, node_field_data.created AS node_field_data_created, node_field_data.nid AS nid, group_content_field_data_node_field_data.id AS group_content_field_data_node_field_data_id FROM {node_field_data} node_field_data LEFT JOIN {group_content_field_data} group_content_field_data_node_field_data ON node_field_data.nid = group_content_field_data_node_field_data.entity_id AND group_content_field_data_node_field_data.type IN :views_join_condition_0__0, :views_join_condition_0__1, :views_join_condition_0__2, :views_join_condition_0__3 WHERE (( (node_field_data.status = :db_condition_placeholder_1) AND (node_field_data.type IN (:db_condition_placeholder_2)) )) ORDER BY node_field_data_created DESC; Array ( [:db_condition_placeholder_1] => 1 [:db_condition_placeholder_2] => app_toc_topics [:views_join_condition_0__0] => group_content_type_d07c0dea387bb [:views_join_condition_0__1] => mlab-group_node-app_toc_topics [:views_join_condition_0__2] => mw-group_node-app_toc_topics [:views_join_condition_0__3] => rbsip-group_node-app_toc_topics )

SELECT node_field_data.langcode AS node_field_data_langcode, group_content_field_data_node_field_data.langcode AS group_content_field_data_node_field_data_langcode, node_field_data.created AS node_field_data_created, node_field_data.nid AS nid, group_content_field_data_node_field_data.id AS group_content_field_data_node_field_data_id
FROM
{node_field_data} node_field_data
LEFT JOIN {group_content_field_data} group_content_field_data_node_field_data ON node_field_data.nid = group_content_field_data_node_field_data.entity_id AND group_content_field_data_node_field_data.type IN 'group_content_type_d07c0dea387bb', 'mlab-group_node-app_toc_topics', 'mw-group_node-app_toc_topics', 'rbsip-group_node-app_toc_topics'
WHERE (( (node_field_data.status = '1') AND (node_field_data.type IN ('app_toc_topics')) ))
ORDER BY node_field_data_created DESC

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented 
$placeholder = '( :views_join_condition_' . $select_query->nextPlaceholder() . '[] )';

That part tells me you have the old core patch still in place.

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

The join is now working and the view shows what I would expect, so it is all 'working' now, although I still get this warning:

Type php
Date Sunday, June 5, 2016 - 19:01
User Alan Upstone
Location http://xxxxx/admin/structure/views/view/topic_by_group/preview/block_1?_...
Referrer http://xxxxx/admin/structure/views/view/topic_by_group
Message Warning: Illegal offset type in isset or empty in views_query_views_alter() (line 663 of /srv/bindings/xxxxxxx/code/core/modules/views/views.module).
Severity Warning
____________________________________

Here's what I patched in the Group module with 51.patch:, based on the 4th June dev build.

iMac:group aupstone$ curl https://www.drupal.org/files/issues/group-2706495-51.patch | patch -p1
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 28175 100 28175 0 0 31751 0 --:--:-- --:--:-- --:--:-- 31728
patching file group.views.inc
patching file src/Entity/GroupContentType.php
patching file src/Entity/Views/GroupContentViewsData.php
patching file src/Plugin/GroupContentEnablerManager.php
patching file src/Plugin/GroupContentEnablerManagerInterface.php
patching file src/Plugin/views/relationship/GroupContentToEntity.php
patching file src/Plugin/views/relationship/GroupContentToEntityBase.php
patching file src/Plugin/views/relationship/GroupContentToEntityReverse.php
iMac:group aupstone$
_____________

Here's JoinPluginBase.php in Views after running the 19-patch:
(removed to reduce post length)

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented
Assigned: joachim » Unassigned

Hi Alan,

The warning you see is caused by the patch from the other issue but can be safely be ignored. As far as this issue is concerned, it can be committed and marked as fixed as soon as it's reviewed by someone else.

P.S.: Please don't post entire files or logs in the comments, it makes the thread really long to read :) If possible, put it in a .txt file and attach it to your comment. Snippets are fine, though.

alan.upstone’s picture

alan.upstone CreditAttribution: alan.upstone commented

Good point about the long post; I've edited the source code quote down since it's not relevant anyway. Thanks for your support on this, Kristiaan. And thanks to you and Joachim for this very useful enhancement.

Over to someone else to test too.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented
Status: Needs review » Fixed

Right, we need to move forward and this has been tested by both me and Alan. The resulting queries are looking sane, so committing this and adding a mention to the release notes about having to patch core.

Status: Fixed » Closed (fixed)

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

The last submitted patch, 6: 2706495-6.group_.views-reverse-relationships.patch, failed testing.

The last submitted patch, 8: 2706495-8.group_.views-reverse-relationships.patch, failed testing.

The last submitted patch, 16: group-2706495-16.patch, failed testing.

The last submitted patch, 34: group-2706495-34.patch, failed testing.

The last submitted patch, 37: group-2706495-37.patch, failed testing.

The last submitted patch, 44: group-2706495-44.patch, failed testing.

Status: Closed (fixed) » Needs work

The last submitted patch, 51: group-2706495-51.patch, failed testing.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented
Status: Needs work » Closed (fixed)

Bad testbot! :)