Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#31 | interdiff.txt | 2.99 KB | jibran |
#30 | Screenshot 2014-12-05 19.13.31.png | 38.82 KB | larowlan |
#27 | field_settings_form-2346273-27.patch | 26.04 KB | jibran |
#27 | interdiff.txt | 1.59 KB | jibran |
Comments
Comment #1
larowlanhi @jibran - I'm happy with the existing selector - but if you want to add the notion of selection plugins that's fine with me.
Comment #2
jibranWell 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!!!
Comment #3
larowlanFrom DynamicEntityReferenceController::buildEntityQuery?
But either way, go for it :)
Comment #4
BerdirYes, that's the theory. See all the hacks in the different entity reference selection plugins why that is not enough.
Comment #5
jibranHow 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.
Comment #7
jibranHere is some more progress.
@todo Update DynamicEntityReferenceWidget accordingly.
@todo FIx/Write tests.
Comment #9
jibranI 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.
Comment #11
larowlanComment #12
jibranFixed tests. I think we should add tests for autocreate functionality.
Comment #13
jibranGreen Yay!!!
Comment #14
jibranComment #15
jibranComment #16
jibranReroll after #2366093: Unable to set field value programmatically.
Comment #17
larowlanwe're calling a static method using $this?
Should we call this again at the end to reset the values?
We can inject this?
Can you elaborate why we need fake field settings with a comment?
Comment #18
jibranThanks for the review. I made some doc fixes.
\Drupal\dynamic_entity_reference\Plugin\Field\FieldWidget\DynamicEntityReferenceWidget::elementValidate()
so we are not saving anything we are just updating the loaded object.Comment #20
jibranDamn it schema is messing this up again. :S
Comment #21
jibranSo 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.
DER
$default_settings
arearray();
in above patch cuz DER has no way to set the default values untilDynamicEntityReferenceItem::defaultFieldSettings()
can accessFieldStorageDefinitionInterface $field_definition->getSettings();
. It should returnComment #22
Gábor HojtsyA 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.
Comment #23
yched CreditAttribution: yched commentedOr 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.
Comment #24
jibranThank 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 usearray()
so that config schema will remain the same. Here is the updated patch it fixes the fails.Do you think it is a good idea to create a issue in core for this? "Allow
FieldItemInterface::defaultFieldSettings()
to accessFieldStorageDefinitionInterface $field_storage_definition
"Comment #25
jibranAdded auto create test as per #12. TypeData is mocking this up again so over to you @larowlan.
Comment #27
jibranI think I fixed it and I think it is a right fix.
Comment #28
yched CreditAttribution: yched commentedNot 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)
Comment #29
larowlanmissing an e?
Comment #30
larowlanOther than nitpick, this tests well - awesome work @jibran
Comment #31
jibranAdded following changes on commit. Thank you @larowlan for testing and thank you @yched, @Gábor Hojtsy and @Berdir for you helping me out here.
Comment #34
jibran