🇬🇧United Kingdom @james.williams

james.williams → created an issue.

OK, done!

(Tests all pass, having updated them to account for this, since they already assert the contents of the column that would now include the bundle text. Those existing assertions mean new tests would be unnecessary, I believe.)

Updated MR to account for entities without bundles, and those that use a bundle key other than 'type'.

P.S. The MR uses a branch named 8.x-2.x, which is a bit awkward for when the canonical repo makes updates to the branch of that name! Never mind; I've merged the changes from the original repo's branch into this issue fork's one.

james.williams → changed the visibility of the branch 8.x-2.x-issue-3244334 to hidden.

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

I tried the patch at #158, which does apply to Drupal 10.4, but in my usecase does not fix the issue.

@alberto56 What about your use case meant that updated path didn't work? I don't recall there being much fundamental difference. Would you be able to test the work in the merge request !10820? If there's something it's missing that it shouldn't, I'd love to address it!

The work to review is in the merge request, not the patches, so that issue status change is incorrect.

Argh, sorry, that last patch for D10 causes an error due to a missing trailing comma. This one is better!

Here's a patch that applies to D10.4.x or D10.3.x, using annotations instead of PHP attributes.

I also noticed that one of the functional tests had unnecessarily failed; I re-ran it and it passed. Interesting that it happened in a relevant test, but it was on an assertion about caching that doesn't look to me like it should have failed. I think caching is the kind of thing that can intermittently misbehave in the test runner?

I'm doing some local testing, but this is a promising solution. I had wondered about creating a contrib module to address the problem, but I'm reluctant to do so if that might make it less likely for a solution to arrive in core! Unless anyone can suggest why it might be better/worse to address this in contrib?

Does https://www.drupal.org/node/3414904 → mean this is no longer needed, as I think blocks can be placed in the navigation? I could be wrong!

Reverting status, since that last change is explicitly not wanted.

OK, that makes sense - but then makes the method name rather confusing. Getting a 'service by class' which isn't even the class you asked for, is straight-up confusing. So maybe a name of Drupal::serviceByInterface() would be better? Or, since many services are already aliased to an interface name, could something like phpstan-drupal be made to understand that \Drupal::service(MyInterface::class) returns an instance of MyInterface to avoid the need for this at all?

Making changes for the sake of the tooling still feels a bit like the 'tail wagging the dog'.

If the service registry allows swapping/overriding/decorating the class for a service, how does that interact with this? If you pass a decorated class to serviceByClass(), should it return the decorator, or the decorated class? If you pass the class of a service that has been swapped for a different class, which do you get? I'm not convinced this is a good idea, I'm concerned it could bring confusing DX for the sake of helping tooling rather than anything functional.

There is one issue I noticed: the option "Use the current content language to generate a url (rather than the language of the referenced entity)" is always checked in the text format form. Even if I uncheck it and save it, after a refresh it will be checked again.

@gillesv good spot! That was introduced back up in comment 17. I've added a fix in the MR.

@j-lee try this attached patch for 10.4, which was from a snapshot of the changes in MR !5373 from before the attempts at resolving the merge conflicts.

I believe 4.x will have the same issue; but I've made this against 8.x-3.x which I can more easily demonstrate & test against. It's not clear to me what the status of the two branches is anyway, maybe a roadmap needs posting up on the project page?

Anyway, nice & simple MR ready to review :)

james.williams → created an issue.

Ah, I've been informed those package_manager Build tests can intermittently fail (thanks @nicxvan!); and yes, having now just re-run them again, they pass. Just the tests-only checks fail now, which is as intended.

james.williams → changed the visibility of the branch 11.x to hidden.

I've opened MR !10820 with the same changes from MR !5373 from before that attempt at updating its branch, plus some fixes for the static code checks that have tightened in the meantime and a bump for the deprecation target version.
Compare with https://git.drupalcode.org/project/drupal/-/compare/d766bbd0ec9a2a63a5c8... - which compares the commit from MR !5373 before the merge attempt, with the current HEAD of 11.x.

