🇳🇱Netherlands @Eric_A

So now there's only a choice between either running an insecure 10.3 application or live with an 10.3 application having the out of memory loop issue?

What is the point of having a security supported core version if you cannot update to secure symfony dependencies?

My bad. The loop with the out-of-memory was mentioned in 🐛 DefaultExceptionHtmlSubscriber should not clone the request for 401s Active .

The point more or less remains though. Without this fix in 10.3.x it's a choice between secure dependencies or a pretty broken site.
Granted that out of memory seems to be more of a problem then "just" an exception.

So now there's only a choice between either running an insecure 10.3 application or live with an 10.3 application having the out of memory loop issue?

What is the point of having a security supported core version if you cannot update to secure symfony dependencies?

Patch #38 doesn't apply to Drupal 10.3.0. (It does apply to 10.2.7)

Something must have changed in FieldMissingTypeTest.

patching file core/tests/Drupal/KernelTests/Core/Field/FieldMissingTypeTest.php
Hunk #1 FAILED at 56.

Patch #5 does provide the default alt text from the media entity. Thanks!

However, using language construct empty()is buggy in that it becomes impossible to enter value "0" as an override. (Values "0", NULL and FALSE will result in TRUE when passed to empty().)

I haven't looked into how core does this with core media. Not familiar with these plugins at all.

(Alternatively the old behavior could be restored by having the importer first saving any new entity before continuing...)

Before this I think it was possible to call Feed::refreshItems() on a new, unsaved Feed entity.
Since 2.1.0 this breaks ugly at the point when loading the feed by ID gets triggered and a NULL collection can't be found in the key-value store.

Perhaps change record information will suffice. Perhaps a more meaningful exception should be thrown. Possibly as early as in Feed::refreshItems().

Not sure at this point.

This now needs a small cleanup after 🐛 Consistently sort filter formats to simplify config exports Fixed .
As for the needed update function:filter_post_update_sort_filters() from that issue must be perfect example code.

Eric_A → created an issue.

Thanks!

While running 2.7, at some point I noticed that the Dutch translation file had added a translation for "You can't upload files of this type." But the site still showed the English source string.

I applied the patch in #2 to 2.8 and it fixes the problem.

It seems that this has accidentally broken node form comment settings for bundles that have comments configured as hidden. See 🐛 [regression] "Comments field is required" when creating content for types with a comment field configured as hidden Fixed .

The issue that accidentally broke the node form was filed in the "field_ui.module" component, but I guess the bug it introduced should be in comment.module.

Eric_A → created an issue.

I guess I'm too busy or too lazy to dive in right now.
But why does the early empty check in HEAD even trigger?
I'll dive in later...

Wow, that's a lot of code to review... Is all that code strictly necessary to fix the critical regression or is it more of an improvement?
I'm wondering if we can just put back the DropzoneJS check that was removed in 🐛 Validation missing when empty bulk media upload form submitted Fixed .

@joseph.olstad, are you sure you can reproduce the scandir issue with 1.0.x HEAD? Berdir (and I) are observing that a fix for that issue is committed and pushed to the 1.0.x branch. Of course we can all agree that the issue is present in 1.0.1.

@Taran2L, to get this properly reviewed and to RBTC, comments on the functional manifestation and severity of the *library definition bug* will help a lot. The library definition change from #7 looks simple enough. It's hard for passing by code reviewers to judge wether anything else is needed if there is no information with regards to manifestation and severity.
(The scandir issue on the other hand is very clear to me. I experienced the PHP errors with 1.0.1 and got it resolved by leveraging the pushed fix, mentioned by Berdir in #5.)

To summarize:
In #7 there's code for a library definition bug, but no information on that in the issue title or issue summary.
In #7 there's a very nice-to-have improvement on the existing scandir code. Of course absolutely fine, but at the same time distracting from the library definition issue. Or the other way around.
Clear scoping will get issues much faster to RTBC and RTBC is when committers really start to pay attention.

Some accessCheck() instances are being passed FALSE, which is a change in behavior for both Drupal 9 and 10. Is this just fixing a recent regression or is this a true change/ fix without discussion. Does anybody care to elaborate?

@joseph.olstad, the point that @Berdir makes in #5 is that the scandir issue is already fixed in HEAD. (Fixed by Wim Leers, lauriii and others.)

The point I'm making in #16 is that @Taran2L is
1) Cleaning up the scandir fix that is already in HEAD.
2) Fixing a library definition bug. It's not immediately clear to me what that is about. It's not "Fix scandir error in Drupal 10".

I'm proposing to split off either the fix or the improvement to its own scope and issue.

Yes, the widget is just broken right now. (Unless you're on Drupal 9, because the deprecated code still works on previous Drupal.)
But if one states that the module works on the current Drupal release, then it's pretty critical that it works on the current Drupal release.
The fix is simple. And passing nothing will just keep the old access check behavior. But without the fatal error.

@Berdir, not quite - patch here fixes another occurrence of the core/vendor path + committed patch uses an extra service while there is already injected one

@Taran2L, what is he scope of this issue then? Is it about some bug around asset skins/moono-lisa/editor.css (how exactly does that bug manifest itself) or still about a scandir error?
On the surface it looks like #7 is doing two completely different things:

1. a path fix for a specific library asset
2. an internal technical improvement to the directory scan

The title of this issue however is "Fix scandir error in Drupal 10" which indeed appears to have been fixed already in #3311367: Fix "Unknown themes: classy" error in the tests → , although it looks like that could possibly be fixed in a much cleaner way. Not a functional bug anymore, however.

Much was done in other issues and is already in 2.0.x. This is now one of Needs work/ Duplicate/ Outdated.

Wait a minute... The Twig replacements might rule out Drupal 8 anyways... If so we should do so explicitly in info.yml en composer.json, because Drupal 8 then no longer conflicts because of a dependency, but because of of our own code.

Thank you!

I think it's indeed better to use the available messenger than using the dump() developer method which might not be available. The mesenger is already in use a couple of lines later at the end of the same render() method.
Wether it should be an error or a warning, could be a matter of taste here.

Also adding some related issues for Calendar 1 and Calendar 2.

One more ordering issue:

diff --git a/composer.json b/composer.json
index 3421c73..432f446 100644
--- a/composer.json
+++ b/composer.json
@@ -21,7 +21,8 @@
     "source": "https://git.drupalcode.org/project/calendar"
   },
   "require": {
-    "drupal/core": "^8 || ^9",
-    "drupal/views_templates": "^1.1"
+    "drupal/core": "^8 || ^9 || ^10",
+    "drupal/views_templates": "^1.1",
+    "drupal/color": "^1.0"
   }
 }

Core deprecation message for debug():

deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Use dump() instead.

Actually, using dump() seems problematic outside of tests, given that symfony/var-dumper is not a dependency of drupal/core itself.

Thanks all for working on adding Drupal 10 compatibility to Calendar 1!

Calendar depends on the core/jquery.farbtastic library as well, which is provided by the deprecated core/color module.

The jquery.farbtastic library was provided by core, not by color module. But yes, it is now provided by the contrib color module as color/jquery.farbtastic and it seems like a reasonable idea to depend on contrib color module. Effectively this means that if this code gets merged today, then Calendar 1 will indirectly be depending on Drupal 9.4 going forward, and effectively alpha4 will be the last Calendar 1 release that runs on Drupal 8. Doesn't look like a problem to me, though...

The initial patch converts debug() to dump() while #5 converts debug() to $this->messenger->addWarning().
Core deprecation message for debug():

deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Use dump() instead.

The initial patch uses fully qualified names for the Twig classes, while #5 uses imports. Importing looks like the way to go to me.

Finally:

diff --git a/calendar.info.yml b/calendar.info.yml
index 1deda9a..5f4865c 100644
--- a/calendar.info.yml
+++ b/calendar.info.yml
@@ -4,8 +4,8 @@ description: 'Provides a Views plugin to display views containing dates as Calen
 dependencies:
   - calendar:calendar_datetime
   - drupal:views
+  - drupal:color
   - views_templates:views_templates

diff --git a/calendar.libraries.yml b/calendar.libraries.yml
index fc5f265..4fe1158 100644
--- a/calendar.libraries.yml
+++ b/calendar.libraries.yml
@@ -3,7 +3,7 @@ calendar.colorpicker:
   js:
     js/calendar_colorpicker.js: {}
   dependencies:
-    - core/jquery.farbtastic
+    - color/jquery.farbtastic

I think we should nstick with alphabetical order. It's the way to go to easily find stuff and to reduce chances on merge conflicts. (And latest Coder now even has a rule for this, if I'm not mistaken.)

As so often, the patch against source does not apply cleanly to the dist, because of the extra lines of context in the info file generated by the d.o. release build system.
Here's a patch file that applies to the current dist. ( https://www.drupal.org/project/views_templates/releases/8.x-1.1 → )

On a side note:

I have mistakenly uploaded the same patch as the one uploaded earlier by Bot.

Nope, #7 and #2 are not the same.

Eric_A I think that's a good idea but we can do it in a dedicated issue, can you open one?

📌 Update guzzlehttp/psr7 constraint Fixed

Changing ^2.4 to ^2.4.5 for 10.0 and 10.1. It's not an issue for the other branches because guzzlehttp/psr7 is not a core root requirement there.

Eric_A → created an issue.

If I'm not mistaken only core-recommended constraints were updated and not core constraints.

Isn't the current policy still to also up core constraints (caret) for security updates? Meaning not just core-recommended constraints (tilde)?

Like for example when twig/twig was upped in core from ^2.15.0 to ^2.15.3 in 9.4.7. (https://git.drupalcode.org/project/drupal/-/commit/82a7d4dd3077ef16b69f2...)

I think there are more recent examples out there, but not able to give one right now.

To be more precise: I am proposing to change ^2.4 to ^2.4.5 for 10.0 and 10.1. It's not an issue for the other branches because guzzlehttp/psr7 is not a core root requirement there.

If I'm not mistaken only core-recommended constraints were updated and not core constraints.

Isn't the current policy still to also up core constraints (caret) for security updates? Meaning not just core-recommended constraints (tilde)?

Like for example when twig/twig was upped in core from ^2.15.0 to ^2.15.3 in drupal/core 9.4.7. (https://git.drupalcode.org/project/drupal/-/commit/82a7d4dd3077ef16b69f2...)

I think there are more recent examples out there, but not able to give one right now.

As it is, this issue is no longer about updating, so changing the title accordingly.

I think we're realizing that it might be smarter as D9 continues in the next couple of months to rely on drupal/core-recommended less often. I feel like we're more likely to keep running into this issue with old versions that do not get security updates anymore, than backwards-incompatibility blockers in minor version upgrades.

True, especially for sites that are getting ready for for D10. The problems now with core-recommended:
1) Drupal 9.5 still supporting an ancient version of PHP, which is a pain from the maintainer point of view only.
2) Not having a constraint that also allows the minor version that is used by or would have been used by the next major version of core is a pity. If the core-recommended constraint was "~2.14 || ~2.18" then a lot of sites on core-recommended would have been able to update already. There are of course better examples to reason about then laminas/diactoros as this particular one is not in the next major version of core anymore.
I'm sure this is discussed elsewhere, but don't no where right now.

And more: 🐛 Argument 2 passed to _readonlymode_form_list_check() must be of the type array, string given Active

Was this fixed in 🐛 Config schema errors Fixed ? Or is it partially fixed?
The merge request here is all about arrays, while the other issue further stringified things.

If you're on Drush 9 or Drush 10 it would appear that you must be running Drupal 8 or Drupal 9 and not 10. If you're on Drush 11 you could still be on Drupal 9.

