@heddn

It sounds like in this case, someone needs to propose themselves to be the maintainer and we won't be following this process? Or am I reading that wrong?

Sad to see you go.

Another message sent to the maintainers:

smustgrave → credited heddn → .

Looking at https://developer.mozilla.org/en-US/docs/Web/HTML/Element/picture for inspiration, I think what we want to generate is markup that looks like. Note in the first 2 <source/> tags, we add a type. In the 3rd, we can't because we have 2 types of images. The fallback is yet another image all together.

<picture>
  <source srcset="photo.avif" type="image/avif" />
  <source srcset="photo.webp" type="image/webp" />
  <source srcset="photo.png 1008w, photo.jpg 1312w" media="all and (min-width: 1000px)" sizes="100vw">
  <img src="photo.gif" alt="photo" loading="lazy" width="2000" height ="3000" />
</picture>

OOTB, right now, we can generate markup that looks like:

<picture>
                  <source srcset="/16_9_1008x567_focal_point_webp/public/2024-12/drupal-cms-hero.png.jpeg 1008w, /16_9_1312x738_focal_point_webp/public/2024-12/drupal-cms-hero.png.webp 1312w" media="all and (min-width: 1000px)" sizes="100vw" width="1312" height="738">
              <source srcset="/5_2_1000x400_focal_point_webp/public/2024-12/drupal-cms-hero.png.webp 1x" media="all and (min-width: 700px)" type="image/webp" width="1000" height="400">
              <source srcset="/16_9_704x396_focal_point_webp/public/2024-12/drupal-cms-hero.png.webp 1x" media="all and (min-width: 500px)" type="image/webp" width="704" height="396">
                  <img loading="eager" width="512" height="288" src="/16_9_512x288_focal_point_webp/public/2024-12/drupal-cms-hero.png.webp" alt="A bunch of laptops on a table from above">
  </picture>

What we don't have with core right now is the ability to have multiple image styles on a per image type. For the sizes option, I don't think that is critical. You can make it do what you want. (Please someone confirm this assumption).

What we need to address is Select a single image style.. We want to change that into multiple images. What I wonder is if we really want to do that or not. We really want that image "style" to have multiple image conversions. (Please confirm this assumption).

heddn → made their first commit to this issue’s fork.

On second thought, we'll also want to post a CR for this. I'll do that after I get a review.

It runs on the testbot, but at a huge cost, 36 minutes: https://git.drupalcode.org/issue/drupal-1565704/-/pipelines/397114/test_...

Update tests added and are green now. IS seems up to date. Ready for review.

Let's fix this then.

As I expected, I tracked down over in https://www.php.net/manual/en/ini.core.php#ini.sect.file-uploads that we can't set that value at run-time.

This can't be easily tested with the current approach. Perhaps the testbot CI can handle things, but running the test locally, (5000 max input var is my php's default) and the test never runs to completion. I tried to set the value lower via an ini_set in the test's setup method. But that still left the test form still using 5000 from max_input_vars.

Instead I manually tested things with the test by artificually setting my max_input_vars to 5. That let things run to completion in a very happy way. Moving this back to NW because we either need to be OK with manual testing (and get rid of the tests) or figure out a way to runtime set the max_input_vars to a lower value.

LGTM. All concerns addressed.

heddn → made their first commit to this issue’s fork.

There's tests. All feedback is addressed. LGTM.

re: slot count:

From #1199866-84: Add an in-memory LRU cache → , the thought was that this should be a much higher number then 300. So it is nice to see a number of 1000. Do we know if smaller hosting, e.g. an entry level pantheon plan will hold 1000 entity slots for 5-6 fields on a node or paragraph?

Since Benji RTBCed this, I'm going to assume his suggestions/comments on the MR can be resolved at this point. That is now done. Otherwise, +1 on RTBC.

Added test coverage. This was RTBC, so hopefully someone can simply re-RTBC after reviewing the tests.

heddn → made their first commit to this issue’s fork.

Made a few minor fixes given the last few asks. This looks pretty good at this point. Given all I did was switch a comment to {@inheritdoc}, I think I can still mark this RTBC.

heddn → made their first commit to this issue’s fork.

I've gone through and responded to most of the feedback mentioned in the IS. The requests in #80 & #82 are not super clear what is being asked given that the EnvironmentMemory object didn't materialize. Can we cross them off as well?

This is postponed on 🐛 SqlBase::prepareQuery() should be called also on count Needs work .

My most recent visit to this issue is from ✨ SQL source plugins: allow defining conditions and join in migration yml Needs work , which is the nearly the number #1 asked for feature for migrate.module right now. I'd hate to block real progress on a lot of nice to haves.

I did a rebase of the MR. I'm not sure what are the actionable next steps here. From reading #46, there's a whole rework desired. What is the MVP?

heddn → made their first commit to this issue’s fork.

heddn → changed the visibility of the branch 3069776-sql-source to hidden.

heddn → changed the visibility of the branch 3069776-sql-source-plugins to hidden.

heddn → changed the visibility of the branch 11.x to hidden.

This is pretty close. Can we fix the phpcs, etc issues and add a test case? That's all that is missing from landing this.

