Problem/Motivation

This post is a part bug report, part feature request.

I needed to use the hook_s3fs_copy_params_alter to support my custom S3FS stream wrapper, but noticed a few different issues along the way.

The alter hook is called from \Drupal\s3fs\S3fsFileService::copyObject on line 638:

$this->moduleHandler->alter('s3fs_copy_params_alter', $copyParams);

Because the alter type is "s3fs_copy_params_alter", it would actually make the hook "hook_s3fs_copy_params_alter_alter". It really should be:

$this->moduleHandler->alter('s3fs_copy_params', $copyParams);

Furthermore, according to s3fs.api.php, there should be two different params for the hook: $copy_params and $s3_key_paths. But the code only passes $copyParams by reference. This results in the following error:

TypeError: my_module_s3fs_copy_params_alter_alter(): Argument #2 ($s3_key_paths) must be of type array, null given, called in /app/web/core/lib/Drupal/Core/Extension/ModuleHandler.php.

So in the end, I had to set up the hook in my custom module as follows:

function my_module_s3fs_copy_params_alter_alter(array &$copy_params) {

}

Proposed resolution

It would be really helpful to have some of the additional data available in the hook. In particular, the scheme. Here are my proposed changes:

$this->moduleHandler->alter('s3fs_copy_params', $copyParams, $scheme);

Then I could use it with:

function my_module_s3fs_copy_params_alter(array &$copy_params, $scheme) {
  if ($scheme === 'my_custom_scheme') {
    // alter $copy_params here.
  }
}

Thoughts?

CommentFileSizeAuthor
#4 s3fs-hook_s3fs_copy_params_alter-3323002-4.patch577 bytesjoncjordan

Comments

joncjordan created an issue. See original summary.

joncjordan’s picture

joncjordan commented
Issue summary: View changes
joncjordan’s picture

joncjordan commented

Update: I see that it's handled a bit differently in \Drupal\s3fs\StreamWrapper\S3fsStream::rename

    $rename_context = [
      'from_key' => $from_key,
      'to_key' => $to_key,
    ];

    $options = $this->getOptions();

    // Allow other modules to alter the rename params.
    \Drupal::moduleHandler()->alter('s3fs_copy_params', $options[$this->protocol], $rename_context);

This seems to be the correct implementation, at least based on the comment block in s3fs.api.php.

So, I think all we would need to do is update \Drupal\s3fs\S3fsFileService::copyObject as follows:

    $rename_context = [
      'from_key' => $key_path,
      'to_key' => $src_key_path,
    ];

    $this->moduleHandler->alter('s3fs_copy_params', $copyParams, $rename_context);

This still leaves me without the scheme, which would be helpful for me to do what I need in the alter hook. But at least it fixes the errors.

joncjordan’s picture

joncjordan commented
StatusFileSize
new s3fs-hook_s3fs_copy_params_alter-3323002-4.patch577 bytes

Patch attached.

  • cmlara committed d3b66f9 on 8.x-3.x 
    Issue #3323002 by joncjordan: Incorrect implementation for...
cmlara’s picture

cmlara
he/him
commented
Status: Active » Fixed

This looks good to me,

Thank you for the patch, I have committed it to the dev branch.

At this level the command are generally looking at the bucket path which is why the scheme is not included. This is similar to how 'File-specific Settings' is path based not scheme based.

A file on a scheme will always have a consistent prefix based on the bucket settings.

If you need to obtain the scheme from path https://git.drupalcode.org/project/s3fs/-/blob/80e9294532b36595a3d945309... provides a reference for how we determine the scheme when performing a cache refresh.

joncjordan’s picture

joncjordan commented

Thanks cmlara. I managed to figure out how to do what I needed in the alter hook, without the scheme explicitly passed in.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.