@heddn

fjgarlin → credited heddn → .

The use of $CI_SERVER_URL default variable is problemantic. One suggestion was to use another variable that defaults to $CI_SERVER_URL, so if someone runs this on a non git.drupal.org infrastructure, things can be overridden and things still work.

Our logic around how we use https://www.php.net/manual/en/ini.core.php#ini.precision to get a number is also in error. Its value can be negative 1 (-1). Which really messes with things if we assume it is a positive length to use with number_format.

Re #373: sorry about my poorly worded and unclear statement there. I'm not sure how to word it but making something like this dependent on a PHP ini settings, which is regularly set differently on localdev vs other environments seems wrong. This is the type of thing we would regularly do a hook_requirements around if it were a feature of php for image quality or a feature of mysql for data storage. But we do nothing like that here.

To add to that though, this precision value is something we can control ourselves via an ini_set at runtime. If we have a certain value configured for the field, then we should set the value before invoking it.

I streamlined the approach here a lot. This issue is all about validation. So changes to widgets shouldn't be in scope. And the storage of the data should be limited by the DB engine not by the configuration of PHP. So a lot got stripped out. If we need to address those things, they should happen in a dedicated issue.

A couple things here. 1) We shouldn't do hook updates that look at environment specific changes. Localdev vs live site likely have different php.ini settings. We shouldn't depend on them to be the same. 2) Why do we care what the display precision is set to for data storage? We shouldn't.

I've removed the hook update.

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

The reason for offset errors is that between solr 8 and 9, the defaults for highlighting changed: https://solr.apache.org/guide/solr/latest/upgrade-notes/major-changes-in.... At run time you have to provide hl.method=original.

Highlighting: hl.method=unified is the new default. Use hl.method=original to switch back if needed.

benjifisher → credited heddn → .

Upstream already fixed the regression. This needs a review now.

https://github.com/thephpleague/commonmark/releases/tag/2.6.2

Thanks to the helpful input. I've addressed all feedback on the MR. I re-tested if scenario #2 still works before/after this change and that is still the case. All that wasn't working is scenario #1.

I've opened https://github.com/thephpleague/commonmark/issues/1071 to see if there is an upstream response.

This adds more test coverage for adding relationships when target_id is not the main property. Interestingly, I stumbled upon the fact that JsonApiFunctionalTest does some very similar tests, but with more traditional entity reference fields. Of the 2 scenarios I've added, the first was the only one that actually didn't presently work. The second already worked but I added it so we don't have a regression in that scenario.

Working on some more test coverage and path to patch the addition of a relationship.

Any chance of a new 4.0.1 release to fix this issue with a "stable" version?

Is there an upgrade path between these 2 module names? Can the old module be left around for a while as an empty project with a single update hook to install and transfer all configuration to the new module? Otherwise updating from 2 => 3 is going to be painful.

Another issue we ran into is that v1 upstream support for inline css classes is different than v2. In our case, we don't have that many nodes and blocks so we're manually fixing, but I could see someone benefiting from an upgrade path.

In v1 it supported:

{.display-4.mt-5.mx-auto}
## Here is some H2 text!

V2 requires spaces between each class name:

{.display-4 .mt-5 .mx-auto}
## Here is some H2 text!

This has a failing test now on MR 11771

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

This is the closest open issue. Drive-by review on the current code that is being changed here. PHPStan currently complains about:

public function onChange()
<snip>
    $entity_type = $property->getDataDefinition()->getConstraint('EntityType')[''];
    $entities = \Drupal::entityTypeManager()->getStorage($entity_type)->loadByProperties(['uuid' => $this->get('target_uuid')->getValue()]);
<snip>

$entity_type that is returned is an error and getStorage requires a string.

There was an issue with the last version of the MR (fixed now). If someone had selected additional extensions, they all got removed due to the way that the markdown object gets created.

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

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.