Introduce a plugin system for paragraphs types

One important thing about naming:
We introduced the idea when we learned that we need to separate perspectives "Content" and "Layout".
Further research shows that it's more Content and Non-Content and all kinds of properties. Layout is a good example.

We will use the field system to assign fields or extra fields to a specific perspective. Managing those fields is done on "manage form display" table with parent grouping like core does (for instance block regions).
A plugin will extend the field definitions and offer its properties / form as extra fields.
They will be pre-assigned to a perspective, but the user is free to place them anywhere.

So i propose this terminology and definition:
We introduce ParagraphTypeFeature plugins.
Each plugin is likely a collection of settings and can be enabled on paragraph types.
Enabled plugins can then offer settings.
Based on these settings, the plugin determines what extra fields it offers to form management and display.
The plugin is deeply integrated into:
Paragraph type settings, Field management (form display, formatter output), Paragraph widget (field display, validation, submit)
Thus implementing lots of methods to alter all the elements.

In order to save properties, we use a single serialized field for all plugins. This field needs to be added to the paragraphs entity as base field.

A paragraph type can enable multiple features. All enabled plugins are triggered to extend the paragraphs. Plugins thus need to have (static) weight to deal with dependency problems (a plugin adding grid extra properties will be called after the basic grid plugin).

Let's simply implement a test plugin first. Enable it on a test paragraph type. Make it provide a field. Make it appear in the form. Make sure submission is persisted. And test cover it.

The perspective tab (UI element) is a separate issue. Possibly multiple.
We could also put configurability of those properties (manageme form display) into a follow-up.
Introducing advanced plugin settings that appear in paragraph type configuration can also be a follow-up.

Adding settings per plugin can be added in a follow-up.
Let's first complete a plugin in its most simple form. This means the example plugin needs to provide a form item on the paragraph, its data needs to be stored and test covered.

+++ b/src/Entity/Paragraph.php
@@ -296,6 +296,10 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface, EntityN
+    $fields['features'] = BaseFieldDefinition::create('string')

I'd say, the plugin provides a feature (although feature clashes with the feature module) - however what we store are the serialised settings / properties, not the feature itself.

