Add replication settings entity

1. +++ b/config/install/replication.replication_settings.entity_type.article.yml
   @@ -0,0 +1,8 @@
   +parameters:
   +  entity_type: article
   

   'article' seems like a node bundle. Should the parameters be {entity_type: node, bundle: article}?

2. +++ b/config/install/replication.replication_settings.entity_type.page.yml
   @@ -0,0 +1,8 @@
   +parameters:
   +  entity_type: page
   

   Ditto.

3. +++ b/config/schema/replication.replication_settings.schema.yml
   @@ -0,0 +1,16 @@
   +    parameters:
   +      type: array
   +      label: 'Filter parameters'
   

   I'm not sure 'array' is a valid config schema type. Since the parameters are likely to be a free-form array, maybe this should simply be 'ignore'?

4. +++ b/src/Entity/ReplicationSettings.php
   @@ -0,0 +1,68 @@
   + *     "filter_id" = "filter_id"
   

   I don't think filter_id is a valid entity key...should this be removed? Unless I'm missing something, I'm not sure it confers any benefit.

5. +++ b/src/Entity/ReplicationSettings.php
   @@ -0,0 +1,68 @@
   +   * An identifier for this replication settings.
   

   My inner grammar pedant cringed at this. s/this/these?

6. +++ b/src/Entity/ReplicationSettings.php
   @@ -0,0 +1,68 @@
   +   * The human readable name for this replication settings.
   

   Same here.

7. +++ b/src/Entity/ReplicationSettings.php
   @@ -0,0 +1,68 @@
   +  /**
   +   * The replication filter parameters.
   +   *
   +   * @var string[string]
   +   */
   +  protected $parameters;
   

   Is this going to be the configuration of the filter plugin? If so, I'm not sure $parameters needs to be its own thing; we could simply store the configuration of the plugin itself ($this->getFilter()->getConfiguration() or similar), or maybe use a plugin collection.

