Add DefaultConfigValue MigrateProcessPlugin

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/DefaultConfigValue.php
   @@ -0,0 +1,59 @@
   +/**
   + * @file
   + * Contains \Drupal\migrate\Plugin\migrate\process\DefaultConfigValue.
   + */
   

   un-needed comment.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/DefaultConfigValue.php
   @@ -0,0 +1,59 @@
   + * This plugin sets missing values on the destination using a value from configuration.
   

   > 80 chars.

3. +++ b/core/modules/migrate/src/Plugin/migrate/process/DefaultConfigValue.php
   @@ -0,0 +1,59 @@
   +  protected $container;
   +
   +  /**
   +   * Set the container.
   +   *
   +   * @param $container
   +   */
   +  public function setContainer($container) {
   ...
   +  /**
   +   * Get the container.
   +   *
   +   * @return null|\Symfony\Component\DependencyInjection\ContainerInterface
   +   */
   +  public function getContainer() {
   +    if (!isset($this->container)) {
   +      $this->container = \Drupal::getContainer();
   +    }
   +    return $this->container;
   +  }
   

   This is wrong, we need to use ContainerInjectionInterface and inject just the dependencies we need. See Drupal\block\Plugin\migrate\process\BlockVisibility for an example.

4. +++ b/core/modules/migrate/tests/src/Unit/process/DefaultConfigValueTest.php
   @@ -0,0 +1,109 @@
   +/**
   + * @file
   + * Contains \Drupal\Tests\migrate\Unit\process\DefaultConfigTest.
   + */
   

   un-needed comment

Patch looks good, just one small nitpick and then RTBC.

+++ b/core/modules/migrate/src/Plugin/migrate/process/DefaultConfigValue.php
@@ -0,0 +1,60 @@
+  public function __construct(ConfigFactory $config_factory, array $configuration, $plugin_id, $plugin_definition) {
...
+      $container->get('config.factory'),

Normally, we inject new parameters at the end.

+++ b/core/modules/migrate/tests/src/Unit/process/DefaultConfigValueTest.php
@@ -0,0 +1,93 @@
+  protected function assertProcess($original_value, $expected_value) {
...
+  protected function getConfigFactory() {

Missing comments, bit pointless I know but we need them for phpcs.

Sorry to keep pushing this back on docs but the committers won't commit it unless it's right as there is an effort to make Drupal core pass an automated coding standard check. It's worthwhile making yourself familiar with the standards: https://www.drupal.org/coding-standards

+++ b/core/modules/migrate/tests/src/Unit/process/DefaultConfigValueTest.php
@@ -0,0 +1,104 @@
+   * @param $original_value
+   * @param $expected_value

These need comments and types.

+++ b/core/modules/migrate/tests/src/Unit/process/DefaultConfigValueTest.php
@@ -0,0 +1,104 @@
+   * @return \PHPUnit_Framework_MockObject_MockObject

This needs a comment.

Thanks for sticking with it, I think this is ready. I'd be happy for this to go in core although I can't ever see it been used in the upgrade path from d6/d7 it will be useful for others using the generic Migrate API.

+++ b/core/modules/migrate/tests/src/Unit/process/DefaultConfigValueTest.php
@@ -0,0 +1,107 @@
+   * @param string|boolean|NULL $original_value
...
+   * @param string|boolean $expected_value

This could probably be fixed on commit per: https://www.drupal.org/coding-standards/docs#types it needs to be bool and not boolean.

diff --git a/core/modules/migrate/tests/src/Unit/process/DefaultConfigValueTest.php b/core/modules/migrate/tests/src/Unit/process/DefaultConfigValueTest.php
index 3e213b5..344809f 100644
--- a/core/modules/migrate/tests/src/Unit/process/DefaultConfigValueTest.php
+++ b/core/modules/migrate/tests/src/Unit/process/DefaultConfigValueTest.php
@@ -66,9 +66,9 @@ public function testWithoutStrict() {
   /**
    * Carry out a process and assert that the correct value is returned.
    *
-   * @param string|boolean|NULL $original_value
+   * @param string|bool|NULL $original_value
    *   The original, pre-process value.
-   * @param string|boolean $expected_value
+   * @param string|bool $expected_value
    *   The expected post-process value.
    */
   protected function assertProcess($original_value, $expected_value) {

1. +++ b/core/modules/migrate/tests/src/Unit/process/DefaultConfigValueTest.php
   @@ -0,0 +1,107 @@
   +  /**
   +   * Test DefaultConfigValue works with strict set.
   +   */
   +  public function testWithStrict() {
   +    $this->assertProcess(NULL, 'foo');
   +    $this->assertProcess(TRUE, TRUE);
   +    $this->assertProcess(FALSE, FALSE);
   +    $this->assertProcess('bar', 'bar');
   +  }
   

   This type of test looks like it could use the @dataProvider pattern.

2. +++ b/core/modules/migrate/tests/src/Unit/process/DefaultConfigValueTest.php
   @@ -0,0 +1,107 @@
   +    $configuration = [
   +      'config_object' => $this->configObjectId,
   +      'config_key' => $this->configKeyId,
   +    ];
   +    $this->plugin = new DefaultConfigValue($configuration, 'default_config_value', [], $this->getConfigFactory());
   

   This undoes what setUp is doing - not sure the setUp method is necessary should just be part of the test that needs it.

3. +++ b/core/modules/migrate/tests/src/Unit/process/DefaultConfigValueTest.php
   @@ -0,0 +1,107 @@
   +    $this->assertProcess(NULL, 'foo');
   +    $this->assertProcess(TRUE, TRUE);
   +    $this->assertProcess(FALSE, 'foo');
   +    $this->assertProcess('bar', 'bar');
   

   Again could use the @dataProvider pattern.

4. +++ b/core/modules/migrate/tests/src/Unit/process/DefaultConfigValueTest.php
   @@ -0,0 +1,107 @@
   +  /**
   +   * Provide a mock ConfigFactory.
   +   *
   +   * @return \PHPUnit_Framework_MockObject_MockObject
   +   *   The mocked Config Factory.
   +   */
   +  protected function getConfigFactory() {
   +    $config_data = $this->getMockBuilder('Drupal\Core\Config\ImmutableConfig')
   +      ->disableOriginalConstructor()
   +      ->setMethods(array('get'))
   +      ->getMock();
   +    $config_data->expects($this->any())
   +      ->method('get')
   +      ->with($this->equalTo($this->configKeyId))
   +      ->will($this->returnValue('foo'));
   +
   +    $confing_factory = $this->getMockBuilder('Drupal\Core\Config\ConfigFactory')
   +      ->disableOriginalConstructor()
   +      ->setMethods(array('get'))
   +      ->getMock();
   +    $confing_factory->expects($this->any())
   +      ->method('get')
   +      ->with($this->equalTo($this->configObjectId))
   +      ->will($this->returnValue($config_data));
   +
   +    return $confing_factory;
   +  }
   

   I think this could be replaced with $this->getConfigFactoryStub() in setUp()

Here's a patch showing what I'm on about. Also discovered that the process plugin was typehinting on ConfigFactory and not ConfigFactoryInterface (as it should be).

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/DefaultConfigValue.php
   @@ -0,0 +1,60 @@
   +    if (!empty($this->configuration['strict'])) {
   

   Why is there both a strict and not strict version? Can you give an example where each is needed?

2. +++ b/core/modules/migrate/tests/src/Unit/process/DefaultConfigValueTest.php
   @@ -0,0 +1,62 @@
   +  public function testTransform($value, $expected, array $configuration) {
   

   Does this need @param in the docblock?

3. +++ b/core/modules/migrate/tests/src/Unit/process/DefaultConfigValueTest.php
   @@ -0,0 +1,62 @@
   +  public function providerTestTransform() {
   

   I think it would be clearer to also include tests with different results. That is, have a source of empty string or 0.

Removed the tag since this is not D7->D8 specific.

The more of think of this issue the more it unravels.

1. At least the config value should be a process plugin in itself and then either default_value needs to be amended or we need to add a new plugin default_value_from_property. Welding these two plugins into one looks like an owie (which quietone already started to feel when asking about strict -- I would bet it's just a copy-paste of DefaultValue::transform() and that's it).
2. The process plugin which gets a default from a property needs the Get plugin injected so that you can use both source and destination properties freely.
3. Then of course StaticMap has a default value but it's slightly different semantics because it's not triggered by any kind of emptiness but by failing to map. Still, it could reuse the default value plugin and for this reason expanding the capabilities of DefaultValue and injecting it into StaticMap is preferred.
4. And then we didn't even begin to think that the new process plugin which loads from config is not a process plugin but rather a funnier kind of source constant so I would prefer it to be a value adding plugin from #2543552: Modernize migration source plugins.

I am of two minds about this, but because it's basically an API improvement at this point and not (to my knowledge) blocking anything critical, I find myself leaning more in the direction of what @chx proposes.

I would suggest that we change DefaultValue to support this behavior. Let's add two new configuration options: from_property and from_config.

from_property would utilize an injected Get plugin instance and could therefore be used this way:

# Gets a source row property
from_property: some_source_property
# Gets a destination row property
from_property: '@some_destination_property'

from_config would use an injected ConfigFactoryInterface and would be configured thusly:

from_config: 'system.site:configuration.property.path'

What do y'all think?

If you think this change is obviated, then we can wontfix this. Thank you! :)