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

DRAFT Drupal 11 compatibility. #191

Draft
wants to merge 7 commits into
base: 8.x-1.x
Choose a base branch
from
Draft

Conversation

kepol
Copy link
Contributor

@kepol kepol commented Sep 29, 2023

We shouldn't merge this yet because some of our dependencies are D11 compatible yet. Moving this to draft for now.

https://www.drupal.org/project/simple_sitemap - not compatible asf 5 Jul 2024 [D11 issue]
https://www.drupal.org/project/tome - not compatible as of 5 Jul 2024 [D11 issue]
https://www.drupal.org/project/webform - not compatible as of 5 Jul 2024 [D11 issue]
https://www.drupal.org/project/token - dev version is compatible as of 18 May 2024
https://www.drupal.org/project/purge - compatible as of 28 Jun 2024

Although we don't need to provide D11 support yet, there are quite a few modules that already do support D11. Someone provided a patch so I decided to test it. It worked with one minor tweak.

See d.o issue: https://www.drupal.org/project/quantcdn/issues/3389070

Copy link
Contributor

@steveworley steveworley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, we might just need to do a little testing on the self::MAIN_REQUEST change in the Url registrar class to ensure its compatible with d9/10.

@kepol
Copy link
Contributor Author

kepol commented Sep 29, 2023

I did a quick test using Umami for D9.5, D10, D10.1 and D11 and they all seemed fine other than D11 didn't process the webp image on the /recipes page. Not sure why that's webp anyway. I didn't see anything obvious in the core issue queue but I assume it's a bug in core.

src="/sites/default/files/styles/scale_crop_7_3_medium/public/vegan-brownies-hero-umami.jpg.webp?itok=A9_Np84o"

Screenshot 2023-09-29 at 8 10 15 PM

@kepol
Copy link
Contributor Author

kepol commented Sep 29, 2023

@steveworley Would you prefer we merge this or park it until later given thewebp issue?

UPDATE: Note that page/image looks fine on the Drupal side.

steveworley
steveworley previously approved these changes Oct 18, 2023
@steveworley
Copy link
Contributor

I think we should try and see if we can resolve the webp issue before merging. I haven't had a chance to dig in just yet.

@kepol kepol marked this pull request as draft October 31, 2023 22:14
@kepol
Copy link
Contributor Author

kepol commented Oct 31, 2023

Sounds good. Moved this to draft for now.

@kepol kepol changed the title Drupal 11 compatibility. DRAFT Drupal 11 compatibility. Nov 2, 2023
@kepol kepol self-assigned this Nov 2, 2023
@kepol
Copy link
Contributor Author

kepol commented Jan 4, 2024

Updated D11 test site and tested again and am seeing that this may be a responsive image issue or an Umami profile issue. I see the webp image at some widths and not others. I see webp images in Quant.

The markup has:

                  <source srcset="/sites/default/files/styles/scale_crop_7_3_wide/public/vegan-brownies-hero-umami.jpg.webp?itok=mOV_7Acn 1x" media="all and (min-width: 1400px)" type="image/webp" width="3000" height="1285">
              <source srcset="/sites/default/files/styles/scale_crop_7_3_large/public/vegan-brownies-hero-umami.jpg.webp?itok=0_b82U2r 1x" media="all and (min-width: 800px) and (max-width: 1400px)" type="image/webp" width="1440" height="617">
              <source srcset="/sites/default/files/styles/scale_crop_7_3_medium/public/vegan-brownies-hero-umami.jpg.webp?itok=A9_Np84o 1x" media="all and (min-width: 500px) and (max-width: 800px)" type="image/webp" width="1200" height="514">
              <source srcset="/sites/default/files/styles/scale_crop_7_3_tiny/public/vegan-brownies-hero-umami.jpg.webp?itok=DI5cHAO5 1x" media="all" type="image/webp" width="500" height="214">

And Quant has:

Screenshot 2024-01-03 at 6 04 14 PM

Screenshot 2024-01-03 at 6 03 47 PM

Screenshot 2024-01-03 at 6 03 23 PM