Unfortunately the package_manager module's Build tests are now failing, which is a mystery to me. Those tests pass locally for me! The test output includes the "Failed asserting that 500 is identical to 200" message ... but how would I diagnose what is causing the 500?

The last review was from @catch on 21st October (comment 142), for a couple of code improvements, after @smustgrave had set this to RTBC. I really hope we can get this resolved ASAP, as far more time has been spent addressing issues due to having to keep up-to-date with the goalposts moving, than actually addressing the problem :'-(

Oh dear. But thanks for trying to help. There were still merge conflicts anyway, so I tried reverting that merge commit, and then re-merging, but that's basically saved reverts to what was in 11.x rather than fully merging the changes back in from 11.x. Hard-resetting the branch to the previous commit isn't possible either. I'll see what I can do but it might be better to just spin up a new MR. Would just be a shame to lose the history.

I have merged some of the proposed change, but not all of it. Some of the changes are really about compatibility with certain versions of PHP, Symfony, etc, which don't have to be conflated with Drupal 11 compatibility more generally. Thanks for the changes that are useful.

I have committed the changes from MR !9, alongside a return type compatibility fix that hadn't been flagged up by any of the proposed changes, but was necessary for tests to pass! Shows how 'real' testing is so much better than automated scanning 🤨 Hopefully that will stop unnecessary changes being proposed now anyway.

@thiagomoraesp when you say the problem persists - what exactly are you seeing vs what you expect? Do you have a module implementing code>hook_language_fallback_candidates_alter() (such as https://www.drupal.org/project/language_hierarchy → ) that would actually make use of this fix? The fix on its own shouldn't trigger any immediate improvement, it only facilitates other modules implementing change.

@alberto56 that looks like a reroll of the patch from comment 13? But the patch in comment 14 has the latest work, so that's the one that should be re-rolled, if even still necessary.

I've left some feedback on the review. Generally, why was MR !10 necessary, with changes beyond what was in MR !9?

Sorry that still doesn't explain anything here. Why is this issue open? Does it represent something to be concerned about? Can we just close it?

It's only the test against the previous major PHP version which fails for MR !52 (the one for 2.x), which also fails for the current 2.x HEAD. So I think this is good for review again? The tests in MR !53 (targeting 3.x) are passing fine.

@zterry95 please could you clarify what this issue is about? Feel free to message me directly on https://www.drupal.org/user/592268/contact → if you want to keep the details of a security issue private.

Thanks for the review!

I believe this is due to the module's invalid approach of trying to make the asynchronous Stripe payment request whilst building up the AJAX request to submit the form, which ends up with duplicate AJAX submissions. I actually reckon it's therefore quite likely to be a similar issue to webform's own issue 🐛 Ajax integrity constraint on uuid with autosave and ajax-compatible confirmation type Needs review .

We've got a merge request with a fix that I reckon could well fix this problem, with full details written up on #3401338-10: Several errors and warnings in the console → . Please can you try that out to see if it solves your problem? Thanks!

See #3401338-10: Several errors and warnings in the console → : this is due to the module's invalid approach of trying to make the Stripe payment request whilst building up the AJAX request to submit the form. We've got a merge request there with a fix that I reckon could well fix this problem entirely. Please can you try the fix out to see if it solves your problem? Thanks!

See #3401338-10: Several errors and warnings in the console → : this is due to the module's invalid approach of trying to make the Stripe payment request whilst building up the AJAX request to submit the form back or forward. We've got a merge request there with a fix that should fix this problem too as it moves away from overriding any standard AJAX behaviour :) Please can you try the fix out to see if it solves your problem? Thanks!

I've spotted that the approach of overriding the ajax.beforeSend method cannot be relied upon, because Stripe returns the payment status asynchronously. So the beforeSend method itself got marked as async, which means that when Drupal is preparing to make the AJAX request, it calls the beforeSend method but also continues on to send it, regardless of what happens inside the beforeSend() method. So if the response from Stripe is that the payment has failed, the call to xmlhttprequest.abort(); may happen after the original AJAX call which submitted the form back to Drupal has already been made!

This also means that even when the payment succeeds, the usual AJAX submission back to Drupal that the submit button triggers has already been sent. If Stripe processes the request to it quickly enough, the payment will go through (as noted in 🐛 Payment Received, no form submission Active )
and Drupal considers the submission as completed, but the module javascript clicks the submit button in submitWebform(), causing another identical AJAX submission back to Drupal - and the Integrity Constraint Violation error like the one reported in 🐛 Integrity Constraint Violation Active . (And in fact, in the webform module's own issue 🐛 Ajax integrity constraint on uuid with autosave and ajax-compatible confirmation type Needs review !) That duplicate AJAX submission will therefore report an error, making it look like the payment didn't work, when actually it had!

In addition to all this, the override of beforeSend() means the payment request is triggered for any AJAX functionality added to the page - including the webform's button to go back to the previous page (as reported in 🐛 Previous button behavior like submit button Active ).

So basically, there are too many problems with this approach of trying to abort an AJAX request according to the result of an asynchronous call made whilst preparing the AJAX request. Instead, in my new commit to the MR, I've cloned the original submit button with an identical one that won't trigger any of the AJAX requests or submit the form, and hidden the original button. It has a custom click handler that will call the asynchronous handlePayButtonClick() method to make the payment request, and only once that returns successfully, will the form submission AJAX request be triggered for the original submit button.

This could still do with some more testing (e.g. what happens when a payment fails now? do we need to re-enable the new payment button to allow users to try again?), but Irina tells me it's looking good so far :) (We're colleagues so have worked through this challenge together!)

I've linked the various issues together here, but I'll make some updates across each of them explaining this again so the various followers can be pointed towards this potential fix.

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

Re-rolled for 10.3. (I don't expect this to get into core any more; it's just most useful for sites needing to upgrade that have been using the previous patches here.)

@zterry95 Unless I hear any other objections, I'll merge the MR this week. It's working well as far as I can see, and I've incorporated a couple of little improvements that a colleague of mine has suggested after they reviewed the code too - including using case insensitive checking on the HTTP Content-Type header.

I trust that will be OK with you - of course let me know if not!

james.williams → created an issue.

Thanks very much! I've opened MR !8 to introduce configuration for allowing logging when there is no response (defaulting to not), and filtering by response content type (defaulting to XML, JSON and HTML as currently). No doubt this could do with some more testing and a second pair of eyes.

By the way, would you be up for switching the default branch of the repo to 1.1.x instead of 1.0.x?

Tests are finally passing via phpunit :)

Thanks for the reviews! Should be good again now 👍

Finally got a working test, failing without the fix to demonstrate that it is needed, and passing with it!

Silly me for thinking that this could have been suitable for novice... I took hours fighting to get this to work! So many dark corners in the test system.... e.g. you can't assume the system module is enabled, and it's best to pass proper Url objects to $this->drupalGet(). Ah well, now I know :)

Linking a couple of issues which are for functionality that language hierarchy already has. (From my point of view, their functionality is essential. But the ELF maintainers don't seem to have recognised either as so necessary for ELF, given neither issue has progressed for over a year, despite each having already been supported in LH for much longer.)

Even if these were resolved and the two modules could reach feature parity, the fundamental difference in approach is still whether to have different fallback chains per-language vs a single global hierarchy of fallbacks.

FWIW, if we want to unite efforts - I'd love some help in completing the core issue about fallbacks for path aliases, which holds back each of our projects: 🐛 path.alias_repository service does not use a proper language fallback mechanism Needs work .

@kristiaanvandeneynde thank you for spotting that and pointing it out, yes! Next step then is to get the test demonstrating the actual problem. It should fail without the changes, but pass with them - whereas it's now passing in both situations.

Oh, thank you! Do you have any particular plans or ways of working on this module that would be worth discussing so that we can align well?

Thanks @catch ... ready for review again... :doubly_crossed_fingers:

P.S. As I'm a keen user of your module, and have given various contributions in the past already, I'd be happy to help co-maintain this module if you would like that?

james.williams → created an issue.

What needs doing here? Seeing this issue about security, in the public queue, with no details, makes me nervous that there's some unresolved security issue in the module. Maybe it's fine, but it's probably worth posting some details if there's some task awaiting completion, to help build confidence in using this module please.

MR !7 should resolve this.

james.williams → created an issue.

The new test passes without the fix, which can't be right. I don't really understand what that new test is actually doing - I'm not sure it really represents the problem at hand.

We've also got an issue in that existing core tests fail with the change, with this warning:

Trying to overwrite a cache redirect with one that
has nothing in common, old one at address "languages:language_interface,
theme, user.permissions" was pointing to "url.path.parent,
url.path.is_front", new one points to "route".

Although the test itself doesn't look relevant, that does sound like we've adjusted cacheability metadata in some way that hasn't been expected. More work needed on all fronts!

The update hook is for sites that had language hierarchy installed from before the schema hook existed. New installations of the module would not run the update hook. If you already had the table on installing the module, or on updating it, I imagine perhaps your database got into an unsupported state somehow? Did something interrupt its installation at some point perhaps, or reset its record of its schema / last installed version?

https://www.drupal.org/docs/drupal-apis/update-api/updating-database-sch... → is the most up-to-date documentation I can find; that shows how both of the hooks are necessary. Update hooks aren't usually designed to be safely run more than once, but of course if you got into this situation, I can understand how your fix would be necessary.

So I'm closing this issue as works as designed, but I'm happy to continue dialog here if you can provide more detailed clearly repeatable steps with a fresh install? (I just tested your steps to reproduce on simplytest.me, using the browser equivalents for running updates, and could not reproduce the error.)

I've opened MR !9913 to demonstrate what probably needs to change; though a test would be needed anyway. Probably using a test module with a service provider to remove the path-based breadcrumb builder, or decorate it with one that won't always apply. The test would then load a page where no breadcrumb builders apply, then another where one should - an assertion should expect breadcrumbs to appear, but the test should fail without this fix as no breadcrumbs would show.

I need to focus on some other work for now, so I reckon maybe that's something a novice could pick up?

Thanks for this improvement! I have though found a bug for a follow-up: the cacheability is ignored if no breadcrumb builders apply, which can cause breadcrumbs to disappear across a whole site. Reported as 🐛 BreadcrumbManager ignores cacheability when no builders apply Active .

james.williams → created an issue.

Going by catch's update in #2484411-36: Manual path aliases are not the same as aliases on the node form → , I've updated the issue summary to say this issue is now just about fixing the the language defaults on the path alias admin UI. Together with Berdir's comment #2484411-42: Manual path aliases are not the same as aliases on the node form → , it sounds like the decision was made that an upgrade path is no longer necessary. There was also mention in other comments (e.g. #11) about defaulting the language selector to use the default language on multilingual sites, which isn't strictly within the scope of the current issue title, but sounds very relevant to me so I've included it in my issue summary update.

The most recent patch still includes the upgrade path, but is also very old - I'd be amazed if it still applies! But in the meantime, 📌 Use interface language on the add URL alias form Active contains an mergeable MR which I believe is a solution for this anyway. It's tests need some work, maybe we can bring across updated tests from here to there, or the work from there to here?

I interpret the current ticket description as being about ensuring that the right language is selected for creating aliases in, from the URL alias form. At the moment, the alias form creates aliases as Language Neutral (shown as 'Not specified', see the screenshot I've added for the issue summary). So regardless of whether a site is monolingual or multilingual, aliases are not currently created in the interface language. I believe that is what a valid test should show.

Normally a test should fail to show an expectation isn't being matched, and then pass when run alongside a fix. At the moment, the test is passing against core itself, because it is not checking the unexpected behaviour. Yes, perhaps that's just because the issue wasn't clear enough - so I've updated the steps to reproduce, including that screenshot.

Another way of testing could be to check whether an alias added through the standalone URL alias form is given the same language as an alias created from a node creation form. At the moment; they would be different. That is the angle that #2484411: Manual path aliases are not the same as aliases on the node form on single-language sites → looks at this problem from. I believe both of these issues can be solved with the solution that I added in MR !8175 for this issue.

My MR changes did go a bit beyond the basic request, to respect the configurable language content settings for more flexibility for different use cases.

I've run the test-only pipeline job, which passed - i.e. your test passes without the changes. The test is simply checking that aliases work for nodes that happen to be called 'English node' or 'French node'; it's not checking that the aliases are created in an appropriate language for appropriate node translations. I also believe the monolingual scenario should be covered: aliases should be created in the same language on the standalone alias form as when creating them via the node add/edit form (i.e. the site default language for both; whereas the standalone alias form creates aliases as language-neutral without these changes).

Hi, thanks Joachim. I've written up what I was up to https://www.computerminds.co.uk/articles/automatically-generate-forms-co... which was for a configurable block plugin in which I explored generating as much of the form as possible from the config schema. I've posted my code up at https://gist.github.com/anotherjames/bcb7ba55ec56359240b26d322fe2f5a5 . Here are a few thoughts from my early experiments with this...

My case was a configurable plugin form, whereas the original suggestion here is for forms extending ConfigFormBase. Can we help both cases, and maybe more? There is code currently in config_translation for automatically generating forms from config schema, such as config_translation_config_schema_info_alter() and the parts of ConfigTranslationFormBase::buildForm() that fetch the schema and build form elements. So maybe the first step to achieve this feature request is to identify and refactor those parts out of the config_translation module to become more useful in other cases (including monolingual sites!). Of course having a concrete use case (like the suggestion for ConfigFormBase) might help pull that possibility along, and then facilitating other cases could be added.

The config_translation approach uses a 'form_element_class' key in the schema; what other keys specific to this could be supported usefully? In my example, I added 'default' (which I then used in my plugin's ::defaultConfiguration() method) and 'locked' (for #disabled on the form elements). Maybe that's just scope creep, but it hints at the possibility of shifting the inherent definition of config data into the schema rather than form API.

Things like option values would still need to be provided in the form.

Not necessarily! If a data item only has certain allowed values, those can (should?) be defined as constraints on it - e.g. using the Choice constraint → (and/or Enums?!). Then the form element could be automatically generated with its #options set from that. There will always be cases where it makes sense for a form element to be customised further (e.g. for things like #empty_option or #placeholder) which really are only about the form, not the inherent definition of the data. But that just means a form making use of this could just merge in those changes after calling the parent/trait method implementation that does the automatic form building.

Thanks for opening this issue and building some momentum around this idea! I thought maybe for now I was just on my own crazy island exploring this idea ;-)

I was looking into whether the work here could help resolve #2484411: Manual path aliases are not the same as aliases on the node form on single-language sites → by ensuring new aliases were created in the same language on the standalone path creation form, but I noticed that the current approach overrides the language when editing any existing path alias entity (which might already have a specific language). I think this should only be about setting the default initial value on creating an alias entity? So I've pushed a fix for that.

I've also made it respect the language settings configured at /admin/config/regional/content-language which allows choosing what the default language for aliases should be (which itself defaults to the site language), so that this is actually configurable too.

This is due to the standalone admin form for aliases always creating aliases as language-neutral whilst the site is monolingual. See #2484411: Manual path aliases are not the same as aliases on the node form on single-language sites → .

Pretty sure this is actually intended behaviour. It's quite valid for a site to have a language-neutral path that might mean one thing across most languages, but something else in a specific language.

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

Here's a screenshot showing how the switcher links look with the languageicons module:

OK, I've opened MR !6 to handle render arrays. I've tested with and without the languageicons module; I believe it works quite nicely! I do love seeing all the little flags in my toolbar now :-)

Please take a look and review/test if you can; thanks!

Thanks for the additional detail, and yes, I think you've put that very well. Let's ensure the module doesn't run into any kind of error. Whether that means just showing the language name as in comment 11 (as a bug fix), or supporting render arrays (as a feature request), I accept that either would be an improvement. And it doesn't really matter what kind of fix this is; I'd just like it to be a good one :)

Perhaps we could support render arrays and see how those from the languageicons module look. Maybe even with something like a max-height / overflow: visible rule to avoid seriously breaking the toolbar appearance if any other modules try to render something especially large?

If none of us get time to work on supporting render arrays any time soon, I'll probably accept the patch from comment 11 that just replaces arrays with the language name.

OK - see my last commit; the format of the title is now configurable (which avoids adding a new translatable string too), and there's a configuration option to avoid falling back to using the video ID. So by default, titles will come out using that '@provider | @title' format, but those of us who might want something else can configure that to '@title' and untick the box to use a fallback at all.

I've spun up a merge request for the most recent patch. However, I'm no fan of the title format, especially as it is forced even when no title could be retrieved from the oembed data - which can make it look something like 'Vimeo | 12345'. Perhaps the title could be made configurable and/or skipped over if it wasn't retrieved from the oembed data?

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

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

This isn't particularly serious, but I just happened to go back and visit my first ever post on drupal.org ... which was this. And since I'm a bit smarter than I was then ... or can at least write code for Drupal more than I could then ... I thought I'd make a solution to satisfy my past self 😆

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

Thanks! And yeah, I did wonder what that missing interface report was about :) I did have a little look into why content might have been (double?-) escaped, but couldn't see an answer.

