Changes to config entities that use plugins are not propagated to the plugins

I'm going to work on a more generic version of this, since we need this fix whenever a config entity has a ConfigurablePluginInterface.
Since unlike most other plugins, BlockInterface always implements ConfigurablePluginInterface, it is the easiest example.

I do have a couple questions though:

1. Why would we delete Block::$settings, as stated in the OP?
2. Why aren't StorageComparer and ConfigImporter services?

Here's the rest of the entities using a single plugin, I couldn't figure out image styles tonight.
Remaining: FilterFormat, Tour, ImageStyle, View.

There's a fair chance that not all of them are broken the same, but we should have tests either way.

Just fixing the block test for now.

Hmm, I wrote the filter format test, which should have failed... But it didn't (well except for a schema clash!)
Sooo, not sure what to do here.

Same deal. It seems entities with multiple plugins work just fine?
I haven't really stopped to think through why, just wrote these tests while watching some Olympics :)

It turns out that the config import bug is actually a very simple one, but is not the actual bug we were all seeing and expecting.
Entities that always instantiate their plugin bag in their __construct will break on config import, those that check for an existing instance first will not.

But the addition of code to set() is still needed, and across the board, and I finally came up with some tests to prove it.

After further discussion today in IRC with @pwolanin and @EclipseGc, I went ahead and genericized the implementation.
It's a little ugly right now, but it can be much cleaner with traits.

I'll update the issue summary in a bit, but here's the patch.

I also consolidated the test coverage after discussion in IRC.

Addressed the test failures.

