Can these changes please get merged sometime soon? I think this is our only contrib module left that's holding us back from upgrading our site to Drupal 11.
Thank you!
After load-testing the above, this does present serious performance concerns. I load-tested our server with requests for several different types of requests:
- Valid Webform CSS/JS files, e.g.
/webform/(css|js)/<real_form_id>/custom.(css|js) - Paths that get routed to Webform but do not actually exist, e.g.
/webform/(css|js)/<random_uuid>/custom.(css|js) - Requests for CSS/JS files that do not get routed to Webform, e.g.
/<random_uuid>/<random_uuid>.(css|js)
Again, if you enable the workaround I suggested above I strongly recommend enabling "fast 404" functionality instead of having Drupal render 404 pages. With a substantial number of requests (a few thousand per minute), even with fast 404 functionality, our CPU usage gets pegged near/at 100% — if Drupal is rendering "pretty" 404 pages, your website will likely become completely unusable. I would also strongly recommend using a CDN to cache 404s.
It didn't seem to make a difference whether the request got routed to Webform or was just some random garbage request that /index.php didn't know how to handle. Our website gets a lot of requests that would fall under the latter.
Ultimately though, it feels like any path is a risky approach. We don't load any of our forms via Ajax, so we never had any need for the fix that led to this bug. Is there any reason why custom CSS/JS can't be inlined into <style> and <script> tags, or why this behavior overall can't be made configurable? For example, allow the user to choose between the old routes, the Ajax-capable routes, or inline the scripts and styles, perhaps on a per-form basis.
This is happening to us too. We use the wodby/nginx image as our server both locally and on the public internet, and by default it identifies a range of file extensions as static files, including .css and .js. It will then attempt to load requests for paths matching those extensions as static files only, with PHP execution disabled. Where custom.css and custom.js are generated by PHP, that obviously won't cut it.
As far as I can tell, there's no way to exclude paths (in this case, /webform/*) from that behavior. However, for the Wodby image, you can work around it globally by setting the environment variable NGINX_STATIC_404_TRY_INDEX=1, which will send the request to index.php file if/when the "static" file request 404s.
I haven't tried it, and I'm allergic to changing a complex default value, but you can probably also use the NGINX_STATIC_EXT_REGEX env var for the Wodby image to update the default static file extension regex and remove CSS and JS files.
Note that the above fixes are only for the Wodby nginx image though — they are not a cure-all for all nginx setups.
This fix completely broke our webforms' custom CSS/JS because our server assumes that paths ending in .css or .js are static files and attempts to serve them accordingly (that is, without PHP). We are running Drupal 10.3.1 / Webform 6.2.7 behind nginx 1.24 using the wodby/nginx Docker image.
I believe this behavior on the Docker image's part is configurable by either no longer treating CSS/JS requests as static files (which might break the ones that actually are static files...we haven't tried it yet) or by passing static file 404s to index.php, but I feel like we're justifiably reluctant to do either one of these. Our website sees a lot of random garbage requests, and we don't want to risk those taking down our website.
It'd be nice if we had the option to use either the old routes (which never caused a problem for us) or to use the new paths in this fix.
I narrowly prefer a one-liner for this case but agree there's value in listing the commands separately with the explanation/troubleshooting steps. Perhaps a solution is to add a "copy install commands" button to the "view commands" modal that just uses the Clipboard API to copy the one-liner command, while leaving the remaining content basically as-is.
That way those who just want to copy-paste can do so quickly (arguably even quicker than if we just printed out the one-liner) and the added detail is there in the existing content for those who need it alongside the individual commands. I'd be happy to help with that!
I disagree that a legend isn't helpful. Accessibility guidelines suggest that icons should be accompanied by visible text descriptions wherever feasible, and tooltips are generally not an option when a user isn't using a pointer device. This makes the alt-vs-title attribute issue 🐛 Security and maintained icons need to communicate correct information Fixed less of a problem because visible text should always be read by screen readers, and it makes icon descriptions available when not using pointer devices. While it'd take some rearranging, I think it'd be more than feasible to include brief text descriptions in the cards. It could even be something like "Covered" and "Maintained" with a more verbose description in the detail view (which is effectively what we're doing already).
More broadly, I feel especially for people new to Drupal that we'd rather make this information readily visible instead of making them go out of their way find out what it means. I feel like "hover over something for a tooltip describing it" is no longer something we can count on people to know to do — if it's even possible on their device.
Thanks Chris! I wanted to add that I kept the chip background color gray because blue is intended for the "active" state of a chip per the Drupal Design system Figma document. However, there is no "inactive" counterpart in this case. Given that, it seems best to stick with the "default" state as pictured.
Per @gbois's comment above, I replaced the chip close icon with that from the Drupal Design system and removed the old one, and I updated padding accordingly. Merging in months' worth of changes led to some craziness but it looks like stylelint is no longer complaining. The merge request title is no longer accurate; I'm new to contributing and can't figure out how to deal with that — very sorry! That aside, I think this should be ready.
Per @gbois's comment above, I replaced the chip close icon with that from the Drupal Design system and updated the padding accordingly. Fixed stylelint issues and this should be ready for review.
jonblatho → made their first commit to this issue’s fork.
@oscarclement: Sorry for the delayed response. We pushed production updates this week with Warmer 2.0.13, Drush 12.5, and Drupal 10.2.x without any apparent issues. Is the error output identical to that mentioned in the original issue?
Can we please get a fresh release with this patch merged? Drupal 10.2.x requires Drush 12 or later → , and this issue is the only thing blocking us from upgrading.