silverstripe / silverstripe-assets

Silverstripe Assets component
BSD 3-Clause "New" or "Revised" License
9 stars 65 forks source link

Add task to pre-warm image cache #492

Open maxime-rainville opened 2 years ago

maxime-rainville commented 2 years ago

As server administrator, I want the ability to create image variants progressively prior to new image template being deployed.

Acceptance criteria

Note

We explored a similar concept on an internal project.

maxime-rainville commented 2 years ago

My thinking is that you could have a task that will call each page in your site tree with an alternative template. That would generate all the images for you. Once the task is done, you deploy your site with the alternative template on by default.

There's probably alternative approaches worth considering as well.

GuySartorelli commented 2 years ago

an alternative template

Not sure what this means... how is the template defined? How can we know that the template has all the variants from the website's theme templates? Or is this intended to catch all of the variants defined in PHP files?

maxime-rainville commented 2 years ago

When you do a call like $MyLogo in an SS template, your Image will be rendered via DBFile_image.ss.

One way we could do this, is have custom implementation of DBFile_image.ss with something like this:

<picture>
  <source srcset="$Me.WebP.Url" type="image/webp">
  <img $AttributesHTML />
</picture>

Maybe that template is disabled somehow, but your task can enable it.

maxime-rainville commented 2 years ago

Another question is what to do for project that have custom controllers that don't map to any SiteTree. Do we want to add a way for that task to discover those pages, maybe an extension hook on the task itself?

GuySartorelli commented 2 years ago

Ahh, okay. So this task runs under the assumption that sites are using the default template? That's fair.

There are definitely people using <img src="$MyLogo.ScaleWidth(150).Link"> but I think it's fine for the cache warmup task to not consider those cases.

The task will also either have to be told how to traverse all logic branches which lead to different images, or else the documentation clearly state that images under branching logic may not have their variants pre-generated.

maxime-rainville commented 2 years ago

If you are manually writing <img> tags in your template, there's literally nothing we can do for you. As a general practice, we should discourage this.

michalkleiner commented 2 years ago

We integrate custom <picture> tags through a reusable template in the majority of projects mainly for full control over what it does, and secondly for the lack of such feature when images are just used from the WYSIWYG editor. Even with all these proposed changes, I would still guess there will be use cases where we would still do it as I can't imagine this would support all custom design and template structure requirements. So I'd be against discouraging devs from doing what they need to be doing to deliver their builds. Sure, it's a nice and better default for standard TinyMCE-inserted images with a bit more responsiveness aspect considered.

maxime-rainville commented 2 years ago

I wouldn't say it's a horrible practice to manually write your own picture/img tag. But it does come with the downside that it undermines some of the CMS capabilities. But if the CMS doesn't give you the tool to achieve the outcome you want, you got to do what you got to do.

We should aim to make sure that our Image implementation has all the APIs people need to implement common use case, and make sure people are aware of some of the pitfall if they chose to write their own tags.

GuySartorelli commented 2 years ago

I think one of the main issues is, as Michal alluded to, the fact that WYSIWYG images don't use the same template as relation images, so it's hard to have a clean way to customise the way images render on a site. That's covered by silverstripe/silverstripe-cms#2735 in this same epic.

Another fairly common situation I've seen is the desire to use BEM or other specific css naming conventions that require a different class selector on images in different scenarios (e.g. you may have a card and a banner, and you want .card__image on the img/picture tag in the card, and .banner__image on the img/picture tag in the banner.

I don't think this is something that can currently be done with the built-in image template. I guess you could override the template and add use it as an include, and pass the class names in as arguments... but then you might also need certain data attributes for interactive elements, etc... it can get a bit messy.

All that to say: There will be edge cases this task won't catch. I think that's fine - in fact it's inevitable. We just need to document it in whatever documentation introduces the task, so that developers will be aware of what it does or doesn't do.

michalkleiner commented 2 years ago

Thanks, Guy, that sums it up well. On some projects, we even went as far as disabling the TinyMCE image plugin and only used images via has_one/many_many from blocks/pages to have the best control over things from the design implementation perspective. We didn't want users to put images just anywhere they wanted to break the visual aspect of the sites. There's definitely a use case for that and if it was made easier to customise the insertion modal to be able to provide custom controls and then render them using custom templates, it would surely be closer to being more usable for more devs directly from the WYSIWYG, so I'm supporting this work, it's a good step forward.

michalkleiner commented 2 years ago

If the rendering was done through an overridable method (or via an extension point), using an overridable template, that'd be the best — could be customised per WYSIWYG/per relation/per data model or controller.