The PHPStan failures are unrelated to this issue and will be addressed in a separate follow-up issue that I will create. Will proceed to merge the MR.
Patch in #2 works for me using Select2 version 2.0.0 and Gin 4.1.0
I'd argue that the Claro theme changes would demand a follow-up issue, since the issue title here specifically calls out a fix for Gin.
Issue : In Mobile view Im not able to scroll
Thank you for the review @divya.sejekan!
I decided to revert the initial approach and try something different: position: fixed. for the image on desktop.
This approach does not impact scroll behavior on mobile, plus has the added benefit that the scrollbar appears at the far left edge of the screen, just as before apply the patch does. I wasn't much of a fan of having the scrollbar in the middle of the screen on desktop.
This seems to work much better now.
Feedback and reviews welcome.
Thanks.
I'm with you @ressa. Keeping regex, but making it more approachable makes sense.
One way to make the syntax less daunting, is using # as delimiters (no \/'s then) and an example can be seen in the README MR.
Another option could be to remove the delimiter completely, and handle proper escaping, based on whatever delimiter we decide to use, leaving that part internal to the module.
For example:
/.*\.asp/
/.*\.jsp/
/\/blog_edit\.php/
/\/blogs\.php/
/\/wp-admin.*/
/\/wp-login.*/
/\/my_blogs/
/\/system\/.*\.php/
/.*systopice.*/
/.*login.json/
/\/episerver.*/
Becomes:
.*\.aspx?
.*\.jsp
/blog_edit.php
/blogs.php
/wp-admin.*
/wp-login.*
/my_blogs
/system/.*\.php
.*systopice.*
.*login.json
/episerver.*
Much easier to read.
The goal here is to centralize Honeypot administration under a single UI where you'd expect to see it: on the Honeypot admin page.
In my initial investigation, I did not find the global /admin/structure/webform/config (that page is incredibly long!).
Setting aside the time restriction bug you're seeing – which sounds like it should be a separate follow-up – there might be two possible approaches to improve this MR
1. Expose the Webform global settings toggle in two places in the UI:
- At the top of /admin/config/content/honeypot, where you'd expect to see it.
- At the bottom of /admin/structure/webform/config under Third Party Settings.
This way we can remove the need for an additional configuration. I understand that such an implementation might be too tightly-coupled, and would complicate the form submit handler, so perhaps the better solution is ...
2. Add help text to the Honeypot config page
To enable Honeypot protection for all Webforms, go to:<br>
<strong>Webforms → Configuration → Third Party Settings</strong>
(<a href="/admin/structure/webform/config#edit-third-party-settings-honeypot">open settings</a>).
If you prefer, you can also configure Honeypot individually for each Webform under its own Third Party Settings.
There are before/after screenshots in the IS, from the raster PNG to the SVG. The approach taken on this issue is exactly the same approach taken on the loading and throbber icon issues, whereby we're not really introducing redesign, we're simply updating the format of the existing files rom raster to vector, so as not to have to spend time bikeshedding redesign, and getting this into core in the next immediate major release.
Splitting these apart into separate issues feels like it would open the door for more cat herding.
I propose keeping this on a single issue, and using the following steps to take before/after screenshots (untested):
- Install Drupal with the Standard profile.
- Add a multi-cardinality file field to the Article content type that allows upload of each of the file mime types.
- Configure field display to display the generic file widget.
- Create a node and upload a bunch of files with different file extensions. The files could be zero byte and just be the correct file extension.
- Take before/after screenshots of the rendered node.
There would be some amount of iteration, testing, and exploration to ensure we establish a definitive list of file extensions that map to each mime type.
Whoops, forgot to update status. This was merged to 2.0.x branch, but not released yet in an official version.
Thanks for clarifications. So this is not the same issue as 🐛 Stylesheet missing on /user/register when 403 Needs review , but its in the same vein and the fix there would likely be influenced in some way by a fix for this issue.
I just tested on Gin Login 2.1.x branch with a clean standard Drupal 11 install (via ddev-drupal-contrib), but was unable to reproduce the issue. I also sanity checked same behavior on MR 48 for aforementioned issue #3547364, and saw no adverse impact.
Maybe this is something specific to do with Drupal CMS?
I'm attaching some screenshots as evidence that it works as designed with a clean standard Drupal 11 install:
✅ 403 page left empty
✅ 403 page set to /user/login
Is this the same (or similar) issue as 🐛 Stylesheet missing on /user/register when 403 Needs review ? The steps to reproduce sound quite different, but maybe the MR I wrote there would work to solve this issue too? Please check! Thanks.
Long User Register form before MR:
Long User Register form after applying MR:
Instead of uploading these images for every site, it'd be simpler if we could simply point them all to the same path, e.g., /themes/custom/login-logo.png and /themes/custom/login-wallpaper.jpg
My understanding is that the "Logo > Path to custom logo" and "Wallpaper > Path to custom image" fields satisfy this behavior already. What am I missing?
Currently, trying to set the path to anything outside the site's own /sites/foo/files directory results in an error.
I see you used /themes/custom/login-wallpaper.jpg. Can you try removing the leading slash and specifying a proper theme path? I tried values like: themes/custom/mytheme/logo.svg and theme/custom/mytheme/images/wallpaper.jpg respectively, and both worked just fine (note: no leading slash).
Sidenote, you may also be interested in the work on ✨ Random local images Needs work to specify a folder of random images.
It seems like this may introduce discrepancy between the IMG tag used for the single default image versus the random wallpaper setting (handled by client-side js/wallpaper.js) which contains nothing more than the src and (empty) alt attributes.
jwilson3 → changed the visibility of the branch 3161447-2.0.x to hidden.
I've restored the use_custom_wallpaper_folder field mentioned in comment #49 above, in order to fix comment #47 on the MR 37.
Comparing the diffs between both MR branches, they're now effectively the same.
You can confirm this by running the following commands:
wget https://git.drupalcode.org/project/gin_login/-/merge_requests/37.diff
wget https://git.drupalcode.org/project/gin_login/-/merge_requests/38.diff
diff 37.diff 38.diff
I'm pretty sure 2.1.x MR 37 will apply cleanly with fuzz against the 2.0.x branch, if needed.
IMO, 2.1.x branch MR 37 is ready to go.
Regex changes look good. I've tested MR for the 2.1.x branch with a folder of JPGs and AVIF files. I tested invalid folder names. All works nicely. I made some minor grammar fixes to the form edit screen. Given the extra busy work involved in testing 2 branches, and cherry-picking commits between them, I don't see huge value in continuing the 2.0.x branch MR.
IMO, 2.1.x branch MR is ready to go.
Screenshots taken against the 2.1.x MR 37 with the dark variant of Gin admin theme.
In addition to #39, I found the terminology of the form elements on the user edit form a bit confusing.
Currently, the UI looks like this:
› Admin theme settings
[ ] Enable overrides
Enables default admin theme overrides.
I’d propose the following improvements:
- Expose the field to Manage Form Display so it can be drag-n-dropped/enabled/disabled in different form modes. (comment #39)
- Update the Details element label from “Admin theme settings” to “Administrative theme preferences”.
- Revise the toggle field label from “Enable overrides” to “Personalize the appearance of the administrative UI”, and remove the redundant description text.
- Implement a proper Field API widget with display options similar to the Address field widget, so users can select a Wrapper type (Container, Details, Fieldset) and override the default label text for Details/Fieldset.
The benefit of this approach is flexibility: if a frontend theme also provides customization options, both admin UI and frontend UI preferences could be grouped together under one field group. For example, by setting the wrapper to Container (invisible), you could present two checkboxes side by side:
› Website Look & Feel
[ ] Personalize the appearance of the backend administrative UI
[ ] Personalize the appearance of the frontend website UI
I'm here to report a related issue I'm having whereby the "Admin theme settings" wrapper element gets added to every user edit form mode. I don't know if this makes sense to add here or not. This does have to do with "fine-grained control" over display admin theme user settings.
Steps to reproduce:
1.- Standard D10/11 site with Gin admin theme and "Enables default admin theme overrides" enabled.
2.- Go to /admin/config/people/accounts/form-display, expand "Custom display settings" and click "Manage Form Modes".
3.- Create a new User Form Mode call it anything you want, eg "Password reset" (machine name user.password_reset)
4.- Return to /admin/config/people/accounts/form-display and configure the new form mode to only display a single field: "User name and password"
5.- Visit /user/1/edit?display=password_reset
6.- See that the "Account" fieldset is visible, and the "Admin theme settings" details element is also shown below.
Proposed resolution:
Make "Admin theme settings" available for placement via drag and drop on Manage Form Display settings on /admin/config/people/accounts/form-display
If this can be tackled independently of the awesome overhaul going on in this issue, I'm happy to split this off to a separate issue, but I don't know enough about the plans here or whether it makes sense to roll this into this feature request or not. Thanks.
Great reply. Thanks for taking time to clarifying your thinking and approach around generating and caching CSS files. That angle hadn’t occurred to me.
The only point I’d push back on is the dropdown implementation.
As it stands, the <button>-based environment switcher requires JavaScript to toggle aria-expanded between true and false. Without JavaScript, there are really only two HTML-native options: <select> or <details>.
<select> isn’t a good fit because many browsers don’t allow custom styling of option colors. Also it requires a <form> plus server-side handling and a redirect, when we only need simple links.
<details> works better because it allows us to maintain environment-specific colors in the links and avoids unnecessary form complexity.
That said, adopting <details> would be a fairly fundamental change—possibly significant enough to warrant a new major version bump.
It sounds like we have an architecture more or less hashed out here and could move forward with a POC.
Remove all use of the [...]
<script />[...] elements.
If we remove use of the <script/> tag, how would the switcher dropdown functionality work? Maybe we can switch it to a highly stylized <details> element, instead of a <button which would require no additional scripts.
Using style attribute for example shouldn't be necessary, when CSS rules don't need to be highly dynamic. Ie, background colors and text color etc is known, can be compiled to CSS files, and included formally.
Environment Indicator's foreground and background colors are highly dynamic, though. Historically, Environment Indicator requires project and environment-specific rules be be written in settings.php. (See code snippet on https://architecture.lullabot.com/adr/20210609-environment-indicator/ for one common pattern.). If we're not allowed to use inline styles via <style> tag or style="" attributes, how are we to supply the dynamic color changes per environment this module is used for?
The only thing that occurs to me is to create a Drupal route that dynamically generates a CSS file. But then that puts this module square in the realm of HTTP response caching, CSS/JS aggregation. and varying cache based on different project-based rules.
It would be cool to feed three birds with one scone, and as part of overhauling color contrast issues, introduce css custom properties (aka variables, and come up with a color scale along the lines of either general named colors var(--webform-fg-gray-light) and combining them with more targeted, purpose-built attributes var(--webform-wizard-step-fg-color) and var(--webform-wizard-step-bg-color) (the third bird is that we'd be setting webforms up for native dark mode support).
It is probably also useful to see when any Webform Nodes exist but do not reference any webform. A report going in both directions is akin to a more proper Correspondence or Bijection graph, where we're trying to ensure each entity has a matching partner, a natural 1-to-1 mapping.
The basic information can be obtained, roughly, with a simple query like this:
SELECT `webform`.`webform_id`, `node__webform`.`entity_id` as node_id FROM `webform`
LEFT JOIN `node__webform` ON `node__webform`.`webform_target_id` = `webform`.`webform_id`
ORDER BY node_id;
But this only accounts for webforms that either have or don't have Webform Nodes, it doesn't account for Webform Nodes that don't link to webforms, or Webforms referenced by multiple Webform Nodes.
For a more complete picture, we need a more complex query:
-- Show pairs + orphans + cardinality checks in one result.
SELECT
x.webform_id,
x.node_id,
CASE
WHEN x.webform_id IS NULL THEN '1_NODE_WITHOUT_WEBFORM'
WHEN x.node_id IS NULL THEN '2_UNREFERENCED_WEBFORM'
WHEN wn.cnt_nodes_for_form = 1 AND nw.cnt_forms_for_node = 1
THEN '5_PAIRED_1_TO_1'
WHEN wn.cnt_nodes_for_form > 1 THEN '3_FORM_REFERENCED_BY_MULTIPLE_NODES'
WHEN nw.cnt_forms_for_node > 1 THEN '4_NODE_REFERENCES_MULTIPLE_FORMS'
ELSE 'CHECK'
END AS status,
wn.cnt_nodes_for_form,
nw.cnt_forms_for_node
FROM (
-- A) all webforms, with their referencing node if any
SELECT w.webform_id, n.entity_id AS node_id
FROM webform AS w
LEFT JOIN node__webform AS n
ON n.webform_target_id = w.webform_id
AND n.deleted = 0
/* optionally constrain to the node bundle that should carry the field:
AND n.bundle = 'webform_node' */
UNION
-- C) nodes whose field points to a webform id that doesn't exist
SELECT w.webform_id, n.entity_id AS node_id
FROM node__webform AS n
LEFT JOIN webform AS w
ON w.webform_id = n.webform_target_id
WHERE w.webform_id IS NULL
AND n.deleted = 0
/* optionally: AND n.bundle = 'webform_node' */
) AS x
LEFT JOIN (
-- count how many nodes reference each webform
SELECT webform_target_id, COUNT(*) AS cnt_nodes_for_form
FROM node__webform
WHERE deleted = 0
GROUP BY webform_target_id
) AS wn
ON wn.webform_target_id = x.webform_id
LEFT JOIN (
-- count how many webforms each node references (should be 0 or 1)
SELECT entity_id, COUNT(*) AS cnt_forms_for_node
FROM node__webform
WHERE deleted = 0
GROUP BY entity_id
) AS nw
ON nw.entity_id = x.node_id
ORDER BY status, x.node_id, x.webform_id;
jwilson3 → created an issue.
Since we're removing email due to limitations above, it makes sense to also not use 'Co-authored-by' — in addition to the other arguments made above — because other platforms would typically expects a username + email in that format.
In #23 my proposal was that if consensus is that we're not trying to satisfy machines and us a standard format, then I'd propose taking it one single step further than just the technical email limitations written in the issue summary, and make the 'By: ' trailer readable as a single comma-separated list of usernames, which aligns more closely with how we used to do it.
Before:
Issue #999999 by user1, user2, user3, user4, user5, user6, user7, user8, user9: Convert MediaSource plugin discovery to attributes
After:
[#999999] task: Convert MediaSource plugin discovery to attributes
By: user1, user2, user3, user4, user5, user6, user7, user8, user9
Instead of:
[#999999] task: Convert MediaSource plugin discovery to attributes
By: user1
By: user2
By: user3
By: user4
By: user5
By: user6
By: user7
By: user8
By: user9
I didn't see anyone address this point yet and it was the primary point I was trying to make. Sorry for what sounds like it devolved into a bikeshed, but this goes directly to the point of considering how the data is used so we can get the format right once and not change it again for 10+ years.
I'm surprised no one has mentioned wildcard support here.
webform_submission_*
Were this MR to have wildcard support, then we could likely close other 3rd party module integration issues like ✨ Honeypot Facets 3 submodule? Active and ✨ Add "Protect all Webforms" options to Honeypot's global module config Active . Additionally, 🐛 Conflicting ID when honeypot is rendered more than once on a page Active could potentially even be solved via wildcard support as well.
The honeypot module already has a dev dependency on Webforms and tests that enables webform, so we could use that module in a wildcard test on the MR here.
As @longwave pointed out in previous comments, GitLab, GitHub and others seem to have come to some level of semi-standardization around the Co-authored-by: username <email> trailer, though this is not official in any way. So if the goal is for getting off the island, I'd go with that. On the other hand, if the goal is to just make the old "byline" info in the git commit message available (and easily human-scannable) for casual users, then a custom By: [username1]\nBy: [username2] is okay and By: [comma-separated list of users] even better, since IDE tools that do inline Git blame popups, like GitLense or Better Git Line Blame (which I use in Cursor) would get vertically truncated for really long multi-line commit messages, making it less scannable.
I think it comes down to what problem we're trying to solve. Since we use GitLab, it seems smart to make the effort and kill 2 birds with 1 stone by going with something that GitLab supports. https://github.blog/news-insights/product-news/commit-together-with-co-a...
From https://gitlab.com/gitlab-org/gitlab-foss/-/issues/31640
Gitlab should recognize
Co-Authored-By: [name] <[email]>in the message body, and should display user icons on UI related to the commit
they don't actually link to the user from the commit message, as is suggested here, but they list the co-authors along with the main author in the interface
Follow! My understanding is that AI bots are not actually submitting forms, but that the bots love clicking through links and the Facets issue boils down to the fact that Facet blocks aren't really checkboxes, but are in fact just links styled to look like checkboxes. Facets 3 switches to using actual checkboxes and forms instead of lists of links, which is promising and will probably save us for now.
But as time wears on and bots get more advanced, there might be real opportunity here. But my suggestion would be to first try adding additional form ids for the Facets 3 block forms to the existing Honeypot module settings on your site, to guage whether Honeypot + Facets forms it is having any effect in rebuffing bot requests. ✨ Allow for more flexible form configuration Active seems like a promising solution, if you're not opposed to manually entering facet form ids. If this does help, then maybe adding a UI option to manually enter custom form ids would be a good first step. A more custom UI just for Facet blocks could be possible, but probably complex, and easily doable from a few lines of custom form_alter code.
Here is what I'm doing to add Honeypot to all Webforms. Presumably it could be adapted for Facet blocks as well assuming they follow some kind of standard form id prefix in their naming conventions.
function mymodule_form_alter(&$form, FormStateInterface $form_state, $form_id) {
// Add honeypot protection to all Webforms.
// @todo remove when https://drupal.org/i/3544510 lands
if (strpos($form_id, 'webform_submission') === 0) {
if (\Drupal::moduleHandler()->moduleExists('honeypot')) {
// There is no reason to not enable time restriction here since
// Honeypot respects the time_limit set in the configuration.
\Drupal::service('honeypot')->addFormProtection($form, $form_state, ['honeypot', 'time_restriction']);
}
}
The form-specific "Protect [FORMNAME] webform with Honeypot" feature is provided by the Webform module, so we'll need a follow-up created in the Webform module issue queue to respect the value of the honeypot module's global setting, and disable the per-form option accordingly.
jwilson3 → created an issue.
Okay, that all makes sense, but now I'm trying to understand whether the (fallible, potentially inaccurate) author credits in the commit message is intended to serve machines or humans?
I still prefer the simplicity and less wordy
By.
If an issue has 20+ authors, then a newline and By: ends up getting really long itself. If we're back to only showing username instead of username <email>, then maybe we can also go back to a single line format like: By: author1, author2, author3, author4.
I've come here via a link to this issue, currently buried inside a collapsed FAQ item on the new contrib record page (https://new.drupal.org/contribution-record/:id ), which is currently being encouraged to module maintainers.
If you are committing, you are encouraged to use the formatting here.
Is it really safe to recommend people start using a new commit message format if the part about multiple authors is not yet settled? Will issue credit be properly picked up, if the format ends up changing?
Thank you for creating this issue to improve the Shortcut per Role module.
I am working through modules I help maintain to decide if this task is still relevant to the currently supported version of the module. There hasn't been any follow-up here for over 9 years which suggests that this is no longer relevant. Your thoughts on this will allow a decision to be made.
Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information as to whether this issue still applies, it may be closed after three months.
Thanks!
Thank you for creating this issue to improve the Shortcut per Role module.
I am working through modules I help maintain to decide if this task is still relevant to the currently supported version of the module. There hasn't been any follow-up here for over 9 years which suggests that this is no longer relevant. Your thoughts on this will allow a decision to be made.
Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information as to whether this issue still applies, it may be closed after three months.
Thanks!
Thank you for creating this issue to improve the Shortcut per Role module.
I am working through modules I help maintain to decide if this task is still relevant to the currently supported version of the module. There hasn't been any discussion here for over 9 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.
Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!
- @drupal_infra on Twitter/X - last updated 3 months ago, lately only shows planned outages, has intuitive threading (first report shown first, follow-ups threaded underneath).
- @drupalinfra on Mastodon - last updated 3 days ago, shows both planned and unplanned outages, has an unintuitive threading (follow-ups show up first, hard to find the original incident report).
- #drupal-infrastructure on Slack - last updated 12 hours ago, shows early incident reports, direct support from infra maintainers, is intuitively threaded (first report shown first, follow-ups threaded within).
Having a third-party hosted incident report / status page would be super nice, and there are plenty to choose from:
- GitLab.com incident management mentioned 2 years ago in #10.
- GitHub Discussions was mentioned in #16.
- Atlassian Status Page is the industry standard, but is also ridiculously inaccessible (one might even say hostile) to open source due to their pricing model based on number of subscribers.
- incident.io is a newish tool which has a "free forever" tier.
But there are several issues:
- There are too many platforms to choose from, leading to bikesheding (see: this issue).
- Ultimately, the implementation requires assessment and buy in from maintainers, which takes even more time.
- Implementing an entirely new channel just to replace the embed on the 500 error page seems like the wrong angle to come at the problem.
- A new dedicated channel will upset existing and expected outage workflows for both community users and maintainers.
- Setting the embed aside, it seems that most bases are covered with Mastodon and Slack, so introducing yet another channel adds noise — not to mention more busywork for Infra team to post updates across channels — unless steps are also taken to pull back from all the other platforms and centralize comms.
Reframing this problem from an end user perspective, when I see a 500 Error page the first thing I want to do is to be able to see if the incident is known, and if not know where to go to report it. I typically go to the https://x.com/drupal_infra first but only because that's where I'm directed to go. If I don't see anything reported there — it usually hasn't been — then I head over to #drupal-infrastructure channel on Drupal Slack to check for a report or discussion — it usually has been. From there, I can then finally see the link in the channel description to https://mastodon.social/@drupalinfra which might have different outage information than what is reported in Slack.
As an interim solution, could we just point to Mastodon and (optionally, the Drupal Slack channel) instead of Twitter/X on the 500 error page? Do we really even need to replace the embed on the 500 page?
Thank you Jacob! 🙏
jwilson3 → created an issue.