@kepol
Copy link
Contributor Author

kepol commented Jan 4, 2024

It looks like this is where the Umami profile got the responsive images (webp) added:

https://www.drupal.org/project/drupal/issues/3349447#comment-15184905

@kepol
Copy link
Contributor Author

kepol commented Jan 4, 2024

It doesn't seem to be a Quant issue. Once the responsive images are generated in Drupal, they do get seeded. For some reason the scale_crop_7_3_tiny image isn't getting created on the Drupal side so it generates an error log but the other 3 do show up.

@kepol
Copy link
Contributor Author

kepol commented Jan 4, 2024

Okay... used emulator to go more narrow and then tiny showed up and was seeded. So... the main issue is that for some reason the Quant code in src/Plugin/QueueItem/FileItem.php to generate the image styles (if they don't exist) isn't working for these. The image styles will only get seeded if they've already been generated.

Screenshot 2024-01-03 at 7 29 04 PM

@kepol
Copy link
Contributor Author

kepol commented Jan 4, 2024

I think I figured it out. In some cases, the Quant code is pulling in the 1x into the filename which is probably what is screwing up the image style generation, e.g.

/sites/default/files/styles/scale_crop_7_3_tiny/public/vegan-brownies-hero-umami.jpg.webp?itok=DI5cHAO5 1x

I assume this is during the parsing the srcset. I'll see if I can find the regex for that.

@kepol
Copy link
Contributor Author

kepol commented Jan 4, 2024

Added a work around to this PR. To test, I got rid of the various image styles for the recipes page and then seeded views so they would be picked up. The code now generates all the image styles properly.

Screenshot 2024-01-03 at 8 44 40 PM

@kepol kepol requested a review from steveworley January 4, 2024 04:48
@kepol kepol marked this pull request as ready for review January 4, 2024 04:48
@kepol kepol changed the title DRAFT Drupal 11 compatibility. Drupal 11 compatibility. Jan 4, 2024
@kepol kepol dismissed steveworley’s stale review January 4, 2024 05:04

Made changes since this time.

@kepol
Copy link
Contributor Author

kepol commented Jan 4, 2024

Stu mentioned that the responsive image issue might be better fixed elsewhere but I'll leave the code here for now. I did test it using an image with spaces.

Screenshot 2024-01-03 at 9 23 05 PM


// Handle responsive images by removing extraneous data like ` 1x`.
// @todo Fix this in the Quant infrastructure.
if ($this->fullPath) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code looks fine, but I'm not sure why were transforming path on disk — do image styles not include the 1x when resaving?

Copy link
Contributor Author

@kepol kepol Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@steveworley We're not changing the path on disk afaik. In some situations (unknown why, assume a core bug), when you get the "full_path", it shows a space and then 1x at the end even though that is not correct and should not be there. I assume there is some bug somewhere parsing the srcset value.

Stu said he will handle this at some point on the infra side, but this is just a workaround for now.

Without this workaround, then some of the image styles may not be created if they don't exist already.

@kepol
Copy link
Contributor Author

kepol commented Jan 20, 2024

We shouldn't merge this yet because some of our dependencies are D11 compatible yet. Moving this to draft for now.

https://www.drupal.org/project/purge - not compatible as of 19 Jan 2024
https://www.drupal.org/project/simple_sitemap - not compatible as of 19 Jan 2024
https://www.drupal.org/project/token - not compatible as of 19 Jan 2024
https://www.drupal.org/project/tome - not compatible as of 19 Jan 2024
https://www.drupal.org/project/webform - not compatible as of 19 Jan 2024

@kepol kepol marked this pull request as draft January 20, 2024 02:49
@kepol kepol changed the title Drupal 11 compatibility. DRAFT Drupal 11 compatibility. Jan 20, 2024
@kepol kepol removed their assignment Jan 20, 2024
@kepol
Copy link
Contributor Author

kepol commented Jul 6, 2024

Updated the issue with the D11 status of module dependencies. Still waiting on 3 of them.

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

Successfully merging this pull request may close these issues.

2 participants