Problem/Motivation

Right now in \Drupal\dynamic_entity_reference\Plugin\Field\FieldType\DynamicEntityReferenceItem::fieldSettingsForm() we are doing nothing.
I want to make it more similar to \Drupal\entity_reference\ConfigurableEntityReferenceItem::fieldSettingsForm()

Proposed resolution

Add DynamicEntityReferenceSelection plugin just like EntityReferenceSelection and add selection plugins for all core content entities. We are just reusing core Selection plugins.

Remaining tasks

  • Discuss it.
  • Finalize it.
  • Implement it..
  • Review it.
  • Add more tests.
  • Commit it.

User interface changes

User will have the ability to choose the selection widget for field settings.

API changes

API Addition only.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

hi @jibran - I'm happy with the existing selector - but if you want to add the notion of selection plugins that's fine with me.

jibran’s picture

Well I talked with @berdir about this and he said it is a security issue because our selection is not ensuring entity access so user might be able to select un-publish nodes. So we have to fix that one way or an other an we both think selection plugin is the best way forward. Thoughts!!!

larowlan’s picture

From DynamicEntityReferenceController::buildEntityQuery?

    // Add entity-access tag.
    $query->addTag($target_type . '_access');

But either way, go for it :)

Berdir’s picture

Yes, that's the theory. See all the hacks in the different entity reference selection plugins why that is not enough.

jibran’s picture

Status: Active » Needs review
FileSize
1.42 KB
184.44 KB

How about reusing the ER selection plugin manager? That way we don't have to copy all the ER code.
This is work in progress. Posting here for opinion.
@todo
Update schema so that we can store the settings.
Update DynamicEntityReferenceWidget accordingly.
This is the field setting page.

Status: Needs review » Needs work

The last submitted patch, 5: 2346273-5.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
2.81 KB
2.64 KB

Here is some more progress.
@todo Update DynamicEntityReferenceWidget accordingly.
@todo FIx/Write tests.

Status: Needs review » Needs work

The last submitted patch, 7: field_settings_form-2346273-6.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
10.9 KB
11 KB

I think it is ready for review. Only remaining thing is to write/fix tests.
And I think I fixed #2366093: Unable to set field value programmatically. while working on this. I have to check it first.

Status: Needs review » Needs work

The last submitted patch, 9: field_settings_form-2346273-9.patch, failed testing.

larowlan’s picture

Assigned: Unassigned » larowlan
jibran’s picture

Status: Needs work » Needs review
FileSize
9.29 KB
19.54 KB

Fixed tests. I think we should add tests for autocreate functionality.

jibran’s picture

FileSize
850 bytes
19.54 KB

Green Yay!!!

jibran’s picture

Title: [meta] Discuss and finalize fieldSettingsForm » Allow user to select per entity type selection on field settings page
Issue summary: View changes
jibran’s picture

Title: Allow user to select per entity type selection on field settings page » Allow user to select bundles per entity type on field settings page
jibran’s picture

