Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add task to pre-warm image cache #492

Open
4 tasks
maxime-rainville opened this issue May 3, 2022 · 11 comments
Open
4 tasks

Add task to pre-warm image cache #492

maxime-rainville opened this issue May 3, 2022 · 11 comments

Comments

@maxime-rainville
Copy link
Contributor

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

Acceptance criteria

  • A task can be queued that will create all expected image variant prior to a change in the Image template.
  • If that task fails, it can be restarted where it left off.
  • This task is generic enough that it can work for any custom Image template.
  • Developers have clear guidance on how to configure the task.

Note

We explored a similar concept on an internal project.

@maxime-rainville
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

maxime-rainville commented May 5, 2022

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

michalkleiner commented May 7, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants