Referencing a different entity type is broken in the complex widget #163

idimopoulos · 2016-03-11T15:41:28Z

$this->fieldDefinition->getTargetEntityTypeId() returns the type of the entity that the field is attached to.

This is a logical mistake. Target type should be the group type.
Changed to $this->getFieldSetting('target_type') in order to obtaing the target type of the original widget field.
Similar to: #120

To reproduce, you can create an entity type other than node or user and make it a og group.
Then create a content type and make it a candidate member of that group.
Create some dummy group content.
Try to add content as an admin of og in order to get the "Other groups" field.
The functionality should be problematic because the target type of the "Other group" field is set to node (our content type) instead of the group entity type.
if you enter something and submit, the site also breaks.

…he entity that the field is attached to. This is a logical mistake. Target type should be the group type. Changed to $this->getFieldSetting('target_type') in order to obtaing the target type of the original widget field. Similar to: amitaibu#120

pfrenssen · 2016-03-11T17:00:56Z

Can you be a little bit more specific in the instructions how to reproduce the problem? When I try to replicate it I indeed hit a bug, but it is different than what you describe, and the fix doesn't solve it. A test case would be even better :)

Here's what I tried:

Do a clean install using the standard profile.
Enable og and og_ui.
Log in as UID1.
Go to admin/structure/block/block-content/manage/basic and check the option to make the "basic" custom block into a group.
Go to block/add and create a new block called "Group".
Go to admin/structure/types/manage/article and set the "article" node type to be group content, with target type "Custom block" and bundle "Basic block" (I specifically selected "Basic block" in the list, because leaving it empty has a known bug).
Go to admin/structure/types/manage/article/form-display and verify the group audience field is visible.
Go to node/add/article and start typing "Group" in the "Other groups" field.

Result: the AJAX call fails with the following error:

Uncaught PHP Exception InvalidArgumentException: "Placeholders must have a trailing [] if they are to be expanded with an array of values." at core/lib/Drupal/Core/Database/Connection.php line 719

From a first look this is due to my field config being incorrectly saved, I see this in field.field.node.article.og_group_ref:

settings:
  handler: 'default:node'
  handler_settings:
    target_type: block_content
    target_bundles: null
  access_override: false

That default:node handler is not correct I think. Also the target bundles are empty even though I specifically selected them.

pfrenssen · 2016-03-11T17:39:22Z

I manually fixed my field settings to the following:

     settings: [
       "handler" => "og:default",
       "handler_settings" => [
         "target_type" => "block_content",
         "target_bundles" => [
           "basic" => "basic",
         ],
       ],
     ],

But I still couldn't reference my Group custom block. I digged into OgComplex and there were a couple more instances where the wrong target type was retrieved. I fixed them in the commit above.

With these changes I can reference the custom blocks.

Two things to do:

We should create a followup ticket to fix the incorrect saving of the field handler.
Create a test for this. We can look at Port OgComplexWidgetTest #140, that already has done some work on testing OgComplex. We can probably lift some code from there.

pfrenssen · 2016-03-11T17:45:19Z

Fix the test. Seems like the field handler settings are saved differently in the test. The 'handler_settings' entry seems to be missing.

pfrenssen · 2016-03-16T20:58:03Z

There is still a bug here. The "Other Groups" field works now, but the "Groups Audience" field still doesn't work as expected.

pfrenssen · 2016-03-16T21:05:01Z

This is how $this->configuration looks in OgSelection::getSelectionHandler() when using the "Groups Audience" field. This looks like the settings that come directly from the field definition:

array (
  'target_type' => 'block_content',
  'handler' => 'og:default',
  'handler_settings' => 
  array (
    'target_type' => 'node',
    'target_bundles' => 
    array (
      'group' => 'group',
    ),
    'field_mode' => 'default',
  ),
)

This is how $this->configuration looks in OgSelection::getSelectionHandler() when using the "Other Groups" field. This looks like the settings that are generated in OgComplex::otherGroupsSingle():

array (
  'target_type' => 'node',
  'handler' => 'og:default',
  'handler_settings' => 
  array (
    'other_groups' => true,
    'field_mode' => 'admin',
  ),
)

We need to decide on either format.

amitaibu · 2016-03-16T21:29:26Z

The first block code looks like the correct one

array (
  'target_type' => 'block_content',
  'handler' => 'og:default',
  'handler_settings' => 
  array (
    'target_type' => 'node',
    'target_bundles' => 
    array (
      'group' => 'group',
    ),
    'field_mode' => 'default',
  ),
)

Further more I believe OgComplex::otherGroupsSingle is going to be soon removed by #161

(btw, just to set the common language -- a single (group audience) field has to "field modes" default and admin, or could be called "my groups" and "other groups")

pfrenssen · 2016-03-17T08:51:33Z

Ok let's wait until #161 is in.

pfrenssen · 2016-03-18T14:42:22Z

