Problem/Motivation
Saving a paragraphs type creates configuration for all behavior plugins - even disabled ones.
Like so:
behavior_plugins:
style:
group: ''
grid_layout:
paragraph_reference_field: ''
available_grid_layouts: { }
lockable: { }
primer_background:
background_image_field: ''
This empty dummy configuration can be invalid. We should get rid of this behavior.
Proposed resolution
Change the saving behavior of paragraphs types. Disabled plugins should have not configuration stored.
Remaining tasks
User interface changes
API changes
Let /** var \Drupal\paragraphs\ParagraphsBehaviorCollection $plugin_collection */
.
$plugin_collection->getConfiguration()
now only contains keys for enabled plugins, while $plugin_collection->get('<diabled_plugin_id>')->getConfiguration()
should still return (the default) valid plugin configuration.
The output arrays of ParagraphsBehaviorCollection::getAll
, ParagraphsBehaviorCollection::getEnabled
and ParagraphsTypeInterface::getEnabledBehaviorPlugins
are now required to be sorted by behavior plugin weight.
The instanceIDs
array of a ParagraphsBehaviorCollection
is now kept sorted by plugin weight, while the pluginInstances
array remains unsorted.
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#14 | stop_saving_of_empty-2884131-14.patch | 7.26 KB | VladimirMarko |
| |||
#14 | interdiff-2884131-12-14.txt | 2.39 KB | VladimirMarko |
#10 | stop_saving_of_empty-2884131-10-test-only.patch | 1.48 KB | VladimirMarko |
Comments
Comment #2
VladimirMarko CreditAttribution: VladimirMarko commentedComment #3
VladimirMarko CreditAttribution: VladimirMarko commentedThis causes test failures, because it reorders the behavior plugins in the behavior form and also in the view, for some reason.
Comment #4
VladimirMarko CreditAttribution: VladimirMarko commentedI do not like the issues this creates. Just filtering the output of
ParagraphsBehaviorCollection::getConfiguration
should not have caused them. As we cannot restrict when this function gets called, I do not think that there is a nice way of fixing this.I also do not like that
$plugin_collection->getConfiguration()['<diabled_plugin_id>']
and$plugin_collection->get('<diabled_plugin_id>')->getConfiguration()
can now return two different results.Unassigned.
Comment #5
miro_dietikerStop saving is about saving and submitting the paragraph type form.
We should never implement workarounds in getters to filter out garbage data, instead clean before / on save.
Let's look at \Drupal\paragraphs\Form\ParagraphsTypeForm::save
So it looks like here we are setting an empty array instead of unsetting the key.
Comment #6
VladimirMarko CreditAttribution: VladimirMarko commentedWe are setting the plugin configuration to default, in that code snippet. This is because of
in
ParagraphsBehaviorBase
.The defaults are apparently needed for our code to function, so we cannot actually unset the configuration. So the only option would be to filter the configuration somewhere along the way to the export, as I did in my patch above.
Comment #7
miro_dietikerPlugins should not be called if they are disabled. Thus there should be no dependency in code to anything like default configuration.
If anything, we should make sure default configuration is only applied and required if it is enabled.
If a new plugin is registered, all paragraph types are changed on next export. That's really bad.
Comment #8
miro_dietikerAh.. i mixed this again with #2884236: ParagraphsBehaviorBase::submitBehaviorForm() results in in a lot of empty settings being saved
Will edit the previous comment.
It's still annoying with config export and maintenance of something like a distribution. Thus keeping major.
Comment #9
VladimirMarko CreditAttribution: VladimirMarko commentedI'll give this another attempt.
Comment #10
VladimirMarko CreditAttribution: VladimirMarko commentedI needed to adjust multiple functions which return arrays of behavior plugin to return these ordered by plugin weight.
Comment #12
VladimirMarko CreditAttribution: VladimirMarko commentedFixed a typo.
Comment #13
Primsi CreditAttribution: Primsi at MD Systems GmbH for MD Systems GmbH commentedThis one is missing the docbloc.
I guess we can remove this getAll()?
I am not really familiar with this, so asking: can we just return $this->pluginInstances?
Wandering if we could use something like SortArray::sortByKeyInt here
Comment #14
VladimirMarko CreditAttribution: VladimirMarko at MD Systems GmbH commented3.
As discussed, the order of keys in
instanceIDs
andpluginInstances
is not necessarily the same andsort
(inherited formDefaultLazyPluginCollection
) only sorts the former. As I did not want to change this, each time an array of plugin instances is returned (getAll
,getEnabled
), I need to reorder it.I added a comment to help clarify the purpose of the loop.
Implemented the rest.
Comment #15
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedI think this issue does more now than what was the original scope...
Since we could use the previous contributions I retitled this issue to #2884131: Sort behavior plugins by weight and addressing the original problem in #3121776: Filter out disabled behavior plugins on the paragraphs type configuration form.
Comment #16
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedThis was committed in #3119488: Sort list of enabled behaviors in paragraphs.paragraphs_type configs alphabetically
Can't we do the sort when needed, in
ParagraphsBehaviorCollection::getAll()
?This should be avoided as per #5.