@heddn

benjifisher → credited heddn → .

It is sorta a bug in that you can add the field with no comment type available. There should be a type of check in that space.

#257: I don't think you technically _must_ create a hook_update. You could just keep legacy code, check a Settings.php key or State key or something to see if you should trigger the legacy `additional` logic. You could also write a cron queue job that processes revisions nightly between 10pm and 4am. There are lots of options here. Performance of running the update isn't the biggest concern to me. I've added a link in the CR to comment #245 who might that helpful.

For other places where this happened, look at the Group v2/v3 => v4 upgrade path. Its a bit gnarly but seems to work.

re #253/254: I came to the exact same conclusion as Adam after I had to convert some custom code to using tps. It was very specific to that custom module. I also ended up writing the upgrade path as a BC compliant solution so it would convert on page load. Of course, that is all very custom to that site's use case. There could be a hundred ways to convert over.

Secondly, is there any value in converting? Yes. If you ever tried to enforce config schema on additional vs tps, you'll immediately see that its impossible with additional. I've been waiting for tps on sections for several years for just that reason. Sure it will be painful. Arguably though we shouldn't have use additional in the first place. But we live and learn.

This can go back to RTBC.

From re-reviewing the docs on options_allowed_values, I think we need to cache it on both, yes. Leaving at NW for this.

heddn → created an issue.

This is a straight 10.4 re-roll. Git stats between the patch in 72 vs this one are pretty close so it shouldn't be missing anything. Leaving at NW since no tests were added, nor was it even modernized for Drupal 11 and attributes instead of annotations.

Always helpful with a test.

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

This can now be worked on. The lone dependency landed.

I think we are now at valid test failures. The library got downloaded fine at this point. I ended up copying from gitlab-ci on dropzone.

From https://git.drupalcode.org/issue/file_browser-3451080/-/jobs/4258213, we should address the undefined calls to libraries_get_path

And that might be why tests are failing in https://git.drupalcode.org/issue/file_browser-3451080/-/jobs/4258217. Maybe.

Dropzone has a js library that doesn't natively get pulled in. My guess we have to add a before_script section to gitlab-ci to trigger download of the js library into the correct location.

Events are fine. Hooks are fine for what they are, but events seem less hacky.

Can we fix the same issue for D7? Pretty please.

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

CR added and feedback on MR addressed.

I didn't do any manual testing. Are there any automated tests that would at least prove out some common functional differences?

Added an MR

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

Maybe we should open a new issue for additional security releases. But for now, bumping the title to keep in step with the version.

🌱 Stable releases for D11 support Active is now opened to coordinate a stable release.

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.