In general, I like the approach discussed in #74. It might not solve all the problems, but it does solve a single problem and should help with some people's issues. Let's roll it into an MR and add tests.

Nice improvement to an existing migration to make this more flexible. There's tests that are passing. LGTM.

Thanks for the contribution. I've added a test that will help us catch this in the future.

heddn → made their first commit to this issue’s fork.

This is no longer a concern as it got solved in the D10 compat issue.

Replied in MR. Thanks for your kind review.

heddn → created an issue.

heddn → changed the visibility of the branch project-update-bot-only to hidden.

heddn → changed the visibility of the branch 3435465-automated-drupal-11 to hidden.

heddn → changed the visibility of the branch 3435465-d11_ready to hidden.

Can you provide some examples/links where you are contributing already to the module? That would definately improve your chances of getting maintainership access.

heddn → made their first commit to this issue’s fork.

Closing as duplicate of 📌 t() calls should be avoided in classes. Active .

Thanks #7 for your input. I typically would agree with you. But since this and 📌 \Drupal calls should be avoided in classes, use dependency injection instead Needs review are dealing with 2 different phpcs things I wonder if we should just re-title/re-purpose to fix all phpcs issues in one shot. Instead of having to review several distinct issues. With that mind, re-titling and closing the other as duplicate to this.

NW to address all PHPCS.

heddn → made their first commit to this issue’s fork.

heddn → made their first commit to this issue’s fork.

Let's merge and see what the users of the community say.

heddn → made their first commit to this issue’s fork.

Will need to be rerolled into an MR at this point so tests can execute.

Will need to be rerolled into an MR at this point so tests can execute.

Thanks for the contributions.

heddn → made their first commit to this issue’s fork.

Let's move this over the the 2.x branch. Will need to be rerolled into an MR at this point so tests can execute.

Let's move this over the the 2.x branch. Will need to be rerolled into an MR at this point so tests can execute.

With the EOL of D7 past, let's close out old stuffs. If this is still an issue in the more modern version, feel free to re-open and document steps to reproduce.

With the EOL of D7 past, let's close out old stuffs. If this is still an issue in the more modern version, feel free to re-open and document steps to reproduce.

With the EOL of D7 past, let's close out old stuffs. If this is still an issue in the more modern version, feel free to re-open and document steps to reproduce.

With the EOL of D7 past, let's close out old stuffs. If this is still an issue in the more modern version, feel free to re-open and document steps to reproduce.

With the EOL of D7 past, let's close out old stuffs. If this is still an issue in the more modern version, feel free to re-open and document steps to reproduce.

With the EOL of D7 past, let's close out old stuffs. If this is still an issue in the more modern version, feel free to re-open and document steps to reproduce.

Thxs

heddn → made their first commit to this issue’s fork.

This is for the D7 version of the module and doesn't seem to apply to the more modern version. Please re-open if it is still an issue on that version.

My changes to the tests are so minimal, I'm going to say this is good to go. LGTM

Now that 💬 Revert requirement that IDs not include spaces Needs work landed, I think this can be closed as outdated. If that isn't the case, please re-open.

Thanks for your contributions.

heddn → created an issue.

Thanks everyone.

Open an MR with tests?

Fixed failing tests by converting to a data provider and a kernel test. There isn't a 100% replacement as many of the previous tests were simple testing of setter/getters. Needs a review.

The changes to drupal core were actually added in 10.1. You don't need to do anything except uninstall advagg.

I would mark this as RTBC except I made some minor changes to the tests to get them to green. Anyone else able to make that call?

Unit tests are now passing green on MR 15.

heddn → made their first commit to this issue’s fork.

RTBC++

re #280: my site's config was already using sub_handler and sub_handler_settings with a recent version of the patch. I'm about to switch to the contrib module with the core code in 10.4.0, but wanted to provide this insight.

Can we roll that patch into an MR? Tests no longer get triggered on patches.

There's a lot of re-work needed to make this work w/ CK5 and D11. https://www.drupal.org/node/3291493 → deprecated the image dialog forms. I'm going to leave this at NW for now. But I started some of the rebase work in https://git.drupalcode.org/project/drupal/-/merge_requests/10767. It is a pure rebase. From here on, we'll need to figure out how to make this work with CK5. I'm not taking that on at the moment.

Can you open a follow-up to remove the old state entry? We should get rid of it.

Thanks for your contributions.

CR added

With force pushes and a series of confusing commits, I'm not sure what has happened since #12 when I last RTBCed this thing. It looks like my changes even disappeared. And we've regressed from green passing tests to failures. This is not RTBC ready.

I'm pretty sure this needs a rebase.

Errors fixed in readme. Thanks.

heddn → made their first commit to this issue’s fork.

Let's add test coverage so regressions like this can't happen.

We have Guzzle available to us so the curl isn't really needed.

Also, I'm not sure I like the idea of having remote resources download-able by default with this module. That opens up the attack surface quite a bit. Being able to do any prep work for a resource could/should happen before the migration happens. Say via a shell script that curl/downloads the resources locally first. Convince me why this is strictly necessary.

Thanks for the graphics.

Thanks for the contributions. This makes a lot of sense.

heddn → made their first commit to this issue’s fork.