So the command class must be compatible with Drupal 8 and 9.0, 9.1 and 9.2 if the info files claims that the module code is runnable on ^8 || ^9.
Old potx.drush.inc files are only loaded by Drush 8 or lower, so that file must be compatible with Drupal 8. Drush 9 is not compatible with Drupal 9 and higher so no point in trying to handle thoses versions.

Ok, things are not that simple.

There are two drush command files. One that is loaded by Drush 8 and older and one that is loaded by Drush 9 and higher.
These Drush versions have their own Drupal core version compatibility.
https://www.drush.org/latest/install/#drupal-compatibility

I've updated the patch of #16 to be compatible with Drupal 10 (and the latest version of Drupal 9).

Hold on... Aren't *.drush.inc commandfiles only loaded by Drush 8 and earlier? Drush 8 is not compatible with Drupal 10 and Drupal 9. (It's Drupal 8 compatible but not recommended.)
So it seems then that for Drupal 10 only the new Command class should be touched and the potx.drush.inc should be left alone for those on old Drush and old Drupal.

https://weitzman.github.io/blog/port-to-drush9
https://www.drush.org/latest/install/#drupal-compatibility

More or less postponed on 🐛 The potx info file incorrectly claims ^8 || ^9 || 10 core compatibility Needs work ?

Eric_A → created an issue.

I've updated the patch of #16 to be compatible with Drupal 10 (and the latest version of Drupal 9).

The change involves the deprecation of drupal_get_path() in Drupal 9, see https://www.drupal.org/node/2940438 → . In case older versions of Drupal should still be supported, instead of $path = \Drupal::service('extension.list.theme')->getPath($theme);, something like $path = function_exists('drupal_get_path') ? drupal_get_path('theme', $theme) : \Drupal::service('extension.list.theme')->getPath($theme); (but in that case this change/fix should also be applied on the code handling extracting string translations from modules).

According to the info file, potx is runnable on ^8 || ^9 ||^10. If that were true, then #19 would not be correct is as. However, looking at the code from the D10 compatibility issue it is clear that the new services are already in use since 1.0.0. That code will only run on higher versions of Drupal. According to the referenced change record that would be ^9.3.
So I guess the ideal way forward is to first tackle the version compatibility issue in a separate issue where the code will either be made more compatible or the info file would be fixed to declare a more restrictive version constraint.
I'll see if I can create that issue today.

So here's something of an interdiff. I'll try to review later on.

Right. The two relevant files contained irrelevant hunks from the D9-D10 commits. Removed those manually as well.

To facilitate reviewing I created an interdiff between #16 and #19 with git. While #19 applies on HEAD (1.0.0), for #16 I had to go back to 2 commits to Drupal 9 code. The interdiff contained a lot of files. I then removed all the files except the two that are changed by both patches.

So here's something of an interdiff. I'll try to review later on.

Thanks, @Anybody.

Turns out my patch is kind of a duplicate of the latest merge request in 🐛 Incorporate Drupal 9.4 core changes to getLanguageSwitchLinks Fixed , which currently really has an outdated title...

The title of this issue really is outdated, though. And not helpful. :-)

Looking at https://git.drupalcode.org/project/dropdown_language/-/merge_requests/3/... I see the same fix as the one I just provided in 🐛 Warning: Attempt to read property "links" on null Closed: duplicate ( https://www.drupal.org/files/issues/2023-03-16/attempt-to-read-property-... → )

The only difference is the local variable name: my $language_switch_links vs languageSwitchLinksObject.
Up to the maintainer which version he wants. I'd say pick one and credit us both. :-)

I think we should close this outdated and instead handle the other follow-up issues to prevent confusion.

@Anybody, thanks for looking into this.

What follow-up are you referring to? (As far as I can see 🐛 ->getname() is called on $link['language'] = NULL Closed: duplicate is not related at all.)

In fact, with the fix for SA-CORE-2023-003 in Drupal 9.5.5/10.0.5 it may even be more relevant to get #17 in to fix the warning and accompanying type errors for people on current PHP.

No interdiff. Different (but easy to grok) approach. Really small patch.

I'll provide a patch that works on both current PHP and PHP 7.4.

LGTM!