Support ReplicationTask

Merged this, but still needs tests.

Can I suggest using BrowserTestBase like https://github.com/timmillwood/drupal-workspace/blob/8.x-1.x/tests/src/F... does?

The UID issue might be a deeper bug. We don't replicate users anymore, but instead try to match on UUID or username, I guess this is failing somewhere in the serializer.

1. +++ b/src/EventSubscriber/WorkbenchModerationSubscriber.php
   @@ -79,8 +80,16 @@ class WorkbenchModerationSubscriber implements EventSubscriberInterface {
   +    $replication_settings = $workspace->get('push_replication_settings')->referencedEntities();
   +    $replication_settings = count($replication_settings) > 0 ? reset($replication_settings) : NULL;
   +    if ($replication_settings !== NULL) {
   +      $task->setFilter($replication_settings->getFilterId());
   +      $task->setParametersByArray($replication_settings->getParameters());
   +    }
   

   Why have a $replication_settings flag? Why not simply do something like this:

   if (count($replication_settings) > 0) { $replication_settings[0] }
   

2. +++ b/src/Form/UpdateForm.php
   @@ -89,9 +90,22 @@ class UpdateForm extends ConfirmFormBase {
   +      $replication_settings = $active
   +        ->getWorkspace()
   +        ->get('push_replication_settings')
   +        ->referencedEntities();
   +      $replication_settings = count($replication_settings) > 0 ? reset($replication_settings) : NULL;
   +      if ($replication_settings !== NULL) {
   +        $task->setFilter($replication_settings->getFilterId());
   +        $task->setParametersByArray($replication_settings->getParameters());
   +      }
   

   This is pretty close to what's in WorkbenchModerationSubscriber. Can it be moved into a utility method for DRY purposes?

3. +++ b/src/Form/UpdateForm.php
   @@ -89,9 +90,22 @@ class UpdateForm extends ConfirmFormBase {
          $response = \Drupal::service('workspace.replicator_manager')->update(
            $upstream,
   -        $active
   +        $active,
   +        $task
   

   A bit out of scope, but the replicator_manager service should be injected if this is a form (FormBase implements ContainerInjectionInterface).

4. +++ b/src/Plugin/RulesAction/ReplicateContent.php
   @@ -99,9 +100,17 @@ class ReplicateContent extends RulesActionBase implements ContainerFactoryPlugin
   +    $replication_settings = $source->get('push_replication_settings')->referencedEntities();
   +    $replication_settings = count($replication_settings) > 0 ? reset($replication_settings) : NULL;
   +    if ($replication_settings !== NULL) {
   +      $task->setFilter($replication_settings->getFilterId());
   +      $task->setParametersByArray($replication_settings->getParameters());
   +    }
   

   Again, this is repeating code. Can that be fixed?

5. +++ b/src/ReplicatorManager.php
   @@ -52,14 +53,31 @@ class ReplicatorManager implements ReplicatorInterface {
   +    $replication_settings = $source
   +      ->getWorkspace()
   +      ->get('pull_replication_settings')
   +      ->referencedEntities();
   +    $replication_settings = count($replication_settings) > 0 ? reset($replication_settings) : NULL;
   +    if ($replication_settings !== NULL) {
   +      $pull_task->setFilter($replication_settings->getFilterId());
   +      $pull_task->setParametersByArray($replication_settings->getParameters());
   +    }
   

   Ditto here.

6. +++ b/src/ReplicatorManager.php
   @@ -52,14 +53,31 @@ class ReplicatorManager implements ReplicatorInterface {
   +    $pull_log = $this->update($target, $source, $pull_task);
   +
   +    // @todo use $post_conflicts in a conflict management workflow
   

   How $pull_log used? It doesn't look like it's ever returned or touched further in this method.

@josephdpurcell Tests are failing in Travis too: https://travis-ci.org/timmillwood/drupal-workspace/jobs/150725700

@josephdpurcell Thank you for all your awesome work on this!

Addressed all point from #22.

@josephdpurcell Tests on github are failing because it's testing with 8.x-1.x branch for Replication module and this branch doesn't contain the replication_settings entity type from: https://github.com/relaxedws/drupal-replication/pull/12/files yet.

@josephdpurcell you can modify the drupal-8.1.x.make.yml and drupal-8.2.x.make.yml files (in https://github.com/timmillwood/drupal-workspace/pull/7) to test with your branch (from Replication).

@josephdpurcell Please commit changes from this patch to your branch on github: https://github.com/timmillwood/drupal-workspace/pull/7

Nice work, @jeqq. This is already a lot cleaner and more reusable. I have two points, though:

1. +++ b/src/ReplicatorManager.php
   @@ -83,0 +77,22 @@
   +   * @param \Drupal\multiversion\Entity\WorkspaceInterface $workspace
   

   There's nothing about this method that flat-out /requires/ the replication settings source to be a workspace, so let's change this to EntityInterface, simply as a risk-free, pre-emptive defense against bizarre edge cases).

   Also, the parameter needs a description.

2. +++ b/src/ReplicatorManager.php
   @@ -83,0 +77,22 @@
   +  public function getTask(WorkspaceInterface $workspace, $type) {
   

   I'd rather we passed the full field name in here, rather than the 'push' or 'pull' prefix -- just in case the field has a completely different name. Again, that's an edge case, but if it's easy for us to nip those in the bud then let's do that.

This looks great! My only complaints are really just nit-scale.

1. +++ b/src/EventSubscriber/WorkbenchModerationSubscriber.php
   @@ -108,11 +110,12 @@ class WorkbenchModerationSubscriber implements EventSubscriberInterface {
   +  static public function getSubscribedEvents() {
   

   Nit: For consistency, let's make this public static rather than static public :)

2. +++ b/src/ReplicatorManager.php
   @@ -52,14 +55,49 @@ class ReplicatorManager implements ReplicatorInterface {
   +   * @param \Drupal\multiversion\Entity\EntityInterface $entity
   

   Should be \Drupal\Core\Entity\EntityInterface.

3. +++ b/tests/src/Functional/ReplicationSettingsTest.php
   @@ -0,0 +1,187 @@
   +  /**
   +   * @var \Drupal\workspace\ReplicatorManager
   +   */
   +  protected $replicatorManager;
   

   Needs description.

4. +++ b/tests/src/Functional/ReplicationSettingsTest.php
   @@ -0,0 +1,187 @@
   +  public static $modules = [
   

   Needs @inheritdoc.

5. +++ b/tests/src/Functional/WorkspaceTestUtilities.php
   @@ -195,5 +195,18 @@ trait WorkspaceTestUtilities {
   +    $session = $this->getSession();
   +    $this->assertEquals(200, $session->getStatusCode());
   

   I think you can use $this->assertSession()->statusCodeEquals(200) here...

6. +++ b/tests/src/Functional/WorkspaceTestUtilities.php
   @@ -195,5 +195,18 @@ trait WorkspaceTestUtilities {
   +    $page = $session->getPage();
   +    return $page->hasContent($label);
   

   This isn't an assertion; are you sure you don't want to use one? I think there's got to be something in $this->assertSession() which would work...

@josephdpurcell: Please make sure to reroll the patch against the latest 8.x-1.x codebase, your patch doesn't apply.

As far as I'm concerned, this is RTBC.

A minor change to use the correct replacement for assertResponse() (and instead of the assertEquals() calls that were being used).

The test failures boil down to this one error:

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "replication_settings" entity type does not exist.

Thoughts?

So while comment #14 says "The tests are passing on GitHub: https://github.com/timmillwood/drupal-workspace/pull/7" we wouldn't want to commit a patch that would break every test run from then on.

Maybe the Replication module should be enabled as part of the tests?

@DamienMcKenna This will fail on d.o because this functionality depend on other new functionality in Replication module (it's not yet commited). See #24. Now we are testing this on github: https://github.com/timmillwood/drupal-workspace/pull/7

@jeqq: Oh, grr :( Hopefully the d.o repo will be synchronized soon.

So, lets ignore the patches in #48.

I have one defensive concern and one nitpick, and once these are addressed this is RTBC.

1. +++ b/src/ReplicatorManager.php
   @@ -52,14 +55,49 @@ class ReplicatorManager implements ReplicatorInterface {
   +    if ($items instanceof EntityReferenceFieldItemListInterface) {
   +      $referenced_entities = $items->referencedEntities();
   +      if (count($referenced_entities) > 0) {
   +        $task->setFilter($referenced_entities[0]->getFilterId());
   +        $task->setParametersByArray($referenced_entities[0]->getParameters());
   +      }
   +    }
   

   If $items is not valid, do we still want to return an empty ReplicationTask? It seems like we might want to fail with an exception here.

2. +++ b/tests/src/Functional/ReplicationSettingsTest.php
   @@ -0,0 +1,191 @@
   +  /**
   +   * @var \Drupal\workspace\ReplicatorManager
   +   *   The replicator manager service used to trigger replication.
   +   */
   

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

@josephdpurcell the test is passing now.

I've tried to do manual testing. Push replication works fine but pull replication (update) fails with this error:

Notice: Undefined property: Drupal\workspace\Form\UpdateForm::$replicatorManager in Drupal\workspace\Form\UpdateForm->submitForm() (line 93 of /var/www/drupal8dev/modules/workspace/src/Form/UpdateForm.php)

+++ b/src/Form/UpdateForm.php
@@ -86,13 +86,14 @@ class UpdateForm extends ConfirmFormBase {
+    $replication_manager = \Drupal::service('workspace.replicator_manager');

I think replicator manager should be injected here, FormBase implements ContainerInjectionInterface.

1. +++ b/src/Form/UpdateForm.php
   @@ -7,21 +7,85 @@ use Drupal\Core\Ajax\CloseModalDialogCommand;
   +      $container->get('renderer')
   

   This is injected, and passed into the constructor, but $this->renderer is never actually set in __construct()...?

2. +++ b/src/Form/UpdateForm.php
   @@ -102,16 +166,22 @@ class UpdateForm extends ConfirmFormBase {
       * @param array $form
   +   *   The form object.
   

   $form is not an object. :)

3. +++ b/src/Form/UpdateForm.php
   @@ -102,16 +166,22 @@ class UpdateForm extends ConfirmFormBase {
   +   *   The ajax response.
   

   s/ajax/AJAX or Ajax

The patch looks great. I love it. Excellent work, @josephdpurcell!

The tests are only failing because this patch needs the HEAD of various other modules, but Drupal CI doesn't support that yet. As long as the tests pass on GitHub -- and they do -- we can march happily forward. It is my pleasure to mark this RTBC.

1. I get this error when I try to install the module, seems that there are missing some references:
   

   PHP Fatal error:  Declaration of Drupal\workspace\Form\UpdateForm::create() must be compatible with Drupal\Core\DependencyInjection\ContainerInjectionInterface::create(Symfony\Component\DependencyInjection\ContainerInterface $container) in /var/www/drupal8dev/modules/workspace/src/Form/UpdateForm.php on line 205
   

   Tests in Travis are also failing because of this problem (some of them don't return the correct status): https://travis-ci.org/timmillwood/drupal-workspace/builds/152436700.

2. +++ b/src/Form/UpdateForm.php
   @@ -7,21 +7,86 @@ use Drupal\Core\Ajax\CloseModalDialogCommand;
   +   *   Tthe replicator manager.
   

   A typo here.

I'll also do manual testing of the functionality.

Update:

After manual testing I've detected that replication doesn't work correctly when doing deploy (with Deploy module). The problem is in ReplicatorManager::replicate(), before doing push replication (this line: $push_log = $this->doReplication($source, $target, $task);) we should define the task if it is null. Something like this:

    if ($task === NULL) {
      // Derive a replication task from the source Workspace for pushing.
      $task = $this->getTask($source->getWorkspace(), 'push_replication_settings');
    }

    // Push changes from $source to $target.
    $push_log = $this->doReplication($source, $target, $task);

@josephdpurcell I've missed your last message from #96 yesterday. When I tested it, the push replication didn't work. For example when doing it like this (an example from Deploy module ReplicationForm::save()):

\Drupal::service('workspace.replicator_manager')->replicate($source, $target);

That because the task is null in this case. It looks obvious to me because we should check for replication settings both on pull and push replication (we do both replication types in ReplicatorManager::replicate(), but check for settings just on pull). If the task is null, in InternalReplicator::replicate() it defines a new task without replication setting.

My solution from #95 fixes this. Does this make sense?

@josephdpurcell I've described all steps to reproduce the problem and attached the patch with the proposed solution.

1. Clean Drupal install (8.2.x)
2. Apply this patch for Workspace
3. Enable Deploy module (dev versions for all dependencies).
4. Go to Live workspace edit page (admin/structure/workspace/1/edit):

• Assign default target workspace: Stage
• Replication settings on update: Replicate only page entities
• Replication settings on deploy: Replicate only article entities
• Save

5. Go to Stage workspace edit page (admin/structure/workspace/2/edit), keep defaults:

• Assign default target workspace: Live
• Replication settings on update: None
• Replication settings on deploy: None
• Save

6. On Live workspace add one article and one page.
7. On Stage workspace add one article and one page.
8. Go on Live workspace:

• Click on Update link (top menu right): Confirm that you want to pull changes from Stage to Live.
• Make sure you are on Live workspace, go to /admin/content - you should see one article and two pages (one page has been pulled from Stage) - all fine.
• Make sure you are on Live workspace, click on Deploy link (top menu right): Add a title for the new deploy in the opened popup and click Deploy to Stage. After that you should see confirmation message: Successful deployment.
• Make sure you are on Live workspace, go to /admin/content - you should see one article and two pages - all fine.

9. Go on Stage workspace:

• Go to /admin/content - here, theoretically, you should have one page and two articles (one from Stage and one from Live), but, actually we have here just one article and one page - the page from Live haven’t got replicated.
• Make sure you are on Stage workspace, click on Update link (top menu right): Confirm that you want to pull changes from Live to Stage.
• Make sure you are on Stage workspace, go to /admin/content - you should see two articles and two pages - all fine.
• Make sure you are on Stage workspace, create one new page and one new article.
• Make sure you are on Stage workspace, click on Deploy link (top menu right): Add a title for the new deploy in the opened popup and click Deploy to Live. After that you should see confirmation message: Successful deployment.
• Go on Live workspace, go to /admin/content - you should see three articles and three pages - all fine.

10. Go on Live workspace (try to reproduce again the problem):

• Create one new page and one new article.
• Make sure you are on Live workspace, click on Deploy link (top menu right): Add a title for the new deploy in the opened popup and click Deploy to Stage. After that you should see confirmation message: Successful deployment.
• Go on Stage workspace, go to /admin/content - theoretically, you should have 3 pages and 4 articles, but, actually we have here just 3 articles and 3 page - the page from Live haven’t got replicated.

Conclusion: When a workspace have Replication settings on deploy filter set, the push replication doesn’t work (we have the Live workspace in the previous example). For Stage workspace push replication works fine because it doesn’t have a Replication settings on deploy filter set.

Deploy doesn't make sense without Workspace. IMO Deploy-triggered replications should use the Workspace replication settings.

@josephdpurcell These changes look wrong to me:

1. +++ b/src/ReplicatorManager.php
   @@ -64,7 +64,7 @@ class ReplicatorManager implements ReplicatorInterface {
   +    $pull_task = $this->getTask($target->getWorkspace(), 'pull_replication_settings');
   
        // Pull in changes from $target to $source to ensure a merge will complete.
        $this->update($target, $source, $pull_task);
   

   In this case content will be replicated from $target workspace to $source workspace, so, $source is the target. Setting should come from $source.

2. +++ b/src/ReplicatorManager.php
   @@ -72,9 +72,12 @@ class ReplicatorManager implements ReplicatorInterface {
   +      $task = $this->getTask($target->getWorkspace(), 'push_replication_settings');
   

   Logically, on push replication, settings should come from $source workspace, otherwise push replication settings doesn't make sense.

Commited: http://cgit.drupalcode.org/workspace/commit/?id=35fdcf8

Thank you @josephdpurcell and @phenaproxima!