Mostly thinking aloud here, but I've always found the command line optimizations to be a nice thing to have, but a probably security risk and slow, so I've not really used them in production. I wonder if they would sit better in their own submodule, or even their own separate contrib project.

We could then focus the main project on adding the optimization pipeline to image styles, providing an API for that etc. and then have lots of other modules for actually integrating with other services. Given how Drupal 8 sites are largely built with composer, bringing in these sorts of additional dependencies it's too arduous any more. We'd benefit from having more focussed tests in our modules I think, and would probably be able to get to a stable version much quicker :)

CommentFileSizeAuthor
#14 imageapi_optimize-2990670-update-hook.patch6.6 KBSteven Jones
#5 imageapi_optimize-2990670-remove-the-processors.patch51.26 KBSteven Jones
#3 imageapi_optimize-2990670-remove-the-processors.patch51.31 KBSteven Jones
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Steven Jones created an issue. See original summary.

Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones at ComputerMinds commented

In fact we can do this for resmushit and tinypng too, basically everything.

Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones at ComputerMinds commented
Priority: Normal » Major
Status: Active » Needs review
FileSize
imageapi_optimize-2990670-remove-the-processors.patch51.31 KB

Here's a patch that removes all of the implementation specific code from the 8.x-2.x branch.

Status: Needs review » Needs work

The last submitted patch, 3: imageapi_optimize-2990670-remove-the-processors.patch, failed testing. View results

Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones at ComputerMinds commented
Status: Needs work » Needs review
FileSize
imageapi_optimize-2990670-remove-the-processors.patch51.26 KB

Status: Needs review » Needs work

The last submitted patch, 5: imageapi_optimize-2990670-remove-the-processors.patch, failed testing. View results

jcisio’s picture

jcisio CreditAttribution: jcisio at Axess Open Web Services for Institut français commented

Steven, I think instead of moving to another contrib project, it would be better to split of to a submodule (etc. image optimize providers - I don't see the benefit of creating N submodules), and enable it in a hook_update_N, not hassle with yet another extra lightweight module. While some projects use several contrib projects for their sub modules, I think we are not reach the critical mass that need it: each provider is just a few dozens of lines. The drawback is we can't have independent releases in each module, but is it a real need now? What if we consider the module stable when the core module is stable?

Now, about the patch. If we move to a separate submodule (or three submodules, for CLI and online tools), should we leave the abstract implementation and the interface in the core module?

Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones at ComputerMinds commented

A couple of big things I can think of:

  1. Composer files will only be picked up at a root of the module, so if you want to provide a library for external integration (e.g. tinypng or kraken) then either it needs to go in the root project composer.json or some other manual wrangling needs to happen. Separate projects make this much cleaner.
  2. PHPUnit tests can be found with a simple glob over files in a few subfolders of the projects, without needing gymnastics of paths and making sure that the any composer libraries are also around.

And more generally:

  • It shows how all the processors are simple extensions and provides lots of examples for how to do it. If someone wants to integrate with a new service, they can look at one of the projects, resmush.it or tinypng for example, and see everything they need to implement/copy.
  • It's forces this main project to stay small and focussed. Rather than having to support the 'easy' to integrate with services etc.
  • The code that runs the command line tools has always felt icky to me, and a possible security risk. I'd love to see that hived off so that if I'm not going to be running command line stuff, that code isn't even on my servers.
jcisio’s picture

jcisio CreditAttribution: jcisio at Axess Open Web Services for Institut français commented
Title: Split command line optimizations into separate module » Split optimizations into separate modules

About libraries for 3rd party services, you made a good point. So indeed we need a project for each service.

About local tools, I'm not sure it works like that. "command line optimizations" should be correctly said as "local optimizers" vs. "remote optimizers". And I'd rather argue that remote optimizers is more a security risk than local optimizers, because it depends on a 3rd party remote server. I understand what you feels, about running exec() in PHP. However, it's what we do everywhere, in Drupal and Symfony, in the imagemagick module, and PHP safe mode turned out to be a bad decision and was removed.

That said, these local tools should logically live in a separate project, too. I've just seen https://www.drupal.org/project/imageapi_optimize_binaries so it'd good for me ;)

Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones at ComputerMinds commented

Lovely, I'll work on this today, and work out where we with regard to getting a beta out!

  • Steven Jones committed 0c1eb09 on 8.x-2.x 
    Issue #2990670 by Steven Jones: Split optimizations into separate...
Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones at ComputerMinds commented
Assigned: Unassigned » Steven Jones

I've pushed the initial code removal.

Things that we should do:

  • Document this in release notes.
  • Maybe we should attempt to enable the modules removed if they were being used, so that we can throw a big exception to people upgrading.
  • Get the code in the other projects cleaned up.
Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones at ComputerMinds commented

Maybe we should attempt to enable the modules removed if they were being used, so that we can throw a big exception to people upgrading.

I wonder if we can list the processors on each pipeline and see what their provider was, or at least have some kind of fixed mapping.

Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones at ComputerMinds commented
Status: Needs work » Needs review
FileSize
imageapi_optimize-2990670-update-hook.patch6.6 KB

Here's a patch that does some enabling of new modules.

  • Steven Jones committed 4c54dce on 8.x-2.x 
    Issue #2990670 by Steven Jones: Enable optimizations from separate...

Status: Needs review » Needs work

The last submitted patch, 14: imageapi_optimize-2990670-update-hook.patch, failed testing. View results

Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones at ComputerMinds commented
Status: Needs work » Fixed

Need to remember to document this in the release notes, but this is otherwise done now.

Status: Fixed » Closed (fixed)

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