Going to continue on this. I'm currently blocked on #162 on the saving of target_bundles, and the solution I'm working on is causing the OgComplexTest to fail. I need some fixes in the OgComplex widget, meaning I need either this or #161 to be finished. I had a look at #161 but it still needs some work. This doesn't really interfere with that either it seems, the merge conflicts will be pretty minimal.

…eld definition.

pfrenssen · 2016-03-18T17:43:32Z

I think it's fixed now. Still needs a test.

…rget_type

pfrenssen · 2016-03-21T09:27:30Z

I started on the test by using the Custom Block entity type instead of Node to create the group. So far it seems to work fine for the Other Groups field. Still need to test the Groups Audience field as mentioned in one of my previous comments.

I am only working for two days this week because of some religious holiday, and I will have to work on other tasks for the moment. I will probably be able to pick this back up next Monday at the earliest.

pfrenssen · 2016-03-29T14:48:54Z

Picking this back up since it is blocking my work in #162

pfrenssen · 2016-03-29T16:35:17Z

I now have the strange situation that if I follow the steps from the first comment manually it doesn't work as expected, but when I run the test it does.

It appears that when the group audience field is added in the setup of the test the field storage settings are correct, but when I add it through the interface it is incorrectly set to "node" instead of "block_content".

I compared it with a vanilla entityreference field to make sure that my assumptions are correct.

This is the field storage config of the group audience field (field.storage.node.og_group_ref) as created through the UI:

langcode: en
status: true
dependencies:
  module:
    - node
    - og
id: node.og_group_ref
field_name: og_group_ref
entity_type: node
type: og_standard_reference
settings:
  target_type: node // <----- should be block_content
module: og
locked: false
cardinality: -1
translatable: true
indexes: {  }
persist_with_no_fields: false
custom_storage: false

Here is the config of the vanilla entity reference field I created to compare:

langcode: en
status: true
dependencies:
  module:
    - block_content
    - node
id: node.field_entityreference
field_name: field_entityreference
entity_type: node
type: entity_reference
settings:
  target_type: block_content // <---- here it is
module: core
locked: false
cardinality: 1
translatable: true
indexes: {  }
persist_with_no_fields: false
custom_storage: false

I also checked the field instance config, the target type is erroneously set here it seems. This is field.field.node.article.og_group_ref:

langcode: en
status: true
dependencies:
  config:
    - field.storage.node.og_group_ref
    - node.type.article
  module:
    - og
id: node.article.og_group_ref
field_name: og_group_ref
entity_type: node
bundle: article
label: 'Groups audience'
description: 'OG group audience reference field.'
required: false
translatable: true
default_value: {  }
default_value_callback: ''
settings:
  handler: 'og:default'
  handler_settings:
    target_type: block_content // <---- this is the target_type we are looking for
    target_bundles:
      basic: basic
  access_override: false
field_type: og_standard_reference

As reference, here is the vanilla entityreference equivalent:

langcode: en
status: true
dependencies:
  config:
    - block_content.type.basic
    - field.storage.node.field_entityreference
    - node.type.article
id: node.article.field_entityreference
field_name: field_entityreference
entity_type: node
bundle: article
label: entityreference
description: ''
required: false
translatable: false
default_value: {  }
default_value_callback: ''
settings:
  handler: 'default:block_content'
  handler_settings:
    target_bundles:  // <--- only target_bundle info here, no target_type
      basic: basic
    sort:
      field: _none
field_type: entity_reference

pfrenssen · 2016-03-29T16:58:45Z

Fixed it in the UI. We can also better add a test for this, to check that the fields are correctly saved when created through the UI.

So remaining work here:

Test the "Groups Audience" field.
Test that the target entity is correctly set for the Group Audience field when creating a group content type through the UI.

…oup content type.

pfrenssen · 2016-03-30T13:53:11Z

To prove that the tests correctly detect the bug I created a temporary branch that only contains the tests: https://github.com/amitaibu/og/tree/PR163-test-only

It is currently being tested at https://travis-ci.org/amitaibu/og/builds/119533962 - if all goes well these tests should fail.

pfrenssen · 2016-03-30T14:00:05Z

Great, both tests fail as expected in the PR163-test-only branch.

The first test fails with a 500 error which is due to uncaught InvalidArgumentException that I mentioned in my first comment:

Drupal\Tests\og\Functional\OgComplexWidgetTest::testFields with data set #1 ('Other groups', 'other_groups[0][target_id]')
Behat\Mink\Exception\ExpectationException: Current response status code is 500, but 200 expected.

The second one correctly fails since the target entity type is not set correctly:

Drupal\og_ui\Tests\BundleFormAlterTest::testCreate
The target type is set to the "Custom Block" entity type.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'block_content'
+'node'

That means that this is ready for review. If you want to test this manually you can follow the steps in the first comment after the issue summary.

amitaibu · 2016-03-31T16:49:28Z

