My apologies for starting from the wrong branch. I recreated the same existing MR branch name locally, cherry-picked the existing commits, and force-pushed to the existing MR. Back to NR.
I've pulled over a few additional best practices from the current issue I was working on 📌 Update Drupal's default file type icons Active .
Add the new high-resolution SVGs files alongside existing low-resolution PNGs in all themes shipped with Drupal core. Do not remove old PNG files from the codebase. Add cleanup task #3452493: [12.x] Remove images that have been replaced in core.
I'm having second thoughts about this in the context of the StarterKit theme. Certainly for Umami and Claro themes, we shouldnt remove the PNGs. However the StarterKit theme, when used as intended (if I understand correctly), is supposed to be a kind of "line in the sand" or fork of the codebase and a snapshot in time. This implies to me that we could easily remove the old PNG files and then any new themes created from the Starterkit would have a clean slate and no duplicitous files. Tech debt free!
For anyone following along here, I've opened another issue related to PNG to SVG conversion in core file icons: 📌 Update Drupal's default file type icons Active
Re #58:
Default presentation attributes (fill/stroke/etc) should be moved off from SVG tags if it reduces the weight, and placed as Internal CSS
Please don't do this, especially not in core. I even propose to avoid CSS styles and encourage the use of presentation attributes, as they are very easy to overrule in CSS by contrib+custom modules and themes.
I tend to agree, but I feel like it is hard to make call one way or the other. It seems to me that this is highly dependent on how complex the SVG (or group of SVGs) is and how many attributes there are that would be shared/duplicated. Furthermore, there is a good argument that by using classes instead of inline attributes, the classnames become the api, giving core the freedom to change/tweak things like hex colors getting converted to rgb or CSS attributes eg path.something {fill: var(--my-color, #00ff00); }
without breaking the overrides in your custom theme.
To reframe my point in the context of your example on comment #58...
This:
#contrib-module a svg path.something {
fill: blue !important;
}
#contrib-module a:hover svg path.something {
fill: red !important;
}
Seems much less fickle than:
#contrib-module a svg path[fill="#00ff00"] {
fill: blue !important;
}
#contrib-module a:hover svg path[fill="#00ff00"] {
fill: blue !important;
}
In the second example, if the SVG from core or contrib ever changes the fill attribute value, your theme override breaks. I could be wrong here but it seems unexpected that the fill attribute value is the API, and OTOH, much more obvious that a classname is an API, and has been purposefully added in the SVG to make theme overrides more stable.
Hrm. The test fail seems unrelated.
Drupal\Tests\node\Functional\NodeRevisionsAll
The string "page=1" was not found anywhere in the HTML response of the current page.
Hrm. The test fail seems unrelated.
Drupal\Tests\node\Functional\NodeRevisionsAll
The string "page=1" was not found anywhere in the HTML response of the current page.
Added #3521857 to issue summary (assuming it lands) for file type icons in Umami, Claro, and Starterkit themes.
Since there's been no recent activity here, I suggest postponing this in favor of 📌 Update Drupal's default file type icons Active , which has a clearer scope, avoids bikeshedding, and aligns with the precedent set by other PNG-to-SVG conversions already committed to core.
jwilson3 → created an issue.
jwilson3 → made their first commit to this issue’s fork.
I've installed the Rivet theme on Drupal 11 now that the IU Paragraphs module is compatible with Drupal 11. And I wasn't able to reproduce the original issue mentioned in the issue summary here. I will proceed to merge the current MR, but am depending upon the community to test this and confirm it is fixed, or provide more detailed steps on how to reproduce the issue.
This has sat for a month. I'd requested feedback from various folks at IU and Drupal license auditors and no one has objected. Therefore, I've proceded to merge the MR. Happy to pick this up again if anyone feels I've missed something here.
Thanks Nick. That reads really well and fixes the issue I raised.
¯\_(ツ)_/¯
Here's a screenshot from yesterday in Chrome Incognito.
Thanks again for your continued support and responsiveness, @micropat. It is much appreciated.
I found that there is work going on elsewhere for this, including:
- Another contrib module, that has also implemented a modal approach and has more installs than this module: https://www.drupal.org/project/media_library_edit →
- Core design issue: 📌 Design a UI to allow various kinds of alterations to referenced Media entities in a modal Active
- Core implementation issue: ✨ Allow media items to be edited in a modal when using the field widget Postponed
Would it make sense to rewrite this such that users would use the same drush command regardless of whether they're using the custom code on this docs page, or the version that got integrated into drush 13?
+1! This is such a useful little module! Alas, our institution also requires that contrib modules opt into the security advisory option, in order to use them.
For further information:
https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... →
https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... →
✨ Support CKEditor5 Needs review has been closed and marked fixed. Since this now has a working solution in contrib, should this issue re-focus efforts around inclusion of the functionality from https://www.drupal.org/project/edit_media_modal → into core's Media Library CKE5 integration?
Here is an updated and versioned patch from the current state of the MR.
We should also remove the title attribute, which has known accessibility issues.
See 🌱 [META] Discourage non-inclusive use of the HTML title attribute Active .
image_style attribute no longer appears in the latest 8.x-1.x version.
setting back to NW per #22. This would also benefit from being moved to an MR.
Referencing comment #52 📌 Review/archive the local server setup section Active from @quietone:
The phrase "officially recommended" is not accurate. The choice was made by the community; there is no officiating body that chose DDEV.
However, if you view the Local Server Setup page → in an Incognito window, you'll see a banner at the top stating:
DDEV is the official local development tool of Drupal. And like Drupal, DDEV depends on the support of the open source community.
Meanwhile, the official_docs/local-development-guide → says:
The Drupal community recommends using DDEV.
And on the same local server setup page being discussed in this issue:
Other Docker-based Drupal tools exist for any OS but aren't officially recommended.
So while there's technically no central authority, Drupal.org messaging across multiple pages strongly implies that DDEV is both recommended and "official," even using that language directly in at least one case. Maybe there is a follow-up to dial this in?
Why is this issue marked postponed?
The workaround in comment #12 no longer works. What worked for me was just implementing the google-tag-gtm-iframe.html.twig
template file in my Drupal theme:
{#
/**
* @file
* Overrides the Google Tag Manager noscript fallback iframe.
*
* It is intentionally left blank because GTM tracking is unnecessary
* and against GDPR best practices when JavaScript is disabled.
*/
#}
This is not ideal, but we're in a situation where we can easily patch the module with the MR here due to being on a shared Drupal multisite instance.
I don't understand how not having proxy_headers: ''
in active config (or in $settings overrides) should result in a WSOD when generating images. Shouldn't this just be resolved in the module with a null coalescing operator?
Yep. I should have made this clearer in the issue summary. This issue should stay with the contrib module. We can be certain core would not add a dependency on the CAVIF Rust library, but does anyone know if core would ever plan to add ImageMagick support?
@ben.campbell, thanks for the feedback. Can you tell me a little more about your setup please.
Verify plugin file location Confirm the Svg.php file is in the correct location according to the namespace. If installed via composer with a conventional Drupal composer installation the file should be located in web/modules/custom/svg_image_field/src/Plugin/media/Source/Svg.php (the top-level folder "web" might instead be called "docroot" or something similar, or in some rare cases may not even be present).
Confirm case-sensitive filesystem: Are you running on an operating system that leverages a case sensitive filesystem like Windows or Mac?
Confirm title-case filename: Please verify that the filename is exactly Svg.php
and not SVG.php
, noting correct capitalization of the first letter only. This file is located at modules/contrib/svg_image_field/src/Plugin/media/Source/Svg.php
. The 'svg' MediaSource plugin not being found could be related to the file's case not getting changed properly in the version upgrade to 2.3.4.
Rebuild composer's autoload files. Run a composer dump-autoload
. Then another drush cache:rebuild
, and then check again to see if the problem goes away.
Check file permissions:. Ensure the Svg.php file has proper read permissions and can be read by the webserver with:
chmod 644 web/modules/custom/svg_image_field/src/Plugin/media/Source/Svg.php
Check for stale cache files: Sometimes old cached versions of the plugin definition can persist. Please check and clear:
sites/default/files/php/twig/*
sites/default/files/php/*
All good @joelpittet! And thanks for the issue credit to boot!
Thanks for taking a stab @sourabhsisodia_. However, I think it makes most sense to wait for @micropat (module maintainer and staff at AddToAny) to chime in here to see if this is something that can be dealt with in the upstream JS library itself, just like the other accessibility issue that I filed not long ago was handled 🐛 Accessibility: Unlabeled Form Control a2apage_find a2a_copy_link_text Active .
I fixed this at some point in a custom theme with the following custom js behavior, but it doesnt always work, and depends on whether the mmenu has already loaded or not.
$(".mm-tabstart").attr("aria-label", "menu tab start");
$(".mm-tabend").attr("aria-label", "menu tab end");
This workaround satisfies WAVE WebAIM, but doesn't really help end users by explaining what these buttons actually do, or why they're there. It seems like when mmenu doesn't add any inner text, these buttons are in the DOM just as placeholders.
Since the "Redundant title text" alert is handled by the patch on 🐛 Accessibility: title attribute the same as the text Active , we should focus this issue on solving 3 "Empty button" errors.
jwilson3 → created an issue.
Came here befuddled as to why "CDN" is not even an option on the dropdown, much less why its not the "fallback" option when nothing else is selected. Thanks for this addition.
I suggest also adding the value to the bundled subthemes' info.yml as well.
joelpittet → credited jwilson3 → .
The fix accurately removes the redundant title attribute from the link. Use of the title attribute as a technique to provide additional information is generally considered as harmful due to "extensive user agent limitations in supporting access to the title attribute". In this case the title is not even adding any additional context, since it redundantly indicates the same text as the button itself.
jwilson3 → created an issue.
This is a re-roll of the original patch that was uploaded to this issue against 8.x-2.x branch, with the following changes:
- Adopt the patch naming convention for contrib modules.
- Incorporate the same adjustments from the 3.x branch MR above, including removing unnecessary whitespace changes, removing the isset() logic, and fixing phpcs formatting.
Pending tasks
Add tests to the MR.
the expanded attribute [...] improves the accessiblity for screen readers.
This is true, but the ARIA5 rules say that to be able to use the aria-
attributes, you have to have an element with the correct role. By default, <span>
has no semantic meaning—it's effectively role="presentation"
or role="none"
in ARIA terms unless styled or scripted to behave otherwise.
Ideally, we'd switch to a <button>
, but that could break themes that (incorrectly) target the <span>
directly instead of using a class. Would changing the tag—and potentially breaking CSS—be considered an API change requiring a major version bump?
If so, to resolve the issue, keep screen readers and machines happy, and avoid a major bump, what if we simply add role="button" to the <span>
and ensure it behaves like a native button, including being focusable (tabindex="0"
) and keyboard-interactive (handling Enter/Space key events).
References:
WCAG 2.2: Technique ARIA5: Using WAI-ARIA state and property attributes to expose the state of a user interface component
My understanding is that the patch specifically does not change the tag. (Changing the tag to a <button>
was proposed as the "proper" alternate solution, but the approach in the patch as currently written does not change the tag, just removes the inappropriate aria-expanded.
jwilson3 → changed the visibility of the branch drupal-1934508-1934508-cache-clear-doesnt to hidden.
jwilson3 → made their first commit to this issue’s fork.
Can you manually add @marthinal to the issue for credit 🙏
He originally wrote the patch which we were using internally on a project.
woah. thanks @mattsqd for the responsive reply, review, and merge. 🙌
A related issue was reported in 🐛 Drupal\Component\Plugin\Exception\PluginException: The plugin (svg) did not specify an instance class. Active . I **think** it would also be resolved with a cache rebuild, but needs confirmation from those experiencing the issue to confirm.
I'm not certain how far OG model goes.
- Guide node types are the ones with the gray boxes and short description text.
- Documentation node types are the ones without gray boxes and long body copy.
@ressa: LGTM, thanks for the change.
@quietone: I dont know if this will help, but I'll try. The Organic Groups pages are typically top- or high-level docs pages that serve to aggregate lists of sub-sections/pages (the gray boxes) that are automatically pulled into the page and have to be edited elsewhere (i.e. its harder to change the gray box sections). The OG Pages are also characterised by having minimal unique body content above the gray boxes (due to a restricted body field length). The Local server setup page is an OG Page. Whereas non OG-Pages in the docs section are characterised by having a lengthly and explanatory body copy on a specific topic, where a note-tip looks good.
I can confirm the MR #36 is a faithful reroll of the patch in #42 (without the extraneous code added in #45 and #46).
Thanks for the work @danielkorte
Before:
After:
jwilson3 → created an issue.
I can confirm that the upstream issue https://github.com/pantheon-systems/search_api_pantheon/pull/197 was merged and included in release 8.2.1.
The patch mentioned in #20 no longer applies to anything past version 8.2.1
This issue really should be marked fixed.
I'm creating a new 2.1.x branch to merge this, so that the 2.0 branch retains integration with older versions of Drupal.
The code changes look good. I fixed some phpcs issues introduced in the patch and tested in the UI.
Before:
After:
fix sentence case (requested in https://www.drupal.org/project/documentation/issues/3499635#comment-1605... 📌 Review/archive the local server setup section Active https://www.drupal.org/project/documentation/issues/3499635#comment-16051954 📌 Review/archive the local server setup section Active )
Using the note tip on non-OG pages, where it’s the only gray box and actually stands out, still makes sense. And I get the sentiment and need for consistency, but several points in comment #48 elaborate on why the change is necessary here. I changed it to make the page more uniform and help DDEV stand out a bit but look closer to the others in the vein of "this is like the others and is the preferred option". The mismatch in H2 sizing and subtle whitespace between the blocks annoys me—especially since we don’t have a utility class to match it with the OG sub-page section headers—but whatever.
Since the "Recommended: DDEV (Docker-based)" title was changed and shortened to "DDEV", the uniformity with the other sections I was going for has been lost, so instead of removing the heading for the DDEV section as requested in #52, I suggest iterating on the text and linking it to the main DDEV docs page, for consistency with the other sections on the page.
jwilson3 → made their first commit to this issue’s fork.
Improve the summary text for display on the parent page "Local server setup". (related to discussion on #3499635 📌 Review/archive the local server setup section Active ).
Thanks @ressa for the explanation.
- Taking into account the OG Page limitations, I've done what I could to make things a little more clear per some of the ideas in #18.
https://www.drupal.org/node/2804427/revisions/view/13934826/13942883 → - I then, updated summary of the "Docker-based development environments" sub-page for more coherent display on "Local server setup".
https://www.drupal.org/node/2941639/revisions/view/13925027/13942888 →
Update summary for more coherent display on "Local server setup" parent page, per discussion on #3499635-18 📌 Review/archive the local server setup section Active
Improve UX of the page to best of our ability, given OG Page limitations, per discussion in #3499635-48 📌 Review/archive the local server setup section Active .
Hi @finex,
1. Can you please try the workaround in #6, which has an approach that should resolve the issue for you. TL,DR; create a View Mode called "Thumbnail" for Media, and render the actual SVG Field instead of the bundled Thumbnail field. Then edit the Media Library view to not use "Media Thumbnail" field, but to display the Rendered Entity, and render the "Thumbnail" view mode.
2. If this doesnt fix the problem, can you please provide some additional context, including:
Which theme is being used?
Screenshot of where the SVG is appearing too large.
@ben.campbell, does rebuilding all caches after updating, as indicated in the 2.3.4 release notes → resolve the problem?
jwilson3 → created an issue.
IMO the way https://www.drupal.org/docs/develop/local-server-setup → is currently laid out is confusing due to the different visual components used and content organization.
A few things of note:
- The div.note-tip for DDEV at the top of the page looks like something you'd skip when scanning the page to get to the juicy details further down the page.
- The only section with sub-section links is Windows, and one of those is a DDEV-related link. Shouldn't the DDEV thing just be moved to the DDEV section instead, since we're driving people to DDEV lander first, and then you pick your platform?
- It's not clear whether the other sections on the page are in fact Docker or DDEV related or not, you have to click to find out.
- It's not clear what if any of the tools like XAMPP or USBWebserver are legacy or not.
I get that this is kind of a topic lander page for what could be considered a completely legacy section, since now we have DDEV, but I think it makes more sense to present information in a cohesive way on the page (even if it doesn't belong to this section).
So, I took the text extract of the page to GPT and asked what it suggests to make usability and understandability improvements, and it gave me the following cleanup recommendations:
- Deprecate or archive legacy tools like XAMPP and USBWebserver if unmaintained.
- Clearly tag older alternatives as legacy or unsupported.
- Use components like tabs, accordions, or sections in the UI to separate "Docker-based", "Non-Docker", and "Windows-specific" options.
It then wrote the following up as one example of page organization. To be clear, I'm not saying we should go with this, but certainly making it more clear that DDEV is an option on par with the other options on the page seems sensible to be able to compare apples-with-apples, instead of making the DDEV note-tip into an orange.
Local Server Setup for Drupal Development
A local server environment is recommended for Drupal development. It lets you work offline, safely experiment, and tailor your dev stack as needed.
🔧 Recommended: DDEV (Docker-based)
DDEV is the officially recommended local development tool for Drupal. It supports macOS, Linux, and Windows, and provides:
- Easy project setup
- Consistent environments
- Built-in HTTPS and mail support
- Drush and Xdebug integration
🐳 Other Docker-Based Options
Other Docker-based tools exist but are not officially recommended:
🪟 Windows-Specific Options
For users on Windows who don’t use Docker:
- Installing Drupal on Windows for local usage
- Using DDEV in WSL2 on Windows (consider moving this to the DDev Section)
- XAMPP setup (legacy)
- USBWebserver (legacy)
🛠 Non-Docker Alternatives
If you prefer not to use Docker, see the general guide:
➡️ Installing Drupal without Docker
✉️ Email Handling Locally
Learn how to catch and read emails sent by your local Drupal site:
➡️ Managing mail on a local server
🐞 Debugging & Dev Tips
🔗 Related Guides
When you install the theme, the main menu block configuration provided out-of-the-box displays fine without any whitespace errors.
I'm unable to reproduce this issue and am going to need more precise instructions on how to reproduce this, and ideally a config export of the menu block you're using.
This theme is intended for university websites that typically have 2 or 3 levels deep menu structures.
- The out-of-the-box configuration for this theme is to hide breadcrumb trail when "Home" is the only option.
Page: /admin/appearance/settings/rivet
- But, if you switch to one of the other options, then the breadcrumb trail shows up, as expected.
- Furthermore, if you setup proper breadcrumbs on a node, then you get breadcrumb trail with either of the two breadcrumb settings:
I'm going to close this because, afaict, this is "Works as Designed".
If I've overlooked something, then feel free to reopen and PROVIDE PRECISE STEPS on how to reproduce the issue.
Thank you.