Yeah, so that message is basically saying: You're using webform_protected_downloads, which needs webform, which isn't compatible with Drupal 11. Composer doesn't read your files on disk for the compatibility data, it's taking that from drupal.org. So even if your local webform.info.yml says it's compatible with Drupal 11, that's only for Drupal's benefit, not composer's. If you want to use a Drupal 11 compatible version of webform, you need to inform composer how to do that. You can use https://github.com/mglaman/composer-drupal-lenient to let composer ignore the Drupal 11 compatibility issue, and then https://github.com/cweagans/composer-patches to add a patch to webform, such as one from the work in ✨ Drupal 11 compatibility fixes for webform Active .

@scott_euser OK, done! I didn't really expect this to be something accepted into the module, as I figured it bypasses what the module expects. But hopefully it's useful for at least 2 of us!

Hmm, that’s strange - what versions of webfrom and core are you using please? The ^6.2 constraint should be respecting semantic versioning so should allow 6.3.*, for example.
I note that there’s no official release of webform itself that is marked as compatible with Drupal 11 by the way. Are you sure that’s not the problem?

Closing due to lack of response; I'll presume my response explained why this functionality is not really suitable for this module and what the alternative approaches could be. Let me know if not though!

Closing due to lack of response; I'll presume the issue was resolved. Let me know if not though!

