Account created on 26 December 2007, almost 18 years ago
  • Web Architect, Developer, Themer at Bluespark 
#

Merge Requests

More

Recent comments

🇪🇨Ecuador jwilson3

these static IPs come from \Drupal::service("settings")->get("reverse_proxies")

I haven't dug into the module code yet, but isn't the Drupal $settings[] key either "reverse_proxy" or "reverse_proxy_addresses"?

there is an opportunity in the code to skip a bunch of logic much earlier by checking if we're on an Acquia Cloud environment ...

... or, by adding a feature flag killswitch to explicitly disengage the module on specific environments via settings.local.php, so that both purge and acquia_purge can be enabled but effectively neutralized via $settings, to avoid complex config_split workflows.

🇪🇨Ecuador jwilson3

It would be exceedingly helpful to have a feature flag style killswitch to disable purger diagnostics that piggyback on most Drush commands.

🇪🇨Ecuador jwilson3

jwilson3 created an issue.

🇪🇨Ecuador jwilson3

jwilson3 created an issue.

🇪🇨Ecuador jwilson3

Anyone affected by this issue is recommended to please help test the fix on the other issue by upgrading (if only temporarily) to the 2.3.x-dev branch in composer, clear cache, and confirm the errors are no longer present. If I dont get feedback in 2 weeks, I'm going to cut a release with the fix that casts the net to the wider community using the module. If you're on a case-sensitive file system, I request your assistance to verify there are no more issues here.

See Issue #3516563

🇪🇨Ecuador jwilson3

I'm waiting to get feedback to 🐛 Drupal\Component\Plugin\Exception\PluginException: The plugin (svg) did not specify an instance class. Active , which is potentially a breaking change for some. See my last comment there please. If you're on Windows or case-insensitve filesystem, feedback much appreciated.

🇪🇨Ecuador jwilson3

This has been included in 2.3.x branch.

I would really appreciate if someone affected by this issue could test updating their composer to:

composer require drupal/svg_image_field:^2.3.x-dev

Then clear cache, and confirm that the errors no longer exist.

I'm really surprised no one has bothered to chime in here, especially since I've seen a noted down-tick in adoption of the version that released the renamed SVG.php that caused this issue.

If no one answer on this issue in the next 2 weeks, I'm going to cut a new release, which will affect everyone using the module to cast the net wider to receive feedback.

Thank you.

🇪🇨Ecuador jwilson3

@shaunlaws. I'm also using a hook_captcha_alter, however the idea with this issue is since this is DOM, we need to be using Twig. There was a huge push when going from D7 to D8 to get HTML out of PHP and into Twig.

🇪🇨Ecuador jwilson3

The only tooling I currently use (as a contrib maintainer) that *might* be affected by commit message formats is https://drupal-mrn.dev to generate release notes which also generates links and attribution to those who contributed to a release. Since that is maintained by Matt Glaman, I'm fairly certain he'll accept a fix to support whatever ends up being the solution. The harder thing will be to find those every-day tools that you don't think about as being actual tooling until, you know, it breaks. Things like IDE commit message integrations, cloud-based code hosting tools which turn things into links, git gui and git merge tools, etc. If a 3rd party tool doesn't already support conventional commit formats, it will be easier to request they add support for that than a custom carve-out for a potential non-conforming format.

🇪🇨Ecuador jwilson3

Re: #33 from @justcaldwell 🐛 ID's are stripped from links Needs work

Confirming that the CKEditor 5 Bookmark plugin does not support bookmarks/anchors on links. It my testing it also appears to strip id attributes from existing links.

Interesting that both modules are affected. The Bookmark feature was added to CKEditor 5 in release 44.0.0 https://ckeditor.com/blog/ckeditor-44-0-0-release-highlights/#bookmarks

What is not clear from this is whether the bug is something that needs fixing in CKEditor itself, or is a generalized Drupal <> CKE5 integration issue.

This underlying issue seems pretty critical (due to data loss!) to demand a warning on both this module's project page, as well as on the ckeditor5_bookmark module page, until the issue is resolved.

🇪🇨Ecuador jwilson3

I arrived here via release notes https://www.drupal.org/project/views_bulk_operations/releases/4.4.2

If the 4.3.x hook_update_Ns have been added back, is the warning in the issue summary above to update to the latest 4.3.x version still relevant? If not can it be removed? If its still needed could this warning be mentioned also all 4.4.x release notes pages?

