Hello,

When using remote workspaces, if they are not available, for example CouchDB not started, Pull or push of content fails with fatal error.

I was trying to add a check on my module https://www.drupal.org/project/deploy_individual before making a patch for this module.

// In case of remote workspace, check that it is available.
    if ($target->hasField('remote_pointer')) {
      $remote = $target->get('remote_pointer')->getValue();
      // Check if it is a remote workspace.
      if (!empty($remote)) {
        $remote = Remote::load($remote[0]['target_id']);
        $checks = $this->remoteCheckManager->run($remote);
        $remote_has_error = FALSE;
        foreach ($checks as $check) {
          if ($check['result'] == FALSE) {
            $remote_has_error = TRUE;
            break;
          }
        }

        if ($remote_has_error) {
          drupal_set_message(
            $this->t('One or more problems were detected with the target workspace. Check the status report for more information.', [':status' => URL::fromRoute('system.status')->toString()]),
            'error'
          );
          return [];
        }
      }
    }

But the problem is that I don't want to have the relaxed module as a dependency, my module should also works with local workspaces.

I think we should add a method in the workspace/src/ReplicatorInterface.php to check if target workspace is available.

For local workspaces it should be almost always true. For CouchdbReplicator, something like the above code should be ok.

Should this issue be associated with relaxed module or should I change to workspace and make patches for workspace and relaxed?

Thanks for the reply.

Comments

Grimreaper created an issue. See original summary.

jeqq’s picture

When using Relaxad:
External replication works just with Relaxed module, we don't need to check if the remote target exists in other modules than Relaxed, here it works fine, we have the Workspace rest resource and we can check if the target exists with HEAD http://mysite.loc/relaxed/target or GET http://mysite.loc/relaxed/target.

Without Relaxed:
The internal replicator (from Workspace module) is working just with local workspaces (replication on a single site), it doesn't make sense to check here if a remote workspace exists.

Grimreaper’s picture

Hello @jeqq,

Thanks for the reply.

The problem I have in my code snippet above is that the "remoteCheckManager" (plugin.manager.remote_check service) is provided by the Relaxed module. So I would need to make a "\Drupal::moduleHandler()->moduleExists('relaxed')" and I could not use dependency injection.

That's why I want to add a method in workspace/src/ReplicatorInterface.php to have the replicators having their own check method.

Yes, for the internal replicator, it is a bit overkill to check if the workspace is available. Therefore it should simply return TRUE.

For the CouchDBReplicator, there is already the plugin.manager.remote_check service to use so the method will be relatively simple.

So this method could be used in Deploy and in any other modules.

What do you think of the addition of this method?

If it is ok for you I would provide patches for relaxed and workspace modules.

jeqq’s picture

Status: Active » Postponed (maintainer needs more info)

@Grimreaper Sorry, but I still don't see the necessity to implement such a method in the replicator classes.

We don't need to check for remotes in the internal/local replicator, it works just with workspaces on the same site.

So I would need to make a "\Drupal::moduleHandler()->moduleExists('relaxed')" and I could not use dependency injection.

Why you can't use dependency injection?

Grimreaper’s picture

Hello,

Here are the patches that I wanted to make. Maybe it will be more clear.

We don't need to check for remotes in the internal/local replicator, it works just with workspaces on the same site.

I totally agree, that's why for the internal replicator, it would simply return TRUE.

I can't use dependency injection in my module as "plugin.manager.remote_check" is a service provided by the relaxed module and I want Deploy individual module to be usable with any Replicator.

So, in my module, I would need to use code like:

\Drupal::moduleHandler()->moduleExists('relaxed')
and
\Drupal::service('plugin.manager.remote_check')

and I don't think that it is maintainable / extendable.

Thanks for the review.

timmillwood’s picture

Status: Needs review » Needs work

One concern I have is, this is only really needed for Relaxed replications and the replicator already does this check (https://github.com/relaxedws/couchdb-replicator/blob/master/src/Replicat...). So I think all we need is better error handling. Firstly why don't we submit a PR to the replicator to provide better exceptions. Then in Relaxed we can catch those exceptions and return something pretty.

I think there are a number of places the the replicator throws exceptions or weird errors and they're totally valid, but we're just not handling them very well, for example #2749263: 403 Forbidden response, the 403 might be correct, but we should display a nice message explaining what happened and suggesting fixes.