8. +++ b/src/ReplicationTask/ReplicationTask.php
   @@ -55,7 +55,10 @@ class ReplicationTask implements ReplicationTaskInterface {
   +  public function setParametersByArray(array $parameters_array = NULL) {
   +    if ($parameters_array == NULL) {
   +      $parameters_array = [];
   +    }
   

   Can the default value of $parameters_array simply be [], to avoid the sanity check?

9. +++ b/tests/src/Kernel/ReplicationSettingsTest.php
   @@ -0,0 +1,45 @@
   +  protected $strictConfigSchema = FALSE;
   +
   +  public static $modules = [
   +    'user',
   +    'serialization',
   +    'key_value',
   +    'multiversion',
   +    'replication',
   +  ];
   

   Both of these need @inheritdoc.

Good call on checking that! Per https://www.drupal.org/node/1905070 it looks like I can use 'sequence', correct?

Sequences are used for non-associative sets, like ['foo', 'bar', 'baz']. Since the filter plugin's configuration will almost certainly be associative, and have varying keys and values (depending on the configuration values of the filter plugin in use), I would recommend the use of the ignore type so that validation is skipped entirely for the plugin configuration. There are ways to get it to validate the config values of each plugin, but frankly that's probably more work than we need to do right now.

Confession: I have no idea what "entity_keys" are used for with config entities. I've removed it, however do you have an explanation of their purpose?

Sure. Certain fields of each entity type can have special meaning to Drupal's entity system. For instance, one of the fields is the unique identifier for the entity -- that's the 'id' key. Another one might be the canonical human-readable name of the entity; that's the 'label' key. If the entity can have a unique owner, that'd be its 'uid' key. The entity_keys array in the annotation identifies which fields of the entity map to which keys. There are a bunch of different keys (including 'status', 'uuid', and others), but they are not arbitrary. So filter_id probably does not belong in there, since that's not a known entity key :)

Oh, this is coming along nicely :) I have some nitpicks, as is my wont, plus a few defensive-programming things...

1. +++ b/src/Changes/Changes.php
   @@ -121,12 +121,12 @@ class Changes implements ChangesInterface {
        $parameters = ($this->parameters instanceof ParameterBag) ? $this->parameters : new ParameterBag();
   

   This could be ($this->parameters instanceof ParameterBag) ? $this->parameters->all() : []; then we don't need to bother constructing an empty parameter bag.

   Although this would also require a few changes further down, so maybe not the greatest idea. It's a nitpick in any event.

2. +++ b/src/Changes/Changes.php
   @@ -146,7 +146,7 @@ class Changes implements ChangesInterface {
   +      if ($filter !== NULL && !$filter->filter($revision)) {
   

   if ($filter !== NULL) should probably be if (isset($filter)), in case the variable doesn't exist at all.

3. +++ b/src/Entity/ReplicationSettings.php
   @@ -0,0 +1,67 @@
   +   * @var string[string]
   

   Should be 'array', since there's no real guarantee that every element in $parameters is a string.

4. +++ b/src/Entity/ReplicationSettingsInterface.php
   @@ -0,0 +1,26 @@
   +   * @return string[string]
   

   Same here.

5. +++ b/src/Plugin/ReplicationFilter/EntityTypeFilter.php
   @@ -4,13 +4,13 @@ namespace Drupal\replication\Plugin\ReplicationFilter;
   + *   entity_type_id: a comma delimited list of entity type id's to include
   

   s/id's/IDs

6. +++ b/src/Plugin/ReplicationFilter/EntityTypeFilter.php
   @@ -4,13 +4,13 @@ namespace Drupal\replication\Plugin\ReplicationFilter;
   + *   bundle: a comma delimited list of bundles matching the type ids
   

   s/ids/IDs

7. +++ b/src/Plugin/ReplicationFilter/EntityTypeFilter.php
   @@ -23,16 +23,25 @@ class EntityTypeFilter extends ReplicationFilterBase {
   +    if (count($entity_type_ids) != count($bundles)) {
   +      return FALSE;
   

   If the counts are not equal, does that mean the plugin is misconfigured? If so, maybe we should throw a LogicException here rather than silently returning false, since that could result in some very hard-to-trace bugs.

8. +++ b/src/Plugin/ReplicationFilter/EntityTypeFilter.php
   @@ -23,16 +23,25 @@ class EntityTypeFilter extends ReplicationFilterBase {
   +    for ($i = 0; $i < count($entity_type_ids); $i++) {
   +      if ($entity_type_ids[$i] == $entity_type_id && $bundles[$i] == $bundle) {
   +        return TRUE;
   +      }
        }
   

   It might be simpler (although I don't know if it would be equally or more performant) to do this instead: return in_array($entity->getEntityTypeId(), $entity_type_ids) && in_array($entity->bundle(), $bundles). Just a suggestion.

9. +++ b/src/Plugin/ReplicationFilter/PublishedFilter.php
   @@ -23,9 +22,9 @@ class PublishedFilter extends ReplicationFilterBase {
   +  public function filter(EntityInterface $entity) {
        if (!$entity instanceof NodeInterface) {
   -      return false;
   +      return FALSE;
   

   It appears that this plugin effectively only supports nodes. Because status is a known entity key, we can make this plugin much more generic. All we need to do is inject the entity type manager service into the plugin (using ContainerFactoryPluginInterface), check the entity type definition for a status key, and filter on that if one exists. Let's nip this in the bud and make this plugin generic.

10. +++ b/src/Plugin/ReplicationFilter/ReplicationFilterBase.php
    @@ -37,4 +37,54 @@ abstract class ReplicationFilterBase extends PluginBase implements ReplicationFi
    +   * @return array
    

    Should be string[].

11. +++ b/src/Plugin/ReplicationFilter/ReplicationFilterBase.php
    @@ -37,4 +37,54 @@ abstract class ReplicationFilterBase extends PluginBase implements ReplicationFi
    +    $values = array_filter(array_map('trim', $values));
    

    I'm not sure about the use of array_filter() here. It's entirely possible that FALSE, 0, and '0' -- none of which will pass array_filter() -- could be perfectly valid values. So in the name of future proofing, my preference is to drop the array_filter() call and expect the calling code to remove invalid or empty values.

12. +++ b/src/ReplicationTask/ReplicationTaskInterface.php
    @@ -51,13 +51,13 @@ interface ReplicationTaskInterface {
    +  public function setParametersByArray(array $parameters_array = NULL);
    

    I think we should rename this to setParameters() -- it's going to be pretty intuitive that it expects an array.

13. +++ b/tests/src/Kernel/ReplicationSettingsTest.php
    @@ -0,0 +1,51 @@
    +    $this->assertTrue($entity instanceof ReplicationSettings, 'Replication Settings entity was created.');
    

    Let's use assertInstanceOf() here. $this->assertInstanceOf(ReplicationSettings::class, $entity)

14. +++ b/tests/src/Unit/Plugin/ReplicationFilter/EntityTypeFilterTest.php
    @@ -16,44 +16,61 @@ class EntityTypeFilterTest extends \PHPUnit_Framework_TestCase {
    +   *   The entity type id filter parameter.
    

    s/id/ID

15. +++ b/tests/src/Unit/Plugin/ReplicationFilter/EntityTypeFilterTest.php
    @@ -16,44 +16,61 @@ class EntityTypeFilterTest extends \PHPUnit_Framework_TestCase {
    +   *   The bundle filter parameter.
    

    Maybe this should be "bundle ID".

1. +++ b/src/Plugin/ReplicationFilter/EntityTypeFilter.php
   @@ -4,13 +4,14 @@ namespace Drupal\replication\Plugin\ReplicationFilter;
   +use Symfony\Component\Console\Exception\LogicException;
   

   Whoops, this should be \LogicException (so the entire use statement can go away).

2. +++ b/src/Plugin/ReplicationFilter/EntityTypeFilter.php
   @@ -29,7 +30,7 @@ class EntityTypeFilter extends ReplicationFilterBase {
   +      throw new LogicException('The entity_type_id and bundle filter parameters must have equal length.');
   

   \LogicException. Additionally, I'm told we're not supposed to put periods at the end of exception messages...

3. +++ b/src/Plugin/ReplicationFilter/PublishedFilter.php
   @@ -17,16 +18,47 @@ use Drupal\replication\Plugin\ReplicationFilter\ReplicationFilterBase;
   +  /**
   +   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
   +   */
   +  protected $entityTypeManager;
   

   Missing description.

4. +++ b/src/Plugin/ReplicationFilter/PublishedFilter.php
   @@ -17,16 +18,47 @@ use Drupal\replication\Plugin\ReplicationFilter\ReplicationFilterBase;
   +    if ($definition->get('status')) {
   +      return $entity->status;
        }
   

   This should be something like this:

   if ($status_key = $definition->getKey('status')) return (bool) $entity->$status_key

5. +++ b/src/Plugin/ReplicationFilter/PublishedFilter.php
   @@ -17,16 +18,47 @@ use Drupal\replication\Plugin\ReplicationFilter\ReplicationFilterBase;
   +    // Assume all entities without 'status' are published.
   +    return TRUE;
   

   I wonder if this should be configurable.

Really close now. I'm having trouble finding things to nitpick.

1. +++ b/src/Plugin/ReplicationFilter/EntityTypeFilter.php
   @@ -23,16 +23,35 @@ class EntityTypeFilter extends ReplicationFilterBase {
   +    $types = isset($configuration['types']) ? $configuration['types'] : '';
   +    $types = str_replace(' ', '', $types);
   

   I'm thinking we should maybe make $configuration['types'] always be an array, even if it's just got one value. Totally not a big deal, of course, but I've never been a huge fan of parsing array-or-string configuration options when an array would be perfectly acceptable.

2. +++ b/src/Plugin/ReplicationFilter/PublishedFilter.php
   @@ -3,14 +3,18 @@
   + * Use the configuration "include_unpublisheable_entities" to determine what
   

   Should be "include_unpublishable_entities". (Drop the extra E.)

3. +++ b/src/Plugin/ReplicationFilter/PublishedFilter.php
   @@ -3,14 +3,18 @@
   + * will be included by the filter, else excluded.
   + * with values in the format
   

   Looks like there might be a rogue line in there :)

4. +++ b/src/Plugin/ReplicationFilter/PublishedFilter.php
   @@ -18,16 +22,63 @@ use Symfony\Component\HttpFoundation\ParameterBag;
   +  /**
   +   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
   +   *   The entity type manager to check for "status" entity key.
   +   */
   

   The description should come before the @var line, and they should be separated by a blank line.

5. +++ b/src/Plugin/ReplicationFilter/UuidFilter.php
   @@ -24,8 +25,9 @@ class UuidFilter extends ReplicationFilterBase {
   +    $configuration = $this->getConfiguration();
   +    $uuids = isset($configuration['uuids']) ? $configuration['uuids'] : '';
   

   As I mentioned before, is there any real reason beyond CouchDB compliance to not make $configuration['uuids'] an array in all cases?

6. +++ b/tests/src/Kernel/Plugin/ReplicationFilter/PublishedFilterTest.php
   @@ -0,0 +1,134 @@
   +    $entity = call_user_func($entity_class . '::create', $entity_values);
   

   A bit unorthodox -- I think the preferred/"traditional" way is probably to call $this->container('entity_type.manager')->getStorage($entity_type)->create($entity_values)...but this will do fine as well :) Besides, no entity type parameter is being passed in.

7. +++ b/tests/src/Kernel/Plugin/ReplicationFilter/PublishedFilterTest.php
   @@ -0,0 +1,134 @@
   +    $this->assertEquals($expected, $value);
   

   Let's do assertSame() here to avoid potential snafus over truthy and falsy $expected values.

8. +++ b/tests/src/Kernel/Plugin/ReplicationFilter/PublishedFilterTest.php
   @@ -0,0 +1,134 @@
   +  public function testDefaultConfig($include_unpublisheable_entities, $entity_class, $entity_values, $expected) {
   

   This method has no data provider, so why does it have parameters at all?

1. +++ b/config/install/replication.replication_settings.published.yml
   @@ -0,0 +1,8 @@
   +  include_unpublishable_entities: FALSE
   

   I could be wrong about this, but is YAML case-sensitive? If it is, then this needs to be lowercase 'false'. I know YAML is a superset of JSON, but I don't know if that implies case-sensitivity.

2. +++ b/src/ReplicationTask/ReplicationTask.php
   @@ -11,25 +10,20 @@ use Symfony\Component\HttpFoundation\ParameterBag;
   +   * The id of the filter plugin to use during replication.
   

   Nit: id should be ID. I'm not sure I want to hold this patch up longer over that, though :) Could be fixed on commit.

3. +++ b/src/ReplicationTask/ReplicationTask.php
   @@ -55,18 +52,9 @@ class ReplicationTask implements ReplicationTaskInterface {
   +    if (!is_array($this->parameters)) {
   +      $this->setParameters([]);
        }
        $this->parameters->set($name, $value);
        return $this;
   

   How is it possible that we're doing $this->parameters->set() if $this->parameters is an array? Won't this die with a fatal error?

4. +++ b/tests/src/Functional/ReplicationFilterTest.php
   @@ -70,9 +76,9 @@ class ReplicationFilterTest extends WebTestBase {
   +    $this->assertEqual(1, count($changes), 'Expect there is 1 entity in the changeset for UUIDs filter.');
   

   No need to change this for now, but for the record, you can use the slightly more convenient assertCount(), like so:

   $this->assertCount(1, $changes, 'There is one change')

5. +++ b/tests/src/Kernel/Plugin/ReplicationFilter/PublishedFilterTest.php
   @@ -0,0 +1,136 @@
   +   *   The entity type id of the entity to create for testing.
   

   s/id/ID. Fixable on commit, if we want to nitpick.

6. +++ b/tests/src/Kernel/Plugin/ReplicationFilter/PublishedFilterTest.php
   @@ -0,0 +1,136 @@
   +   *   The values to pass to $class::create().
   

   This doc comment no longer makes sense, since we're using the storage handler's create() method. How about something like "The values with which to create the entity"?

7. +++ b/tests/src/Kernel/ReplicationSettingsTest.php
   @@ -0,0 +1,51 @@
   +    $this->assertInstanceOf(ReplicationSettings::class, $entity, 'Replication Settings entity was created.');
   

   I don't think this assertion adds much value, since we're just testing the output of the entity storage handler, which is covered by core tests. I would recommend we do something like this, if we want to bother keeping it at all:

   $saved = $entity->saved(); $this->assertNotEmpty($saved);

Me likey. And @josephdpurcell deserves a rest for his troubles :) RTBC!

Thank you @josephdpurcell and @phenaproxima for your work on this issue!