🇺🇸United States @douggreen

Winchester, VA
Account created on 5 August 2005, over 19 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States douggreen Winchester, VA

I've been looking into the suggestion by @catch in #71. Here's a patch (no MR because I'm not sure we want to go this direction).

🇺🇸United States douggreen Winchester, VA

I'm sorry I didn't see these requests when they were needed a year ago. The best way to get my attention is through the contact for or slack :( I've committed the change for D11. TBH I don't use this module anymore and I'm not even sure who I wrote it for.

🇺🇸United States douggreen Winchester, VA

sure, I've granted you full access. tbh, I'm not even sure what client I wrote this for.

🇺🇸United States douggreen Winchester, VA

douggreen changed the visibility of the branch 3497225-plain-text-formatter to active.

🇺🇸United States douggreen Winchester, VA

douggreen changed the visibility of the branch 3497225-plain-text-formatter to hidden.

🇺🇸United States douggreen Winchester, VA

This fixed the problem for us as well. Can we get this merged and a new tag applied?

🇺🇸United States douggreen Winchester, VA

Attached is #23 rerolled for core 10.4.x

🇺🇸United States douggreen Winchester, VA

I've also run into another use-case where it doesn't work, still using IEF complex.

🇺🇸United States douggreen Winchester, VA

The last commit fixes the validation warning.

🇺🇸United States douggreen Winchester, VA

When I use this to just upload media, I now get the error This value should not be null. after the upload succeeds, so there's something still wrong here.

🇺🇸United States douggreen Winchester, VA

I think this fixes it with IEF, but I haven't tested it outside IEF. Please test :)

🇺🇸United States douggreen Winchester, VA

Oh wait, this will work, I just need to configure it differently. As for tests ... I apologize in advance, but I'll leave that for someone else.

🇺🇸United States douggreen Winchester, VA

@anybody this solution didn't actually solve our problem, so I'm going to suggest another solution. Our user's were still banned, but after a few tries now. I think we need config that just doesn't ban based on honeypot time.

🇺🇸United States douggreen Winchester, VA

Closing since this is included in Drupal 11 compatibility for Video Embed Field Needs review

🇺🇸United States douggreen Winchester, VA

I updated the project page with details from the README.

🇺🇸United States douggreen Winchester, VA

Closing, this was fixed some time ago.

🇺🇸United States douggreen Winchester, VA

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

🇺🇸United States douggreen Winchester, VA

It does not. This is a very simple port of this one feature from the Drupal 7.x version of https://www.drupal.org/project/govdelivery .

🇺🇸United States douggreen Winchester, VA

This appears to already be fixed (not sure when it was ever a problem TBH). The MR shows no changes.

🇺🇸United States douggreen Winchester, VA

Closing, D8 is no longer supported

🇺🇸United States douggreen Winchester, VA

This was done in June 2023, not sure why this issue still exists, closing

🇺🇸United States douggreen Winchester, VA

Committed, I wasn't initially a fan of this, but as an option it's certainly valuable.

🇺🇸United States douggreen Winchester, VA

sure, committed.

🇺🇸United States douggreen Winchester, VA

This was committed directly. Thank you to the bot developers (having this issue and the bot tell me that nothing else needed to change, was empowering to make this simple change).

🇺🇸United States douggreen Winchester, VA

The MR adds configuration for honeypot. Do we also want to add a flag to just enable/disable honeypot banning?

🇺🇸United States douggreen Winchester, VA

I think it works with a Simple IEF form but not a Complex form.

🇺🇸United States douggreen Winchester, VA

There are also problems with IEF because the FormState built in the Event is missing critical state information.

🇺🇸United States douggreen Winchester, VA

We have also run into this. I think that the honeypot code needs to be more configurable. There should either be config to disable it, or config to control it's value separately from the default IP banning.

🇺🇸United States douggreen Winchester, VA

Closing, ... I ran into the same issue. We've decided to not pursue SRI for external JS provided by reliable 3rd parties such as yourself. While there is a security concern, it's impossible for us to keep up with 3rd party libraries which might change. This really is the responsibility of those 3rd party libraries, such as crazyegg, to provide SRI's.

🇺🇸United States douggreen Winchester, VA

I took a different approach than the existing work, and wrote a drush command that encrypts existing data. See Add drush command to encrypt existing data Needs work

🇺🇸United States douggreen Winchester, VA

The patch solves the error. I'm wondering if there's another solution to encrypt existing content that wasn't previously encrypted.

🇺🇸United States douggreen Winchester, VA

I've seen this warning in my logs but I'm having a hard time finding a test case, and I haven't seen it recently. I think that this patch is just hiding an underlying problem, which is that either that #max_delta is wrong, or that the element that #max_delta points to wasn't added. Can anyone reproduce this still? Is it possible that this was a problem in core that has since been fixed?

🇺🇸United States douggreen Winchester, VA

LGTM

🇺🇸United States douggreen Winchester, VA

I still get a PHP warning in AttributeArray (line 79) because the original input array is statically cached in ViewExecutable::getExposedInput() on line 726, which happens before the data is validated. We need to validate the data before it is saved.

🇺🇸United States douggreen Winchester, VA

Since it's not version controlled, we can't rely on the SRI, and should remove it. The file has changed at least once since we implemented this.

🇺🇸United States douggreen Winchester, VA

I rebase and force pushed the branch. It needs review by someone who is familiar with what this does.

🇺🇸United States douggreen Winchester, VA

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

🇺🇸United States douggreen Winchester, VA

Do this do anything that Add option to show only start or end date in the DateTime Range custom formatter Needs review doesn't do? If it does, I think it needs to be re-worked now that the other issue has committed.

🇺🇸United States douggreen Winchester, VA

The above commit should fix this.

🇺🇸United States douggreen Winchester, VA

douggreen created an issue.

🇺🇸United States douggreen Winchester, VA

douggreen created an issue.

🇺🇸United States douggreen Winchester, VA

$ curl https://cdn.localizejs.com/localize.js | openssl dgst -sha384 -binary | openssl base64
7w/vCB9txvKaPgvRpSBQWnsULnvvH1VyQJXvjZujqTrsAvQI6B0H42gKqQUCG3y6

🇺🇸United States douggreen Winchester, VA

douggreen created an issue.

🇺🇸United States douggreen Winchester, VA

I like the idea of removing the config option extlink_target_append_new_window_label which defines the label text and is new to this patch, always using the recommended text, and linking to the external resource (from above). This isn't removing an option in production. This is removing an option that someone in this thread thought helpful, and replacing it with a standardized (maybe industry standard) a11y text.

The current patch checks for "new window" in the existing label (not "opens in a new window") so I think that the existing patch is already backwards compatible with production systems.

If we agree, then what's left is to remove the config option extlink_target_append_new_window_label and make sure the tests work. If I understand correctly, we're pretty much at the place that you started with this ticket, it just took me a while to get there (sorry). What do you think?

🇺🇸United States douggreen Winchester, VA

This appears to have merged, so marking as fixed.

🇺🇸United States douggreen Winchester, VA

This was made an option because of comments starting in #11 above without any discussion until #51. Also, #51 suggested linking to WCAG somewhere, but that was never done. And IMO that is a better thing to have done than making the accessible feature optional. What should we link to?

🇺🇸United States douggreen Winchester, VA

While I dislike removing options that other people think are helpful, I agree with @jenlampton that the new option extlink_target_append_new_window should not be an option but just always be true.

I'm a little less clear on whether the other new option extlink_target_append_new_window_label should be configurable or some set text. The fact that extlink_target_label is configurable makes me think that extlink_target_append_new_window_label should also be an option.

Also a little unclear to me is if the new JS I wrote should even exist. If both labels are configurable, and if the defaults are as we currently have them, this JS will never be used. And since the site builder has full control over this, is this new JS a waste of time to run in the browser and maintain in code?

🇺🇸United States douggreen Winchester, VA

There is some overlap (and conflicts) with A11y: add screen reader text for external links Needs review so I copied this commit from here to there. This can still be merged, if it goes first. Or if that MR goes first, this can be closed.

🇺🇸United States douggreen Winchester, VA

I created MR https://git.drupalcode.org/project/extlink/-/merge_requests/18 so I'm removing the patch attachments from this, to avoid confusion.

🇺🇸United States douggreen Winchester, VA

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

🇺🇸United States douggreen Winchester, VA

I created a MR, so I'm hiding all of the patches attached to this ticket, to avoid confusion.

🇺🇸United States douggreen Winchester, VA

The attached patch :

  1. makes the "(new window)" label configurable
  2. compares the title and the "(new window)" label in a case-insensitive manner without the parenthesis. See Drupal.extlink.compareLabels().
  3. combines the two titles in a more readable manner, see Drupal.extlink.combineLabels().

It does not update the tests.

🇺🇸United States douggreen Winchester, VA

In response to #3 - You could keep logBlockedSubmission in the native condition, that's a design choice. I like using my own hooks as it shows other developers how to use them.

In response to #4 - we aren't altering anything, I think invokeAll is the correct method. And yes, we should inject the module_handler.

🇺🇸United States douggreen Winchester, VA

I've seen a similar error in my logs, but I'm on Drupal 10.2.x (not 7.x as this issue is against), so this is not the right fix. And I don't think the problem is passing an empty path to the delete, because my log shows the 1.xml in the path not being deleted.

🇺🇸United States douggreen Winchester, VA

This is easy to test. Create a URL that sends the lat and not lng or vice-versa. I normally agree that we shouldn't hide PHP warnings, except when they can be triggered by a user sending bad data. It's a simple fix, and I disagree that this is how it is intended to work. We should fail, but not fail with PHP warnings.

🇺🇸United States douggreen Winchester, VA

Thanks, I've updated the project page and marked the project as Unsupported.

🇺🇸United States douggreen Winchester, VA
🇺🇸United States douggreen Winchester, VA

sure, thanks for the help!

🇺🇸United States douggreen Winchester, VA

Odd, I don't see the "create Merge Request" button. I created a branch and pushed to it but the "create merge request" button is missing and the link that git gives me gives a 404.

https://git.drupalcode.org/issue/drupal-3412420/-/compare/11.x...3412420...

🇺🇸United States douggreen Winchester, VA

MR created.

In reply to the comment by @smustgrave that this was possibly done on purpose because of a layout builder scenario, that isn't mentioned at all in the original ticket. And this very problem here was mentioned there.

Production build 0.71.5 2024