🇪🇨Ecuador jwilson3

We should also leverage the Metatag ecosystem https://www.drupal.org/project/metatag/ecosystem

  • Expand the list in the IS to include everything classified in the metatag ecosystem.
  • Open issues in more modules (eg "Token Or", and others) to add the Metatag ecosystem to their respective project edit forms.

Re: novice task - Makes sense. Writing up the relevant description for each module (as I suggested/requested in #2) might be the harder part, but an appropriately prompted LLM could probably take a first stab.

🇪🇨Ecuador jwilson3

RTBC++ Would love to see this one-liner in both the 3.x and 2.x branches.

The base use case here is to have group members / content editors add *content* to groups in non-live Workspaces, which works great with MR 177. Therefore, comment #15 should be moved to a follow-up. Adding members to groups in a non-live Workspace seems like a corner case. I wonder if core user creation in non-live Workspaces even works as expected?

🇪🇨Ecuador jwilson3

+1. To be helpful, each module needs a super-brief description of what problem it solves in the context of Metatag module. In the case of the ones that extend token logic a brief example of the syntax would be really helpful too. Maybe it makes sense as a docs page instead?

🇪🇨Ecuador jwilson3

Setting back to needs work based on #28

🇪🇨Ecuador jwilson3

I was unable to save/submit the /admin/config/development/performance page on Drupal 11, even long after advagg module has been removed from our codebase.

In my case, without the advagg module present anymore, the fix was to manually remove the offending key (stale_file_threshold) from config/sync/system.performance.yml and import config.

It's not at all clear from reading the comments that this issue should be RTBC. It looks like Alberto put it in RTBC, but then later added several PR comments, at least one of which is unresolved.

🇪🇨Ecuador jwilson3

jwilson3 created an issue.

🇪🇨Ecuador jwilson3

This is a duplicate of 📌 Drupal 11.2.x compatiblity Active

🇪🇨Ecuador jwilson3

jwilson3 created an issue.

🇪🇨Ecuador jwilson3

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.

🇪🇨Ecuador jwilson3

jwilson3 created an issue.

🇪🇨Ecuador jwilson3

Before:

After:

🇪🇨Ecuador jwilson3

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.

🇪🇨Ecuador jwilson3

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.

🇪🇨Ecuador jwilson3

Thanks for getting this in! 🎉

🇪🇨Ecuador jwilson3

Fixed in release 2.2.7

🇪🇨Ecuador jwilson3

jwilson3 created an issue.

🇪🇨Ecuador jwilson3

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.

🇪🇨Ecuador jwilson3

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.
🇪🇨Ecuador jwilson3

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):

  1. Install Drupal with the Standard profile.
  2. Add a multi-cardinality file field to the Article content type that allows upload of each of the file mime types.
  3. Configure field display to display the generic file widget.
  4. 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.
  5. 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.

🇪🇨Ecuador jwilson3

Whoops, forgot to update status. This was merged to 2.0.x branch, but not released yet in an official version.

🇪🇨Ecuador jwilson3

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.

🇪🇨Ecuador jwilson3

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

🇪🇨Ecuador jwilson3

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.

🇪🇨Ecuador jwilson3

MR ready for review.

Before:

After:

🇪🇨Ecuador jwilson3

Long User Register form before MR:

Long User Register form after applying MR:

🇪🇨Ecuador jwilson3

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.

🇪🇨Ecuador jwilson3

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.

🇪🇨Ecuador jwilson3

jwilson3 changed the visibility of the branch 3161447-2.0.x to hidden.

🇪🇨Ecuador jwilson3

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.

🇪🇨Ecuador jwilson3

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.

🇪🇨Ecuador jwilson3

jwilson3 made their first commit to this issue’s fork.

🇪🇨Ecuador jwilson3

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
🇪🇨Ecuador jwilson3

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.

🇪🇨Ecuador jwilson3

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.

🇪🇨Ecuador jwilson3

MR Ready for review

🇪🇨Ecuador jwilson3

Screenshots "after"

🇪🇨Ecuador jwilson3

Screenshots "before"

🇪🇨Ecuador jwilson3

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.

🇪🇨Ecuador jwilson3

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.

🇪🇨Ecuador jwilson3

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).

🇪🇨Ecuador jwilson3

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;
🇪🇨Ecuador jwilson3

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.

🇪🇨Ecuador jwilson3

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.

Production build 0.71.5 2024