backdrop-contrib / project

Projects associate a code-based project with releases and power the update server of BackdropCMS.org
2 stars 10 forks source link

Add ability to exclude certain files from the Packager #37

Closed quicksketch closed 5 years ago

quicksketch commented 5 years ago

When packaging zip files, we currently get a zip from GitHub, unzip, modify all .info files, and then rezip them all into a new archive.

During this process, it would be nice to be able to remove certain blacklisted files, such as CI scripts like .travis.yml, .zenci.yml, etc.

The packager currently lives in project_github.module, in _project_github_create_package().

docwilmot commented 5 years ago

PR adds an alter hook so other modules can do this. Coincidentally needed for removing the screenshots folder from themes, mentioned also here https://github.com/backdrop-ops/backdropcms.org/issues/487#issuecomment-450280682

docwilmot commented 5 years ago

@quicksketch please let me know if a simple hook like this will do the trick. This blocks the work I linked to above so would appreciate a look. I'm also wondering if the alter should also pass the directory url to make it easier to build full urls in implementations.

quicksketch commented 5 years ago

The hook looks great! There's a small issue in the example documentation though, could you replace the $errors array or just remove it entirely?

quicksketch commented 5 years ago

I'm also wondering if the alter should also pass the directory url to make it easier to build full urls in implementations.

Not sure about this. The current hook seems like it would do the job. Could you provide a further example that would use the directory path?

docwilmot commented 5 years ago

On Gitter:

@quicksketch to recap, this hook would help to push through adding more data in info files. https://github.com/backdrop-ops/backdropcms.org/issues/487#issuecomment-450280682 My PR for this would want to rmove screenshot directories. The code to do that in another PR I have waiting is like this: [In an implementation of this new hook:]

  foreach ($files as $path => $file) {
    // Find the base .info file. If the file is a .info file, and it is named
    // the same as the project, then we use this file, even if there are 
    // multiple .info files in the package.
    $extension = substr($file->filename, strrpos($file->filename, '.') + 1);
    if ($extension === 'info' && $file->name == $project_name) {
      $info_contents = backdrop_parse_info_file($file->uri);
      $directory_path = rtrim($path, $file->filename);
      if (is_dir($directory_path . 'screenshots')) {
        $screenshots_directory_path = $directory_path . 'screenshots/';
      }
    }
  }

See $directory_path = rtrim($path, $file->filename);. To get the file path for everything, I'm deducing it by subtracting the file name from the array keys of the $files array. Not sure if that will always be reliable, and I think using this new hook to do any file operations would maybe be more reliable if we passed the directory path where those $files are

@quicksketch reply: @docwilmot Hm, I think because you can already derive the directory path from the $files array we should not pass it as another parameter.

quicksketch commented 5 years ago

Looks great! I merged https://github.com/backdrop-contrib/project/pull/38 into 1.x-1.x branch. Thanks!

docwilmot commented 5 years ago

Woohoo!

quicksketch commented 5 years ago

That said, we have a parallel discussion at https://github.com/backdrop-contrib/project/issues/33 that might change this. I'll cross link over here and see if there are use-cases for which we need to adjust.