1. +++ b/core/modules/block/lib/Drupal/block/BlockPluginBag.php
   @@ -27,8 +27,8 @@ class BlockPluginBag extends DefaultSinglePluginBag {
       * {@inheritdoc}
   ...
   +  public function __construct(PluginManagerInterface $manager, $instance_id, array $configuration, $block_id) {
   +    parent::__construct($manager, $instance_id, $configuration);
   

   parent has no $block_id param so use of {@inheritdoc} is wrong.

2. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/ConfigEntityImportTest.php
   @@ -0,0 +1,241 @@
   +  public function testConfigUpdateImport() {
   

   doc block missing.

BlockPluginBag was missing docs before this patch, and I don't change that param *at all*. But I'm going to fix it now just so we never have to talk about it ever again :)
Fixed the other comment.

The PluginBag changes look solid and sane. I like them. All the entity stuff though is out of my ballpark, someone else will have to RTBC that.

Nice work!

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -192,6 +215,14 @@ public function getExportProperties() {
   +      // Any changes to the plugin configuration must be saved in settings, but
   ...
   +      $this->{$this->pluginConfigKey} = $this->getPluginBag()->getConfiguration();
   

   Can we explain why we want to avoid the extra call to setConfiguration? I can't imagine that the reason is performance.

2. +++ b/core/lib/Drupal/Core/Config/Entity/EntityWithPluginBagInterface.php
   @@ -0,0 +1,28 @@
   diff --git a/core/lib/Drupal/Core/Entity/EntityFormController.php b/core/lib/Drupal/Core/Entity/EntityFormController.php
   
   diff --git a/core/lib/Drupal/Core/Entity/EntityFormController.php b/core/lib/Drupal/Core/Entity/EntityFormController.php
   index 440aa50..58d9035 100644
   
   index 440aa50..58d9035 100644
   --- a/core/lib/Drupal/Core/Entity/EntityFormController.php
   
   --- a/core/lib/Drupal/Core/Entity/EntityFormController.php
   +++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
   
   +++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
   +++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
   @@ -359,15 +359,7 @@ public function buildEntity(array $form, array &$form_state) {
   
   @@ -359,15 +359,7 @@ public function buildEntity(array $form, array &$form_state) {
        // controller of the current request.
        $form_state['controller'] = $this;
    
   -    // Copy top-level form values to entity properties, without changing
   -    // existing entity properties that are not being edited by
   -    // this form.
   -    // @todo: This relies on a method that only exists for config and content
   -    //   entities, in a different way. Consider moving this logic to a config
   -    //   entity specific implementation.
   -    foreach ($form_state['values'] as $key => $value) {
   -      $entity->set($key, $value);
   -    }
   +    $this->copyFormValuesToEntity($entity, $form_state);
    
        // Invoke all specified builders for copying form values to entity
        // properties.
   @@ -381,6 +373,26 @@ public function buildEntity(array $form, array &$form_state) {
   
   @@ -381,6 +373,26 @@ public function buildEntity(array $form, array &$form_state) {
      }
    
      /**
   +   * Copies top-level form values to entity properties
   +   *
   +   * This should not change existing entity properties that are not being edited
   +   * by this form.
   +   *
   +   * @param \Drupal\Core\Entity\EntityInterface $entity
   +   *   The entity the current form should operate upon.
   +   * @param array $form_state
   +   *   An associative array containing the current state of the form.
   +   */
   +  protected function copyFormValuesToEntity(EntityInterface $entity, array $form_state) {
   +    // @todo: This relies on a method that only exists for config and content
   +    //   entities, in a different way. Consider moving this logic to a config
   +    //   entity specific implementation.
   +    foreach ($form_state['values'] as $key => $value) {
   +      $entity->set($key, $value);
   +    }
   +  }
   +
   +  /**
       * {@inheritdoc}
       */
      public function getEntity() {
   

   How did that ended up in the patch? This is a change I don't understand in the context of the patch, but sure this does not block the patch.

3. +++ b/core/modules/block/lib/Drupal/block/BlockPluginBag.php
   @@ -26,9 +26,18 @@ class BlockPluginBag extends DefaultSinglePluginBag {
      /**
       * {@inheritdoc}
   +   *
   +   * @param \Drupal\Component\Plugin\PluginManagerInterface $manager
   +   *   The manager to be used for instantiating plugins.
   +   * @param string $instance_id
   +   *   The ID of the plugin instance.
   +   * @param array $configuration
   +   *   An array of configuration.
   +   * @param string $block_id
   +   *   The unique ID of the block entity using this plugin.
       */
   

   If we change this, we should probably also remove {@inheritdoc} as it is totally wrong here, to be honest.

4. +++ b/core/modules/block/lib/Drupal/block/Entity/Block.php
   @@ -100,19 +106,20 @@ class Block extends ConfigEntityBase implements BlockInterface {
   +      $this->pluginBag = new BlockPluginBag(\Drupal::service('plugin.manager.block'), $this->plugin, $this->get('settings'), $this->id());
   

   Should we put the BlockPluginManager in its own method?

5. +++ b/core/modules/image/lib/Drupal/image/Form/ImageStyleEditForm.php
   @@ -248,4 +249,16 @@ protected function updateEffectWeights(array $effects) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function copyFormValuesToEntity(EntityInterface $entity, array $form_state) {
   +    foreach ($form_state['values'] as $key => $value) {
   +      // Do not copy effects here, see self::updateEffectWeights().
   +      if ($key != 'effects') {
   +        $entity->set($key, $value);
   +      }
   +    }
   +  }
   +
   

   AAAAAAAAAAAAAAh

6. +++ b/core/modules/search/lib/Drupal/search/Plugin/SearchPluginBag.php
   @@ -25,8 +25,8 @@ class SearchPluginBag extends DefaultSinglePluginBag {
      /**
       * {@inheritdoc}
       */
   -  public function __construct(PluginManagerInterface $manager, array $instance_ids, array $configuration, $search_page_id) {
   -    parent::__construct($manager, $instance_ids, $configuration);
   +  public function __construct(PluginManagerInterface $manager, $instance_id, array $configuration, $search_page_id) {
   +    parent::__construct($manager, $instance_id, $configuration);
   

   Should we change the documentation here as well?

1) Talked about it in IRC, calling set() is more consistent even if a bit overkill.
2) I think you answered this question in 5 :)
3) Whoops! Agreed.
4) I don't think that's necessary, we don't do that in any of the other entities. When it's only being used by a `new $pluginBagClass()` call anyway, it doesn't really affect much.

6) Fixed.

Thank you!

Added https://drupal.org/node/2203617.

I made https://drupal.org/node/2203633 its own change record because it will require ported modules to change.

As far as updating existing change records...

• https://drupal.org/node/2190891 is still a draft?! That needs to be updated once this goes in.
• https://drupal.org/node/1878416 needs to be updated once this goes in.

ConfigEntityImportTest is a nice test!

Committed 0230aa2 and pushed to 8.x. Thanks!

We need to check existing change notices to ensure they are correct after this fix. Maybe https://drupal.org/node/1878416 needs work.

Removed some use statements left lying around.

diff --git a/core/modules/block/lib/Drupal/block/Entity/Block.php b/core/modules/block/lib/Drupal/block/Entity/Block.php
index 15f0dfa..d4e3039 100644
--- a/core/modules/block/lib/Drupal/block/Entity/Block.php
+++ b/core/modules/block/lib/Drupal/block/Entity/Block.php
@@ -11,7 +11,6 @@
 use Drupal\block\BlockPluginBag;
 use Drupal\block\BlockInterface;
 use Drupal\Core\Config\Entity\EntityWithPluginBagInterface;
-use Drupal\Core\Entity\EntityStorageControllerInterface;
 
 /**
  * Defines a Block configuration entity class.
diff --git a/core/modules/system/lib/Drupal/system/Entity/Action.php b/core/modules/system/lib/Drupal/system/Entity/Action.php
index 69dd4d4..a487433 100644
--- a/core/modules/system/lib/Drupal/system/Entity/Action.php
+++ b/core/modules/system/lib/Drupal/system/Entity/Action.php
@@ -9,8 +9,6 @@
 
 use Drupal\Core\Config\Entity\ConfigEntityBase;
 use Drupal\Core\Config\Entity\EntityWithPluginBagInterface;
-use Drupal\Core\Entity\Entity;
-use Drupal\Core\Entity\EntityStorageControllerInterface;
 use Drupal\system\ActionConfigEntityInterface;
 use Drupal\Core\Action\ActionBag;
 use Drupal\Component\Plugin\ConfigurablePluginInterface;