Having given it a bit of time, and heard no reason not to, I've merged that MR so this is now fixed. I'll create a new D11-compatible release shortly.

Gave some time, and no issues found. Confirmed over in 📌 Compatibility with D11 Needs review that MR !3 works well; I'm going to close these two issues and make a new 8.x-1.x release. I'll merge that work into 2.x as well though, but there's still no major need to move to that branch yet.

@redseujac Please could you confirm which merge request you've tested please? Was it MR !2 from this ticket; or MR !3 from 📌 Resolve issue reported by PHPStan and PHPCS Needs review ?

I'm not in a rush to make a new release that includes this, because webform itself doesn't have a D11 compatible release yet either. I would rather give more time to hear confirmation from a wider set of people that each of these tickets' work is fine (especially from my co-maintainer in that other ticket). If we don't hear anything more by the time webform provides a D11 compatible release, I'll potentially make the release then though.

I had the opposite issue - I had footnotes containing URLs that weren't explicitly links, but the text format for my body text has the 'Convert URLs into links' filter enabled. So https://www.google.com within my content used to come out as an actual link in the footnotes. Since upgrading to 4.0.x, my footnote was just that URL as plain text, not a link.

I attach the patch I used to solve my problem by invoking an alter hook whilst upgrading the content (so I could pass my content through that filter/format in a custom module's implementation of this hook). Maybe this patch can help you too as it allows bespoke tweaks to the upgraded content? And/or it might be worth thinking through what your text formats' configuration will be doing to escape the footnotes' content.

For reference, this is my custom module's implementation of the hook - it's quite specific to my needs, but might be a helpful example:

/**
 * Implements hook_footnotes_upgrade_3x4x_alter().
 */
function MYMODULE_footnotes_upgrade_3x4x_build_alter(&$build, $context) {
  // Upgraded footnote content needs putting through the URL filter.
  $format = \Drupal::entityTypeManager()
    ->getStorage('filter_format')
    ->load('filtered_html');
  if ($format instanceof \Drupal\filter\FilterFormatInterface) {
    /** @var \Drupal\filter\Plugin\Filter\FilterUrl $filter */
    if ($filter = $format->filters('filter_url')) {
      $build['#attributes']['data-text'] = $filter->process(
        $build['#attributes']['data-text'],
        // Language code is actually ignored by this particular filter.
        \Drupal::languageManager()->getDefaultLanguage()->getId()
      )->getProcessedText();
    }
  }
}

@redseujac Files would only ever be deleted on cron once they are older than the 'Delete temporary files after' setting on /admin/config/media/file-system - assuming that's even set to something other than 'Never'. Does that explain why your files are still around? Do they get deleted by cron after that amount of time has passed, which I guess it may now have?

greggles → credited james.williams → .

Ok, thanks for the report! That message indicates something could be logged in the browser's console; you can enable the developer tools and view its console tab to see this. https://www.coursera.support/s/article/learner-000001653-How-to-open-the... might help you view this. The message would only show up at the point of running into this error, so if you've navigated away, it won't still be there if you return to it. But maybe you can put the file back and try to repeat what you did earlier to see if it happens again?

I've now completed my own testing, which did include some more necessary fixes. But I'm confident this is now fixed. Note though that there's a MR including this but also with further code quality improvements at https://git.drupalcode.org/project/webform_protected_downloads/-/merge_r... which combines the work done in 📌 Resolve issue reported by PHPStan and PHPCS Needs review with this. @redseujac would you be up for testing that?

OK; I've now tested my MR !3 ... it did need a bit of fixing, but I'm now confident it's correct.

@devkinetic what do you reckon; would you be up for us merging MR !3 to cover the file extension validator too and without needing to bump the major version? (And therefore fixing 📌 Compatibility with D11 Needs review at the same time too, where we have users pushing for D11 compatibility.)

I've opened MR !3, which merges MR !1 here with the work from 📌 Compatibility with D11 Needs review since that covered 'file_validate_extensions'. I went with a separate MR to keep MR !1 pristine, if we wanted to keep those two pieces of work separate. (Plus I felt bad building on top of your MR !1 @devkinetic !) I haven't completed testing it, but I have managed to get phpcs and phpstan passing on it now. The 'next major' jobs fail because webform itself isn't compatible with D11 yet so the composer build fails.)

Thanks for the bug report! I've updated the merge request with what I believe to be a fix. I'm well aware this is still otherwise untested code. But you can get an updated patch for the updated merge request to test with again if you wish.

@redseujac Sure - there are guidelines at https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... → about how to get hold of a patch file from a merge request. It's worth getting to grips with this process if you're used to using patches, as drupal.org is heading towards using merge requests for code changes instead of direct patches entirely.

Thank you for being willing to test this work in progress! Please understand that the work is in this form (which I recognise is developer-centric), because it's not yet completed testing. Hopefully the guidance in that link above helps bridge the gap for you as a non-developer?