Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Follow-up of #2931673: [META] Support multiple styles.
There are cases where users might want to select more than one style to a paragraph type. Currently not possible.
Proposed resolution
Allow users select more than one behavior style.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#73 | interdiff-2935219-71-73.txt | 3.73 KB | johnchque |
#73 | allow_selecting_multiple-2935219-73.patch | 57.46 KB | johnchque |
| |||
#71 | interdiff-2935219-69-71.txt | 502 bytes | johnchque |
#71 | allow_selecting_multiple-2935219-71.patch | 57.17 KB | johnchque |
| |||
#69 | interdiff-2935219-67-69.txt | 2.47 KB | johnchque |
Comments
Comment #2
johnchqueFirst try, can display multiple selects depending on how many groups have been selected.
Wondering if introducing the "groups" key would help, or just change the current "group" one.
Comment #4
johnchqueOK, now updating the selects when changing. Needs more work in the style selection in a paragraph.
Comment #6
miro_dietikerLet's shift the titles immediately after the #type line each. That improves readability.
This needs to change to a new key "groups" as well. We also should still keep the old key for the valid fallback.
All existing config needs updates. Most should use the new notation.
We should make sure we have a test with one old test item to deal with the fallback to guarantee compatibility.
Comment #7
johnchqueMade some progress I can show two selects in the paragraph now. WIP patch. :)
Comment #9
johnchqueMore WIP, now both styles are applied to the paragraph, some tests might fail.
Comment #11
johnchqueBeing debugging and investigating, somehow the groups element is rendered with the an extra "[]" at the end of its name. Tried to update tests, then I figured it out that there are some WSOD cause when using either default or defaults, one of them ends up being NULL. Need to check more. Didn't know the code had changed that much.
Comment #13
johnchqueOK, had to do lots of adjustments, by now, not really sure how we gonna handle the template suggestions just implemented in #2935188: Add selected style from plugin to template suggestions. Besides that seems that it works correctly, need to implement new tests for multiple plugins enabled.
I wonder which config is the preferred, if the one set as default in the paragraph type config form or the one selected in the paragraph widget.
Tests about template suggestions gonna fail.
Comment #15
miro_dietikerTemplate suggestions: Let's simply pick the first ones we find. We don't need to combine them at all.
Comment #16
johnchqueFixing all tests, let's see if this passes everything. :)
Comment #18
johnchqueUpdating more tests. :)
Comment #19
johnchqueI think we need to specific tests for this. Will add them. :)
Comment #20
BerdirYes, quite a few. Specifically also for transitions between one and multiple style groups. if you have group A,B,C test going from A to A,B and from A,B to B and from A,B to C or so. All while having existing content.
Comment #21
johnchqueWIP of tests. :) Need to work on them. :)
Comment #23
johnchqueAdded better tests. :)
Comment #24
miro_dietiker:-)
Comment #25
johnchqueOops my bad, fixing that. :)
Comment #26
Berdirmaybe we should have another key here so we have styles[$group_id] = $style?
Also unhappy about the fact that we don't have machine names for our groups but just the labels and then some magically converted version of it for the key..
$this->configuration['groups'][$group] doesn't exist actually, as group is already the value of the group, so just use that.
Can you include a screenshot of this form with one and two selected groups?
unsure about keeping both keys, actually. might be easier to write an update function than maintain all that code duplication forever?
code style.
yep, this again. We should at least have a method for this, to make sure we do this consistently.
Or we add separate definitions of groups, with an id and label, for example in a $module.paragraphs.style_group.yml file or by introducing a sub-key for styles and groups.
That would need to be a separate issue that we do first, need to discuss with Miro.
also here it is $group_name, above $group_id.
Comment #27
johnchqueYeah, update function sounds good.
Would we have more per style config? If that is the case we need a better schema.
Comment #28
Berdirnot sure, but considering that we have the default setting, we could indeed do something like this:
?
Comment #29
BerdirDiscussed.
So yes, lets unify on that structure, write an update function that converts existing paragraph types. We only keep the fallback in one place, and that's for the selected style on paragraphs. add a getStyles($paragraph) method, check the styles behavior setting key first, if empty, check style and then map that to its group.
I think we also need to do the separate group discovery. Probably easier here, then we only need one update function that can do both things at once. That will need to map the label from before back to the new id.
Comment #30
johnchqueJust to clarify, all styles will now require a group.
Working on this.
Comment #31
johnchqueTook me a while to change everything, we don't have code duplication anymore. :) Let's see how many tests it breaks.
Update function is also missing.
Comment #33
johnchqueUpdating tests and providing update function, need to test that still. :)
Comment #35
johnchqueMore and more tests need to change. Still working on it, gonna continue later on today or tomorrow. :)
Comment #37
johnchqueRemoved the required flag of the groups field, even if the style plugin is not enabled it still fails due to that.
Comment #39
BerdirRight, that problem again, because we always show the form elements. You should be able to implement that yourself in the validate configuration form method, that should only be called if enabled.
Hm, isn't general_group something that's only defined in demo? That means hook_install() of the demo module should set that value.
ah, you're actually already doing that here, so should be able to just remove it above. And I guess not have the plugin enabled by default then.
we can remove the quotes in that case.
Might be better to use raw config here, using (config) entity api in update functions is always tricky, especially through the plugins. Default settings or so might get in the way.
You can use $config_factory->listAll('paragraphs.paragraph_type.', then load each with getEditable($id)
Lets try to put that in a single cache entry, will comment on that later.
Ah, you made a separate service. I think I would put it in the same service.
just move those methods on the existing discovery and then when you do the discovery have two methods and just store things in two different properties.
I think this only ever makes sense during the upgrade path and then never again. I'd just implement that logic in the update function and loop over the group list.
Especially with translatable labels and things like that we just don't want to support this as an offical API
Comment #40
miro_dietikerMaybe a follow-up, but realised while reviewing here a bit:
If a style group only has one or zero styles, it shouldn't be displayed at all in the form.
Comment #41
johnchqueOK, addressing changes of @Berdir's comment. Let's see how many test this breaks now. :)
Comment #43
johnchqueOK, this should pass all tests. :)
Comment #45
johnchqueFixing all tests. :) :) :)
Comment #46
BerdirNot what I meant. We do need to do a reverse lookup, I just meant you should do it inside this function. get them all and loop over it.
Also translation is fun, if we have a label that for some reason already has a translation then it woudn't match. If you have a translatable object, you can get the untranslated string from it.
Also, above you use $paragraph_type->getBehaviorPlugin(), that won't work anymore, now you need to access it all as raw config.
Comment #47
johnchqueOK, took me a while to test but this should work. Was confusing the paragraph type with the entity. ^^'
Comment #48
johnchqueRebasing with latest HEAD. :)
Comment #50
johnchqueSorry, had to do some other changes too. :)
Comment #51
johnchqueFound a bug while testing.
Comment #53
johnchqueFound another bug, adding fix. :)
Comment #56
johnchqueBeen working on this and I think that it might get tricky to update existing config when we introduced the key "style" in the config for each paragraph, we would need to use the getBehaviorSetting('style', ['styles, 'dummy']) when initially we didn't have to use an array as second parameter.
Should we remove the styles key so we just keep old config untouched and probably working without being updated.
Comment #57
BerdirI don't care too much about breaking the API, this project doesn't even have an alpha release. But I do care about an update path.
Also, those bugs you are finding should also be covered by test coverage here if possible.
Comment #58
johnchqueYep, the bugs are being solved here, that's why I added a test-only patch each time, to show that the new patch with the new changes fixes the problem.
Updating update function, need to test a bit more to give more detailed explanation about how the config is being updated.
Comment #59
miro_dietikerNot sure if we can perform this without a chunk based process.
We also would need to update previous revisions so reverts would work...
Might not be present on some Paragraphs.
Comment #60
BerdirYes, this definitely doesn't scale. That's why I said we should do this at runtime, not in an update function that updates everything.
Comment #61
johnchqueWill do that then. :)
Comment #62
johnchqueRemoved the migration for Paragraphs instances. Adding better logic so it can work with previous stored data. There are some methods without docs, wanna see if something is getting broken first. :)
Comment #63
johnchqueAdded docs and tested a bit more, seems to work with old config.
Comment #64
johnchqueFound another problem that was driving me crazy.
Comment #66
BerdirGetting there, lots of minor things and possible cleanup/simplification. Didn't review the tests closely.
Hm, what I expected is that you do this transformation on the group definition on each style. This works too.
However, there is no need for this awkward back-conversion of the label into the style. Those are the group definitions, the group ID is already available to you as they of the $groups array, just use it. That has the advantage that the group ids do *not* need to match the values as long as the group labels match.
In the first condition you set $group, but then you actually still get again.
I would actually suggest that you move the $group assignment out of the condition on a separate line, just above it. Then the condition just becomes if ($group && isset($group_ids[$group]) which is 3x easier to read. Maybe also name it $group_label, to make it clear that we are working with the group label here.
you still have $groups from above, keyed by group id, so you can simplify this to isset($groups[reset($style['groups'])]). might also make sense to check that $style['groups'] is !empty() first, just in case there still is a style defined that does not have a group.
$plugin_default is a strange variable name. $default_style or $group_default would make more sense to me.
Are we sure that the $group_id array key is always set or does this need a !empty()? what happens if you enable a new group and then edit existing content?
the description doesn't make sense anymore. you can select more than one, and you must select at least one, so the second part can go away.
$group_default doesn't seem to be used but a new $default is created below which is quite confusing?
Maybe loop over foreach (array_keys($groups) as $group_id) when you don't need the value?
you must not pass dynamic values to t(), this must use something like @group_label
not sure the description here is really useful as it is duplicated for each group. IMHO, either remove or include the group label again.
ok, so we only support the first group for style templates. That's tricky, especially because we can't control which group is first.
I think we should either support multile (which means changing the return value, but we'recompletely changing the structure of the config here as well, so I'm OK with that) or at least loop until you find the first template.
... current styles for each enabled group .. or so.
string[] would be a better return type for this.
I'm missing the default fallback logic here, which would likely simplify code that is using this quite a bit.
My recommendation would be to change the order a bit and do something like this:
1. Create $paragraph_styles (or just $styles, not sure why we need two variables) and loop over the groups, initialize with the default value for each group.
2. Check if you have styles configured, if yes, overwrite the defaults with the configuration that you have.
3. If not, check if you have a single style, match that to the correct group (as you have it now).
Comment #67
johnchqueOK, this should work better, addressing changes of @Berdir's comment. Not sure if this might break some test.
Comment #68
johnchqueGreat, will add tests for multiple templates and test the update method once again. :)
Comment #69
johnchqueSo adding tests for multiple templates and updating the update function, should work now. :)
Comment #71
johnchqueRemoved this by mistake.
Comment #72
BerdirHm.. geStyleTemplates() would make more sense to me as a method name.
question here is also if we want to keep it static or not, but I guess that's fine..
Comment #73
johnchqueShould we make this happen? :)
Comment #75
BerdirImproved variable names a bit in getStyles() but we just have too many different things in there.
Committed.