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

[D8] Provide image style tokens (imagecache_token module in core) #3594

Closed
laryn opened this issue Mar 18, 2019 · 32 comments · Fixed by backdrop/backdrop#2849
Closed

[D8] Provide image style tokens (imagecache_token module in core) #3594

laryn opened this issue Mar 18, 2019 · 32 comments · Fixed by backdrop/backdrop#2849

Comments

@laryn
Copy link
Contributor

laryn commented Mar 18, 2019

Description of the need
The functionality of Imagecache Token could be merged into the existing token functionality. (It looks like this functionality is merged into token in D8.)

This module provides additional tokens for image fields. For each image style available, a token [node:field_image_field:style_name] will be provided. It is also possible to get image properties like width or height by using tokens like [node:field_image_field:style_name:height].

Proposed solution
Since token is in core, I think merging this image style token functionality into core would be ideal.

I would like to be the advocate for getting this feature into 1.15. I've added the milestone candidate label.


Core minor release advocate: @jenlampton


PR by @jenlampton: backdrop/backdrop#2849

@klonos
Copy link
Member

klonos commented Mar 18, 2019

Since token is in core, I think merging this image style token functionality into core would be ideal.

That, plus the fact that this is in D8 core since its early versions, and we are aiming for as much feature parity as possible. Which makes this issue here part of #378 😉

@klonos klonos changed the title Expand image tokens to allow image styles (Imagecache_Token functionality) Expand image tokens to allow image styles (imagecache_token functionality) Mar 18, 2019
@jenlampton jenlampton changed the title Expand image tokens to allow image styles (imagecache_token functionality) [D8] Expand image tokens to allow image styles (imagecache_token functionality) Apr 28, 2019
@laryn
Copy link
Contributor Author

laryn commented Sep 5, 2019

Works for me -- awesome!

@jenlampton
Copy link
Member

I just pushed passing tests to the PR.

@jenlampton
Copy link
Member

In the weekly meeting today there was some resistance to adding these tokens into core. The concern is that perhaps the only place these tokens are used is in metatags, and metatag module is a contrib module. Perhaps this should remain a contrib module too?

Adding the needs feedback label to see if others have different use-cases.

@ghost
Copy link

ghost commented Sep 20, 2019

Since it was mentioned above that this is already in D8 core, do we know what their use case is? I'd be happy to see this added for the reasons given already (adding to existing core functionality, feature parity with D8).

@laryn
Copy link
Contributor Author

laryn commented Sep 20, 2019

I guess image style tokens could be used anywhere regular image tokens are used, and regular image tokens are in core... so I'm not sure I see the problem. It makes sense to me to have it in core, anyway.

@jenlampton
Copy link
Member

jenlampton commented Sep 20, 2019 via email

@klonos
Copy link
Member

klonos commented Sep 27, 2019

Sorry I missed today's dev meeting, when this issue was brought up. I watched the recording afterwards, and it seemed to me that we are looking for the reasoning behind why this was added to core in D8, and why not have it live in contrib instead. So I have taken the time to go through the d.org issue that introduced this change, and here's a few of the comments that may make it a case for core:

From the (content) marketing perspective it is kind of a big deal to be able to serve the correct OG:image to the og:image tag (using Metatag module + the right Token) with Drupal 8.

this is needed, badly. facebook only allows a maximum of 8MB file size for sharing, so if the user uploaded a massive image file, we need to be able to specify a scaled down image style inside this og:image tag.

...it's fairly critical to be able to serve image styles that are tailored to the optimal dimensions for respective social media outlets.

@klonos
Copy link
Member

klonos commented Sep 27, 2019

...there's another issue in d.org against the D8 core Token module, which is related: Add support for field properties , which links to https://www.drupal.org/project/token/issues/2430827 ...and that one in turn seems to have about half a dozen of children issues.

@jenlampton
Copy link
Member

jenlampton commented Sep 27, 2019 via email

@ghost
Copy link

ghost commented Sep 27, 2019

That was my thought too, after reading @klonos' quotes from D.org... If there's no use-case for this other than social media's og:image tag, then I guess this should be in Metatag (at least I'd prefer that over Metatag requiring another contrib module).

@klonos
Copy link
Member

klonos commented Sep 27, 2019

...the/another use case for core is mobile devices, where you need "automagic" derivatives of images you upload, so that people don't download a 2MB image in their phone, over mobile data networks (where speed/cost is an issue). To that effect, we could have also left #869 to contrib, but we implemented it in core 😉

@klonos
Copy link
Member

klonos commented Sep 28, 2019

...long-term, this might fit into #1440 too 😉

@jenlampton
Copy link
Member

jenlampton commented Sep 28, 2019

@klonos I don't see how the mobile device thing is a relevant use case for image style tokens. Are you thinking that we could do something like the srcset attribute on img and deliver multiple URLs to the browser, and let it decide which to render? I could see that, but don't see how we'd use tokens for them. Is there a D7 contrib module that uses tokens for this?

@jenlampton jenlampton self-assigned this Aug 29, 2020
@jenlampton
Copy link
Member

I'm stuck on the failing test. backdrop/backdrop#2849 (comment)
I'm going to move on to another issue for now.

@quicksketch
Copy link
Member

I'm stuck on the failing test. backdrop/backdrop#2849 (comment)

I pushed a commit to the PR that should fix the two failing tests. Because the image styles are cached in memory, if you modify the image styles in various POST requests, the static in-memory cache must be cleared in the testing thread to get a fresh list of styles from the database. So I just added two backdrop_static_reset('image_styles'); calls in there and the tests are passing now.

@quicksketch
Copy link
Member

There are a few changes that I think we should make here, I posted a review to the PR: backdrop/backdrop#2849 (review)

There are some features here that I think are too granular for core, like :first, :second, and :third support for image fields, when as far as I know, we don't have any similar support for any other multi-value fields. That said, their value to me seems clear even if I think they're uncommon by core's current practices.

Lastly, if this is replacing a contrib module in its entirety, we should add it to backdrop_merged_modules() and disable it in an update hook. See field_update_1004() as an example where field_formatter_settings was merged into core.

@herbdool
Copy link

herbdool commented Sep 2, 2020

It doesn't have an update hook yet to disable the contrib version, nor backdrop_merged_modules.

@jenlampton
Copy link
Member

jenlampton commented Sep 2, 2020

See field_update_1004() as an example where field_formatter_settings was merged into core.

There is no Backdrop version of imagecache_token, so I don't think we need the visible message telling people to remove the from their site. I will leave the bit of code that deletes the entry from the system table though, for people upgrading from Drupal 7.

@jenlampton
Copy link
Member

jenlampton commented Sep 2, 2020

Looks like the works for me label was removed when tests were failing, adding back since the problem was with the tests and not the functionality.

@quicksketch
Copy link
Member

Merged backdrop/backdrop#2849 into 1.x for 1.17.0. Thanks everyone!

@jenlampton jenlampton changed the title [D8] Expand image tokens to allow image styles (imagecache_token functionality) [D8] Provide image style tokens (imagecache_token module in core) Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

5 participants