larowlan’s picture

  1. +++ b/src/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
    @@ -155,7 +155,26 @@ class DynamicEntityReferenceItem extends ConfigurableEntityReferenceItem {
    +    $entity_type_ids = $this->getAllEntityTypeIds($settings);
    

    we're calling a static method using $this?

  2. +++ b/src/Plugin/Field/FieldWidget/DynamicEntityReferenceWidget.php
    @@ -137,10 +146,27 @@ class DynamicEntityReferenceWidget extends AutocompleteWidget {
    +        $this->fakeFieldSettings($values['target_type']);
    

    Should we call this again at the end to reset the values?

  3. +++ b/src/Plugin/Field/FieldWidget/DynamicEntityReferenceWidget.php
    @@ -137,10 +146,27 @@ class DynamicEntityReferenceWidget extends AutocompleteWidget {
    +        $handler = \Drupal::service('plugin.manager.entity_reference.selection')->getSelectionHandler($this->fieldDefinition);
    

    We can inject this?

  4. +++ b/src/Plugin/Field/FieldWidget/DynamicEntityReferenceWidget.php
    @@ -148,55 +174,32 @@ class DynamicEntityReferenceWidget extends AutocompleteWidget {
    +   * Sets the fake field settings values.
    

    Can you elaborate why we need fake field settings with a comment?

jibran’s picture

FileSize
3.42 KB
19.49 KB

Thanks for the review. I made some doc fixes.

  1. Nice catch. Fixed.
  2. No need. We are faking it in \Drupal\dynamic_entity_reference\Plugin\Field\FieldWidget\DynamicEntityReferenceWidget::elementValidate() so we are not saving anything we are just updating the loaded object.
  3. No we can't see AutocompleteWidget:66
  4. I have made some docs changes.

Status: Needs review » Needs work

The last submitted patch, 18: field_settings_form-2346273-18.patch, failed testing.

jibran’s picture

Damn it schema is messing this up again. :S

jibran’s picture

So the issue is these lines introduced in #2370305: Refactor field type configuration schemas for DX, easier to find errors. It works fine after commenting these lines.

+++ b/core/modules/field/src/Entity/FieldConfig.php
@@ -138,9 +138,13 @@ public function preSave(EntityStorageInterface $storage) {
+    // Filter out unknown settings and make sure all settings are present, so
+    // that a complete field definition is passed to the various hooks and
+    // written to config.
+    $default_settings = $field_type_manager->getDefaultFieldSettings($storage_definition->type);
+    $this->settings = array_intersect_key($this->settings, $default_settings) + $default_settings;

DER $default_settings are array(); in above patch cuz DER has no way to set the default values until DynamicEntityReferenceItem::defaultFieldSettings() can access FieldStorageDefinitionInterface $field_definition->getSettings();. It should return

array(
  'node' => array(),
  'user' => array(),
  'taxonomy_term' => array(),
// any other entity_type selected on field storage form
)
Gábor Hojtsy’s picture

A somewhat ugly workaround would be to store all the dynamic settings under one more key (one level deeper), which gets around the cleaning of settings. Ideally you would be able to generate the proper settings list though dynamically.

yched’s picture

Or can't you just set the default values to NULL ? Not being able to statically provide "valid" default values is one thing, but defaultFieldSettings() should at least list the available settings in the array keys.

jibran’s picture

Status: Needs work » Needs review
FileSize
1.97 KB
20.01 KB

Thank you @Gábor Hojtsy and @yched for the suggestions and quick response. I went with @yched suggestion. I can't use NULL but I can use array() so that config schema will remain the same. Here is the updated patch it fixes the fails.

Ideally you would be able to generate the proper settings list though dynamically.

Do you think it is a good idea to create a issue in core for this? "Allow FieldItemInterface::defaultFieldSettings() to access FieldStorageDefinitionInterface $field_storage_definition"

jibran’s picture

FileSize
7.57 KB
25.63 KB

Added auto create test as per #12. TypeData is mocking this up again so over to you @larowlan.

Status: Needs review » Needs work

The last submitted patch, 25: field_settings_form-2346273-25.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
1.59 KB
26.04 KB

I think I fixed it and I think it is a right fix.

yched’s picture

Do you think it is a good idea to create a issue in core for this? "Allow FieldItemInterface::defaultFieldSettings() to access FieldStorageDefinitionInterface $field_storage_definition"

Not too hot on the idea. The set of exising settings is not supposed to vary depending on the field. Previsouly the content of defaultFieldSettings() / defaultStorageSettings() were in the @FieldType annotation (and in hook_field_info in D7)

larowlan’s picture

+++ b/src/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
@@ -165,11 +190,18 @@ class DynamicEntityReferenceItem extends ConfigurableEntityReferenceItem {
+      foreach (array_keys($settings) as $entity_typ_id) {
...
+          $entity_typ_id,

missing an e?

larowlan’s picture

Other than nitpick, this tests well - awesome work @jibran

jibran’s picture

Status: Needs review » Fixed
Issue tags: -Needs tests
FileSize
2.99 KB

Added following changes on commit. Thank you @larowlan for testing and thank you @yched, @Gábor Hojtsy and @Berdir for you helping me out here.

  • jibran committed 8713890 on 8.x-1.x
    Issue #2346273 by jibran, larowlan, yched, Berdir, Gábor Hojtsy: Allow...

Status: Fixed » Closed (fixed)

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

jibran’s picture

Assigned: larowlan » Unassigned