1. +++ b/src/Annotation/ParagraphsTypeFeatureBuilder.php
   @@ -0,0 +1,40 @@
   +class ParagraphsTypeFeatureBuilder extends Plugin {
   

   Compared to other plugins, by example:

   class EntityType extends Plugin {
   

   If we name the concept "Feature" then it's the Feature that is a plugin, not the Builder.

2. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -1115,6 +1129,10 @@ class InlineParagraphsWidget extends WidgetBase {
   +          $paragraphs_entity->setUnserializedSettings($item['feature']);
   

   Serializing and unserializing is a matter of putting the lifetime object into storage and loading it. That needs to be abstracted away.

   Users of this settings should not care how it is stored. They just want to call getSettings(), setSettings() - Or should we use get/setFeatureSettings()?

   Be careful about the whole lifecycle including cache serialization and fetching (__sleep, __wakeup). But i would expect it is not a problem.
   Dunno if we should override \unserialize()?

1. +++ b/src/Entity/Paragraph.php
   @@ -119,6 +125,46 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface, EntityN
   +  public function getFeatureSettings($key = []) {
   +    if ($this->get('feature_settings')->value) {
   +      $this->unserializedFeatureSettings = unserialize($this->get('feature_settings')->value);
   +    }
   +    else {
   +      $this->unserializedFeatureSettings = [];
   +    }
   +    if (empty($key)) {
   +      return $this->unserializedFeatureSettings;
   +    }
   +    return NestedArray::getValue($this->unserializedFeatureSettings, $key);
   

   Mixing gettig all and a specific key in a single method is bad practise. Use a getFeatureSettings() and getFeatureSetting() instead.

   Also, the unserialized logic seems backwards. Every time you call it, you replace unserializedFeatureSettings again. You must only do that once per object lifetime, so check it for !== null first.

   example that should be tested:
   1. new paragraphs, set something
   2. save
   3. load again, get feature setting
   4. set new value
   5. get must return the new value.

   Additionally, as I understand, those settings are always for a specific plugin. That means we will *always* have at least the plugin_id. And we shouldn't mix that into the key but have a $plugin_id and separate $keys argument.

2. +++ b/src/Entity/Paragraph.php
   @@ -119,6 +125,46 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface, EntityN
   +  public function setFeatureSettings($feature_settings) {
   +    // Set feature settings field.
   +    $this->unserializedFeatureSettings = $feature_settings;
   

   same here, enforce that the top level is always grouped by plugin_id. We might not even need an API that returns/sets for all plugins, or if, then as a separate method.

3. +++ b/src/Form/ParagraphsTypeForm.php
   @@ -41,7 +70,43 @@ class ParagraphsTypeForm extends EntityForm {
   +        '#empty' => t('There are no plugins yet. Add a plugin.'),
   

   this empty message doesn't make sense, we don't add the table if there are no plugins?

4. +++ b/src/ParagraphsTypeFeatureInterface.php
   @@ -0,0 +1,24 @@
   +   */
   +  public function buildFeaturesForm(Paragraph $paragraphs_entity);
   

   we should probably model this after the build/validate/submitConfigurationForm() pattern except we also pass in the paragraph entity. (not sure we need $paragraphs_entity as a variable name)

5. +++ b/src/ParagraphsTypeFeatureManager.php
   @@ -0,0 +1,60 @@
   +    // @todo define settings for the plugins.
   +    $this->config = $config_factory->get('paragraphs.paragraphs_type');
   

   that's now how this works? their settings are stored in config entities, so if you want to inject something it is the entity storage for paragraph type entities but I would skip that until we actually need it.

6. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -592,6 +592,20 @@ class InlineParagraphsWidget extends WidgetBase {
   +      $paragraphs_type = ParagraphsType::load($paragraphs_entity->getType());
   +      if ($paragraphs_type) {
   +        $config = \Drupal::config('paragraphs.paragraphs_type.' . $paragraphs_type->id());
   

   same here, you want something like $paragraphs_type->getFeaturePlugins(), not read the raw config.

   We should also use the LazyPluginCollection system I think, see FilterFormat for a good/similar example.

7. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -1059,6 +1073,9 @@ class InlineParagraphsWidget extends WidgetBase {
   +        if (isset($item['feature'])) {
   +          $display->extractFormValues($entity, $element['feature'], $form_state);
   +        }
   

   display is about fields, so I don't think this will work. If we do add the feature of adding fields in there, it will need to be done through #groups and fields will need to live top-level. you need to call the validate/submit methods on the plugin instead.

8. +++ b/src/Entity/Paragraph.php
   @@ -119,6 +125,46 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface, EntityN
   +
   +    if ($this->unserializedFeatureSettings) {
   +      $this->set('feature_settings', serialize($this->unserializedFeatureSettings));
   +    }
   +    else {
   +      $this->set('feature_settings', serialize([]));
   +    }
   +  }
   +
   

this is the reason you lose your data.

It is related to the stuff above, and you will need to think about this here as well.

In short, empty array != NULL. If it is NULL, then you must *not* change it, as you never unserialized. Except possibly setting the default value, but you should just use setDefaultValue() on the field definition so it is initialized with the serialized empty array.

You can test test this easily be saving a paragraph with some settings, load again and save without accessing them. Your settings will be lost.

The reason that it only happens on new paragraphs is because our needsSave() logic has a bug.

We save new paragraphs twice. Once because it is new and needs to be saved because of that and a second time because needsSave() is still TRUE.

An easy workaround for that is not setting needsSave() for a new entity. A better fix would be to ensure that needsSave() is set to FALSE in Paragraph::postSave().

So this makes it pretty clear that we absolutely need kernel/API tests for handling feature settings. Try all kinds of saving, loading, updating, loading again, resave without changing and verifying all those operations result in correct data. That should help you find and fix a lot of bugs that will be much harder to debug in UI tests.

1. +++ b/src/Entity/Paragraph.php
   @@ -135,34 +135,60 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface, EntityN
   +   * {@inheritdoc}
   +   */
   +  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
   +    $this->setNeedsSave(FALSE);
   +    parent::postSave($storage, $update);
   

   fine to test, but we should do a dedicated issue for this, with test coverage (not quite sure yet how, could be as simple as calling paragraphs->save() and then making sure that needsSave() returns FALSE)

2. +++ b/src/Entity/Paragraph.php
   @@ -135,34 +135,60 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface, EntityN
   +    if ($this->unserializedFeatureSettings == NULL) {
   

   array == NULL => TRUE
   array === NULL => FALSE

   you need type safe checks.

You're still incorrectly always setting the feature settings back. As I said, you must only do that when you actually unserialized your settings. And then it works just fine* ;)

* Fine as far as that the test is now green. I haven't really reviewed the last patches yet.

We will need a META issue for everything related to the plugins and make all current related issues its children.

A first try to review this monster... :-)

1. +++ b/config/schema/paragraphs_type.schema.yml
   @@ -8,6 +8,19 @@ paragraphs.paragraphs_type.*:
   +          weight:
   +            type: integer
   +            label: 'Weight'
   

   We need weight per plugin (global - to guarantee a coherent sequence in execution), not per plugin per paragraph type.

2. +++ b/src/Annotation/ParagraphsTypeFeature.php
   @@ -0,0 +1,43 @@
   +   * The human-readable name of the paragraphs type layout builder.
   
   +++ b/src/ParagraphsTypeFeatureInterface.php
   @@ -0,0 +1,51 @@
   +   * This method is responsible for building the layout settings for each
   +   * Paragraph so the user can set special layouts.
   

   Let's use "feature" consistently.

3. +++ b/src/Entity/Paragraph.php
   @@ -128,6 +136,67 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface, EntityN
   +    // If un-serialized settings are already set then do not overwrite them.
   

   Outdated comment.

4. +++ b/src/Entity/Paragraph.php
   @@ -128,6 +136,67 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface, EntityN
   +  public function setFeaturesSettings($settings) {
   ...
   +  public function setFeatureSettings($plugin_id, $settings) {
   

   Misleading naming. It took me multiple minutes to realise that the names differ.

5. +++ b/src/Entity/ParagraphsType.php
   @@ -54,4 +55,39 @@ class ParagraphsType extends ConfigEntityBundleBase implements ParagraphsTypeInt
   +   * The ParagraphsType plugins.
   ...
   +  public $feature_plugins = [];
   

   I guess the ParagraphsTypeFeature plugins. But only the names or what?

6. +++ b/src/Entity/ParagraphsType.php
   @@ -54,4 +55,39 @@ class ParagraphsType extends ConfigEntityBundleBase implements ParagraphsTypeInt
   +      $this->featureCollection = new ParagraphsTypeFeatureCollection(\Drupal::service('plugin.manager.paragraphs_type.feature'), $this->feature_plugins);
   

   I first thought: why not on constructur? Because selected types could change?
   But then realised that if feature_plugins change, it is not reinitialised.

7. +++ b/src/Entity/ParagraphsType.php
   @@ -54,4 +55,39 @@ class ParagraphsType extends ConfigEntityBundleBase implements ParagraphsTypeInt
   +  public function getFeaturePlugin($instance_id) {
   ...
   +    return $this->featureCollection->get($instance_id);
   
   +++ b/src/ParagraphsTypeFeatureBase.php
   @@ -0,0 +1,127 @@
   +    $paragraphs_entity->setFeatureSettings($this->getPluginId(), $values);
   

   Not sure if the base should set all of those. I thought let's have each setting explicitly set by each plugin.

8. +++ b/src/ParagraphsTypeFeatureCollection.php
   @@ -0,0 +1,77 @@
   +   * Retrieves filter definitions and creates an instance for each filter.
   

   filter?

9. +++ b/src/ParagraphsTypeFeatureCollection.php
   @@ -0,0 +1,77 @@
   +      // Do not allow the null filter to be used directly, only as a fallback.
   +      unset($this->definitions['filter_null']);
   

   null filter?

10. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
    @@ -620,14 +620,30 @@ class InlineParagraphsWidget extends WidgetBase {
    +          if ($config = $paragraphs_type->getFeaturePlugins()) {
    ...
    +              if ($plugin->getConfiguration()['enabled'] == TRUE) {
    

    Why is there no method that returns the enabled ones only?

11. +++ b/tests/modules/paragraphs_test/src/Plugin/paragraphs_type/Feature/TestFeaturePlugin.php
    @@ -0,0 +1,32 @@
    +      '#title' => $this->t('Label'),
    ...
    +      '#description' => $this->t("Label for the Paragraphs type."),
    

    Bad example.

12. +++ b/tests/modules/paragraphs_test/src/Plugin/paragraphs_type/Feature/TestFeaturePluginNotRequired.php
    @@ -0,0 +1,30 @@
    +    $form['test_field_not_required'] = [
    ...
    +      '#title' => $this->t('Label'),
    ...
    +      '#description' => $this->t("Label for the Paragraphs type."),
    

    Use a different better example with a unique label.

I'll need to discuss with Berdir about how we can get this in without too much commitment.
We should clearly declare all plugins and interfaces as experimental. I guess we will change some while learning more about out plugins.

#2296423: Implement layout plugin type in core has a ton of discussion about the right approach to document a currently experimental new feature in an otherwise stable method.. @internal is the current chosen approach I believe but havent' closely followed.

1. +++ b/config/schema/paragraphs_type.schema.yml
   @@ -8,6 +8,19 @@ paragraphs.paragraphs_type.*:
   +          weight:
   +            type: integer
   +            label: 'Weight'
   

   What miro means with point 1 is that weight should be a flag on the annotation and not something that we store per paragraphs type.

   I guess that makes sense, it is not that important to have a different order in different paragraphs (we could still do that later if we'd really need it, e.g. to have a different order in the UI, but who knows how that will eventually look like).

   That also simplifies the necessary UI to support this a lot. Right now, we'd need a draggable table to control/change the weight. If we move this to the plugin definition, that goes away (doesn't exist yet, but would have to)

   I see that you started to add that, but not quite done yet it seems.

2. +++ b/paragraphs.services.yml
   @@ -0,0 +1,5 @@
   +services:
   +  plugin.manager.paragraphs_type.feature:
   +    class: Drupal\paragraphs\ParagraphsTypeFeatureManager
   +    parent: default_plugin_manager
   

   not sure about paragraphs_type.feature or paragraphs.feature? And then same for the class names.

   Usually this is module.plugin_type, so I'd go for paragraphs.feature, not 100% sure about the class names, but I'd say same there?

3. +++ b/src/Entity/Paragraph.php
   @@ -128,6 +136,67 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface, EntityN
   +   *
   +   * @return array
   +   *   The array of feature settings.
   +   */
   +  public function getFeaturesSettings() {
   +    if ($this->unserializedFeatureSettings === NULL) {
   +      $this->unserializedFeatureSettings = unserialize($this->get('feature_settings')->value);
   +    }
   +    if (!is_array($this->unserializedFeatureSettings)) {
   

   agreed with Miro, getFeatureSettings() vs. getFeaturesSettings() isn't very clear yet. And the documentation also doesn't really help.

   Maybe getAllFeatureSettings()? And the docs should make it clear that one is from all features and the other is for a specific plugin.

4. +++ b/src/Entity/Paragraph.php
   @@ -128,6 +136,67 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface, EntityN
   +   * @param string $plugin_id
   +   *   The plugin id where to get the setting from.
   

   The plugin ID for which to get the settings.

   Or something like that.

5. +++ b/src/Entity/Paragraph.php
   @@ -128,6 +136,67 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface, EntityN
   +  public function getFeatureSettings($plugin_id, $key) {
   

   Setting*s* and then a key to get a specific one is also not that great.

   And the documention can also be improved to describe $key better. See FormStateInterface::getValue() for good documentation example on $key.

   And I'd say we should then go way getFeatureSetting() here.

   Documentation for all those methods should be on ParagraphsInterface, btw.

6. +++ b/src/Entity/Paragraph.php
   @@ -128,6 +136,67 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface, EntityN
   +   * Sets the un-serialized feature settings.
   

   The fact that we serialize is an implementation detail, you should never have to mention that in the documentation. That's obviuous enough when you pass in an array. So drop that word.

7. +++ b/src/Entity/Paragraph.php
   @@ -128,6 +136,67 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface, EntityN
   +  public function setFeaturesSettings($settings) {
   

   type int on array when it is always an array in the argument ($key in FormState::getValue() can also be a string, but then you need to handle that), settings is always an array.

8. +++ b/src/Form/ParagraphsTypeForm.php
   @@ -41,7 +70,42 @@ class ParagraphsTypeForm extends EntityForm {
   +      $weight = count($feature_plugins) + 1;
   +      $feature_plugins_order = [];
   +      foreach ($feature_plugins as $id => $feature_plugin) {
   +        $feature_plugins_order[$id] = [
   +          'label' => $feature_plugin['label'],
   

   this should also get easier then. We can do the sorting earlier, in findDefinitions() and then they will be in the right order already here.

9. +++ b/src/ParagraphsTypeFeatureBase.php
   @@ -0,0 +1,120 @@
   +    $this->configuration += $this->defaultConfiguration();
   

   note that this doesn't work recursively. Doing it recursively has other drawbacks, just means we don't support nested default configuration and should keep it flat.

10. +++ b/src/ParagraphsTypeFeatureBase.php
    @@ -0,0 +1,120 @@
    +      $container->get('config.factory'),
    +      $container->get('entity_type.manager')
    

    we don't need either of those, config is is saved in paragraph/paragraph_type and we shouldn't have to load any entities.. if some plugins do, they can still add those. so you can drop this method completely and also simplify the constructor.

11. +++ b/src/ParagraphsTypeFeatureBase.php
    @@ -0,0 +1,120 @@
    +  public function defaultConfiguration() {
    +    return [
    +      'enabled' => FALSE,
    +    ];
    +  }
    

    I would keep this out of the actual configuration

12. +++ b/src/ParagraphsTypeFeatureBase.php
    @@ -0,0 +1,120 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function setConfiguration(array $configuration) {
    +  }
    +
    

    we can actually implement this.

13. +++ b/src/ParagraphsTypeFeatureBase.php
    @@ -0,0 +1,120 @@
    +   */
    +  public function validateFeaturesForm(array $form, FormStateInterface $form_state) {
    +    // TODO: Implement validateFeaturesForm() method.
    

    no need for a TODO here, just keep it empty like other methods.

    Also, I'd put empty method blocks on a single line... function bla (arguments) { }

14. +++ b/src/ParagraphsTypeFeatureCollection.php
    @@ -0,0 +1,87 @@
    +   * {@inheritdoc}
    +   */
    +  public function sort() {
    +    $this->getAll();
    +    return parent::sort();
    +  }
    

    we don't need sorting then if we don't have per-type weights I think.

15. +++ b/src/ParagraphsTypeFeatureCollection.php
    @@ -0,0 +1,87 @@
    +   * This is exclusively used for retrieving all the plugins available for the
    +   * current paragraph type.
    +   */
    +  public function getAll() {
    

    what does "available for the current paragraph type"? Isn't it *all*, at least for now?

16. +++ b/src/ParagraphsTypeFeatureCollection.php
    @@ -0,0 +1,87 @@
    +   */
    +  protected function initializePlugin($instance_id) {
    +    $configuration = $this->manager->getDefinition($instance_id);
    +    // Merge the actual configuration into the default configuration.
    +    if (isset($this->configurations[$instance_id])) {
    +      $configuration = NestedArray::mergeDeep($configuration, $this->configurations[$instance_id]);
    +    }
    +    $this->configurations[$instance_id] = $configuration;
    +    parent::initializePlugin($instance_id);
    +  }
    

    this is confusing. the definition is not the configuration. we don't have configuration yet (apart from the enabled flag), we'll add that later.

17. +++ b/src/ParagraphsTypeFeatureInterface.php
    @@ -0,0 +1,51 @@
    +interface ParagraphsTypeFeatureInterface extends PluginFormInterface, ConfigurablePluginInterface {
    

    missing docblock on the interface.

    We now hove 3 methods to control the features form, validating and saving it. But what does such a plugin actually *do* now? and how?

    We can say we do that later, then we need at least an issue and possibly a todo.

    If we can define an actually useful example (like setting the text color, I think I suggested that before) then we can discuss on how to implement that and what kind of methods we need. Specifically, to implement that, we need to be able to a) add some css file/library to the output and we need to add a class to the output based on the user selection. So this needs something in view/preprocess. Maybe start with a view() method that has the same arguments as hook_entity_view() and a preprocess() method that receives $variables?

18. +++ b/src/ParagraphsTypeFeatureInterface.php
    @@ -0,0 +1,51 @@
    +   * @return mixed
    +   *   The modified build array that the plugin builds.
    

    modified? we don't get something passed in, so we can't relly modify anyting?

    also should always be an array, not mixed?

19. +++ b/src/ParagraphsTypeFeatureManager.php
    @@ -0,0 +1,35 @@
    +    parent::__construct('Plugin/paragraphs_type/Feature', $namespaces, $module_handler, 'Drupal\paragraphs\ParagraphsTypeFeatureInterface', 'Drupal\paragraphs\Annotation\ParagraphsTypeFeature');
    

    as with the service name, the first folder is usually the provider/module providing the plugin, so just paragraphs. Feature or feature is then always the big question for the second folder. No strong opinion.

    Same for the cache key, alter hooks and so on.

20. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
    @@ -620,14 +620,28 @@ class InlineParagraphsWidget extends WidgetBase {
    +          if ($config = $paragraphs_type->getEnabledFeaturePlugins()) {
    +            foreach ($config as $plugin_id => $plugin) {
    

    $config is a weird name for this. Why not $feature_plugins or so?

21. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
    @@ -620,14 +620,28 @@ class InlineParagraphsWidget extends WidgetBase {
    +              $element['feature'][$plugin_id] = [];
    +              $display->buildForm($paragraphs_entity, $element['feature'][$plugin_id], $form_state);
    +              $element['feature'][$plugin_id] = $plugin->buildFeaturesForm($paragraphs_entity);
    

    I don't understand what the $display line is doing? we're not building an entity form here.

22. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
    @@ -1190,6 +1203,13 @@ class InlineParagraphsWidget extends WidgetBase {
    +        if (isset($item['feature'])) {
    +          $paragraphs_type = ParagraphsType::load($paragraphs_entity->getType());
    +          foreach ($paragraphs_type->getEnabledFeaturePlugins() as $plugin_id => $plugin_values) {
    +            $plugin_values->submitFeaturesForm($paragraphs_entity, $item['feature'][$plugin_id]);
    

    validate doesn't seem to be called yet?

    To be able to test that, I would suggest that the test-text-color plugin would have a text field that only allows "blue" and "red" as input, anything else must result in a validation error.

    And we could then default to "blue" to have the default configuration stuff also tested.

23. +++ b/tests/modules/paragraphs_test/src/Plugin/paragraphs_type/Feature/TestFeaturePluginNotRequired.php
    @@ -0,0 +1,37 @@
    +    $form['test_field_not_required'] = [
    +      '#type' => 'textfield',
    +      '#title' => $this->t('Label'),
    +      '#maxlength' => 255,
    +      '#default_value' => $paragraphs_entity->getFeatureSettings($this->getPluginId(), ['test_field_not_required']),
    +      '#description' => $this->t("Label for the Paragraphs type."),
    +    ];
    +    return $form;
    

    testing #required vs. not isn't a really useful test. That's default form-level validation, not specific to us.

Discussed about naming and synonyms for "feature" to avoid the ambiguous overlap with features module.

We decided to name the plugins "behavior". (and this time it's final) ;-)

Looks like not everything is addressed yet of my review, at least the big/major point #17 for actually doing something with those behavior settings now. And also validation isn't really covered yet I think.

Fine now, but in a case like this, where you do one huge change and also address a review, you might want to do two separate commits, so you can provide two interdiffs, or actually upload separate patches/interdiffs, one for the big rename and another to address the review and do those things separately. Listing which review points were addressed and what's still pending would also help.

1. +++ b/src/Entity/Paragraph.php
   @@ -137,66 +135,69 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface, EntityN
   +  public function setBehaviorSetting($plugin_id, array $settings) {
   

   I think here it actually is Settings() as we set all settings for a behavior. The alternative would be to have $key, $value as argument, then setting might work again.

2. +++ b/tests/modules/paragraphs_test/src/Plugin/paragraphs/Behavior/TestTextColorPlugin.php
   @@ -0,0 +1,49 @@
   +      '#default_value' => $paragraphs_entity->getBehaviorSetting($this->getPluginId(), 'text_color'),
   

   Looks like we're missing a way to set default settings? We can't use defaultConfiguration(), that will be on the paragraph type level. Maybe we could do it like third party settings and add a default value argument to this method, just like $form_state->getValue()? Downside is that we have to repeat it multiple cases but a defaultConfiguration() like system sounds complicated.

3. +++ b/tests/modules/paragraphs_test/src/Plugin/paragraphs/Behavior/TestTextColorPlugin.php
   @@ -0,0 +1,49 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function validateBehaviorForm(array $form, FormStateInterface $form_state) {
   +    parent::validateBehaviorForm($form, $form_state);
   +  }
   +
   

   looks like the validation part isn't here yet?

1. +++ b/paragraphs.install
   @@ -180,4 +180,20 @@ function paragraphs_update_8008() {
   +
   +  $storage_definition = BaseFieldDefinition::create('string_long')
   +    ->setLabel(t('Behavior plugins'));
   +  \Drupal::entityDefinitionUpdateManager()
   +    ->installFieldStorageDefinition('behavior_plugins', 'paragraphs_type', 'paragraph', $storage_definition);
    }
   

   config entities don't have fields, this part isn't needed.

2. +++ b/paragraphs.module
   @@ -238,4 +238,11 @@ function template_preprocess_paragraph(&$variables) {
   +
   +  $paragraph_type_id = $variables['elements']['#paragraph']->getType();
   +  $paragraph_type = ParagraphsType::load($paragraph_type_id);
   +  foreach ($paragraph_type->getEnabledBehaviorPlugins() as $plugin_id => $plugin_value) {
   

   instead of all those ::load() calls, we can just use $paragraph->type->entity, possibly also add a getPararagraphType() method or so (getType() should hae been called getTypeId(), just like node, too late now)

3. +++ b/src/ParagraphsBehaviorInterface.php
   @@ -49,4 +50,32 @@ interface ParagraphsBehaviorInterface extends PluginFormInterface, ConfigurableP
   +   * @param &$build
   +   *   A renderable array representing the entity content. The module may add
   +   *   elements to $build prior to rendering. The structure of $build is a
   +   *   renderable array as expected by drupal_render().
   ...
   +   * @param $view_mode
   +   *   The view mode the entity is rendered in.
   +   *
   

   missing type

4. +++ b/src/ParagraphsBehaviorInterface.php
   @@ -49,4 +50,32 @@ interface ParagraphsBehaviorInterface extends PluginFormInterface, ConfigurableP
   +   * @param \Drupal\paragraphs\Entity\Paragraph $paragraphs_entity
   +   *   The entity object.
   

   we can say that this is actually the paragraph. I would also just name this $paragraph, it's $user and $node, not $node_entity.

5. +++ b/src/ParagraphsBehaviorInterface.php
   @@ -49,4 +50,32 @@ interface ParagraphsBehaviorInterface extends PluginFormInterface, ConfigurableP
   +   * @return array
   +   *   A render array provided by the plugin.
   

   I don't think we need a return type as we have it per reference.

6. +++ b/src/ParagraphsBehaviorInterface.php
   @@ -49,4 +50,32 @@ interface ParagraphsBehaviorInterface extends PluginFormInterface, ConfigurableP
   +  public function preprocess(&$variables);
   

   missing docs.

7. +++ b/tests/modules/paragraphs_test/paragraphs_test.libraries.yml
   @@ -0,0 +1,11 @@
   +  version: VERSION
   

   VERSION is actually only support for core, but it is mis-used everywhere..

8. +++ b/tests/modules/paragraphs_test/src/Plugin/paragraphs/Behavior/TestBoldTextPlugin.php
   @@ -45,4 +45,23 @@ class TestBoldTextPlugin extends ParagraphsBehaviorBase {
   +   * {@inheritdoc}
   +   */
   +  public function preprocess(&$variables) {
   +    if ($variables) {
   +
   +    }
   

   provide an empty default on the base class so you don't need to repeat empty methods.

+++ b/src/ParagraphViewBuilder.php
@@ -16,10 +16,10 @@ class ParagraphViewBuilder extends EntityViewBuilder {
     $build_list = parent::buildMultiple($build_list);
     foreach ($build_list as $key => $value) {
-      $display = EntityViewDisplay::load('paragraph.' . $value['#paragraph']->bundle() . '.default');
+      $display = EntityViewDisplay::load('paragraph.' . $value['#paragraph']->bundle() . '.' . $value['#view_mode']);
       $paragraph_type = $value['#paragraph']->getParagraphType();
       foreach ($paragraph_type->getEnabledBehaviorPlugins() as $plugin_id => $plugin_value) {

It is unfortunately not quite that easy. view modes/view displays have a fallback system. If you pass in view mode "full", and there is no entity view display for that, it falls back to .default. So you need to add "?: EntityViewDisplay::load(... '.default')

Looks

+++ b/paragraphs.install
@@ -181,19 +181,3 @@ function paragraphs_update_8008() {
- * Add behavior plugins fields.
- */
-function paragraphs_update_8010() {
-  $storage_definition = BaseFieldDefinition::create('string_long')
-    ->setLabel(t('Behavior settings'))
-    ->setDescription(t('The behavior plugin settings'));
-  \Drupal::entityDefinitionUpdateManager()
-    ->installFieldStorageDefinition('behavior_settings', 'paragraph', 'paragraph', $storage_definition);

now you deleted too much., *config entities* don't have field definitions. content entities do. this part you need.

We also need to make sure we initialize it with a correct value. That is a bit tricky, we need to set the initial in the schema, see FileEntityStorageSchema for type.

Install with the old version including demo so you have some content, then update, run updates and make sure you don't get any notices after that. and that status page doesn't report any pending changes for paragraphs.

+++ b/src/ParagraphStorageSchema.php
@@ -34,6 +34,10 @@ class ParagraphStorageSchema extends SqlContentEntityStorageSchema {
+
+    if ($storage_definition->getName() == 'behavior_settings') {
+      $schema['fields']['behavior_settings']['initial'] = 'undefined';
+    }

that is not what I said :)

'undefined' is not a valid serialized string. When I run the update on a site with demo installed and then enable paragraphs_test and enable the two plugins for example for text and then edit a paragraph, I get nasty warnings:

Notice: unserialize(): Error at offset 0 of 9 bytes in Drupal\paragraphs\Entity\Paragraph->getAllBehaviorSettings() (line 152 of modules/paragraphs/src/Entity/Paragraph.php).

Look at how the string looks like for an empty array for a new paragraph that you create, that's the initial you want, something like "a:{}" or so.

Ok, then, lets do this ;)

Commiting. Let's move forward and work in follow-ups to improve the monster. ;-)

I improved documentation a bit and added some explanation prepending each added code logic block.

Still a few questions and if we should improve those things:
- I see lots of public methods. Should we make some protected?
- I see no @internal declaration. Are we ready to guarantee them with long term stability?

The type management uses a checkbox element with a label. I thought we are typically using two separate elements?
I see a hardcoded "a:0:{}". I thought we should do a serialize([]).

I'm not 100% sure about naming, when to use ParagraphsXYZ and when ParagraphXYZ. We use now ParagraphViewBuilder, but ParagraphsBehaviorInterface.

Please discuss and create follow-ups.

In https://www.drupal.org/node/2828506#comment-11788732 Miro mentioned a possible follow up regarding plugin configuration forms. While implementing behaviour pluginsit became clear that some settings cannot live in the per-paragraph configuration form (ie background image field selection in #2835789: Implement background image plugin, starting code).

IMHO providing per applied behaviour configuration forms would make sense. And if an even more global settings level is required, it can be provided by the implementing module independently.

Doh, overlooked #2834319: Add per-paragraph-type configuration

FWIW, this commit broke one of our sites, with this stack (no custom code in it, as you can see):

The website encountered an unexpected error. Please try again later.
Error: Call to a member function bundle() on string in Drupal\paragraphs\ParagraphViewBuilder->buildMultiple() (line 21 of modules/contrib/paragraphs/src/ParagraphViewBuilder.php).
Drupal\paragraphs\ParagraphViewBuilder->buildMultiple(Array)
call_user_func(Array, Array) (Line: 376)
Drupal\Core\Render\Renderer->doRender(Array) (Line: 448)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array) (Line: 490)
Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html', NULL, 1) (Line: 52)
__TwigTemplate_d555d99ccef10bf35a317da66d94259be8c7b20c26e80aa2080e587839bb62aa->doDisplay(Array, Array) (Line: 432)
Twig_Template->displayWithErrorHandling(Array, Array) (Line: 403)
Twig_Template->display(Array) (Line: 411)
Twig_Template->render(Array) (Line: 64)
twig_render_template('modules/contrib/paragraphs/templates/paragraph.html.twig', Array) (Line: 384)
Drupal\Core\Theme\ThemeManager->render('paragraph', Array) (Line: 435)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array) (Line: 490)
Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html', NULL, 1) (Line: 71)
__TwigTemplate_5b03bca200df2d5ba1ca47d2e222b86347639abbdcd0d8d587ebad065efba4c8->doDisplay(Array, Array) (Line: 432)
Twig_Template->displayWithErrorHandling(Array, Array) (Line: 403)
Twig_Template->display(Array) (Line: 411)
Twig_Template->render(Array) (Line: 64)
twig_render_template('core/themes/classy/templates/field/field.html.twig', Array) (Line: 384)
Drupal\Core\Theme\ThemeManager->render('field', Array) (Line: 435)
Drupal\Core\Render\Renderer->doRender(Array) (Line: 448)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array) (Line: 490)
Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html', NULL, 1) (Line: 114)
__TwigTemplate_4ae59022820823361d314fdb0db03eda142ac3295eb1399a7323e963e3d3bf5a->doDisplay(Array, Array) (Line: 432)
Twig_Template->displayWithErrorHandling(Array, Array) (Line: 403)
Twig_Template->display(Array) (Line: 411)
Twig_Template->render(Array) (Line: 64)
twig_render_template('core/themes/bartik/templates/node.html.twig', Array) (Line: 384)
Drupal\Core\Theme\ThemeManager->render('node', Array) (Line: 435)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array, ) (Line: 226)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 227)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object, Object) (Line: 117)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.view', Object) (Line: 149)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 64)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 656)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

I think there's an issue about that somewhere in the queue.

I ran into this error, too, but couldn't find a proper issue. So I created a new one and added a patch: #2867069: Error: Call to a member function bundle() on null in ParagraphViewBuilder.