Awesome! I'm finally going to have some OG time this coming week(s), so I plan to review this properly.

amitaibu · 2016-03-31T17:44:12Z

og_ui/og_ui.module

@@ -75,18 +76,20 @@ function og_ui_entity_type_save(EntityInterface $entity) {
  }

  // Change the field target type and bundle.
+  if ($field_storage = FieldStorageConfig::loadByName($entity_type_id, OgGroupAudienceHelper::DEFAULT_FIELD)) {
+    $target_type = $field_storage->getSetting('target_type');
+    if ($entity->og_target_type !== $target_type) {


ok, couldn't hold myself :)

where in the UI to you see the posibility to change the target type?

Oh, I see

I'll open a follow up issue

amitaibu · 2016-03-31T17:50:24Z

I've added a minor comment, but this is good to go. Thanks!

pfrenssen · 2016-03-31T19:10:02Z

Awesome, thanks!

Og membership state cache context

More target_type fixes in OgComplex.

db7683c

Fix failing test.

316e85b

pfrenssen pushed a commit that referenced this pull request Mar 16, 2016

Squashed #163

a019485

pfrenssen mentioned this pull request Mar 16, 2016

Delete memberships when a group is deleted #162

Merged

pfrenssen changed the title ~~Change wrong target_type in OgComplex.php~~ Referencing a different entity type is broken in the complex widget Mar 16, 2016

pfrenssen added 3 commits March 18, 2016 19:35

The target entity type is defined in the field storage.

030a930

Handler settings are defined on the field, not on the field storage.

58a7b55

Update test. Target types are defined in field storage, not in the fi…

17f19ac

…eld definition.

pfrenssen added 2 commits March 21, 2016 10:21

Merge remote-tracking branch 'origin/8.x-1.x' into OgComplexWidget-ta…

812af86

…rget_type

Add a test (WIP).

8d8f0b0

Merge branch '8.x-1.x' into OgComplexWidget-target_type

143d6f6

Correctly save the target type on field storage.

8607613

pfrenssen added 2 commits March 30, 2016 13:04

Test the Groups Audience field in addition to the Other Groups field.

beb5bec

Test that the group target type is correctly saved when creating a gr…

43fca11

…oup content type.

pfrenssen added the ready for review label Mar 30, 2016

amitaibu reviewed Mar 31, 2016
View reviewed changes

amitaibu merged commit a929694 into amitaibu:8.x-1.x Mar 31, 2016

amitaibu added a commit that referenced this pull request Sep 20, 2016

Merge pull request #163 from amitaibu/OgMembershipStatusesCacheContext

2774998

Og membership state cache context

pfrenssen deleted the OgComplexWidget-target_type branch September 22, 2017 09:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Referencing a different entity type is broken in the complex widget #163

Referencing a different entity type is broken in the complex widget #163

idimopoulos commented Mar 11, 2016

pfrenssen commented Mar 11, 2016

pfrenssen commented Mar 11, 2016

pfrenssen commented Mar 11, 2016

pfrenssen commented Mar 16, 2016

pfrenssen commented Mar 16, 2016

amitaibu commented Mar 16, 2016

pfrenssen commented Mar 17, 2016

pfrenssen commented Mar 18, 2016

pfrenssen commented Mar 18, 2016

pfrenssen commented Mar 21, 2016

pfrenssen commented Mar 29, 2016

pfrenssen commented Mar 29, 2016

pfrenssen commented Mar 29, 2016

pfrenssen commented Mar 30, 2016

pfrenssen commented Mar 30, 2016

amitaibu commented Mar 31, 2016

amitaibu Mar 31, 2016

amitaibu Mar 31, 2016

amitaibu commented Mar 31, 2016

pfrenssen commented Mar 31, 2016

Referencing a different entity type is broken in the complex widget #163

Referencing a different entity type is broken in the complex widget #163

Conversation

idimopoulos commented Mar 11, 2016

pfrenssen commented Mar 11, 2016

pfrenssen commented Mar 11, 2016

pfrenssen commented Mar 11, 2016

pfrenssen commented Mar 16, 2016

pfrenssen commented Mar 16, 2016

amitaibu commented Mar 16, 2016

pfrenssen commented Mar 17, 2016

pfrenssen commented Mar 18, 2016

pfrenssen commented Mar 18, 2016

pfrenssen commented Mar 21, 2016

pfrenssen commented Mar 29, 2016

pfrenssen commented Mar 29, 2016

pfrenssen commented Mar 29, 2016

pfrenssen commented Mar 30, 2016

pfrenssen commented Mar 30, 2016

amitaibu commented Mar 31, 2016

amitaibu Mar 31, 2016

Choose a reason for hiding this comment

amitaibu Mar 31, 2016

Choose a reason for hiding this comment

amitaibu commented Mar 31, 2016

pfrenssen commented Mar 31, 2016