πŸ‡ΊπŸ‡ΈUnited States @douggreen

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

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡Έ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

$ 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

Committed, thanks!

πŸ‡ΊπŸ‡Έ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

merged, thanks!

πŸ‡ΊπŸ‡Έ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.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

Thanks! Committed :)

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

I rebased and force pushed.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

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

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

We also are seeing this, a total of 8 sites, that we upgraded over a period of 2 weeks (didn't want to upgrade all at once).

On 10/11 we upgraded the first 5 sites
On 10/18 we upgrade the remaining 3 sites
On 10/31 I recorded 1867 errors in the prior week
On 11/7 I recorded 845 for the prior week

Has anyone considered using fallback defaults? For example, if 'theme' is not set as a query argument, use the default theme (taking into account if it's an admin path). If 'delta' is not set, use 0. etc...

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

This must have been a D9 problem, because now that we're on D10, this patch has broken things. I'm closing this issue.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

Can you cut a new release so that we can stop using the dev branch please :)

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

lol, no worries, I made a similar mistake earlier today with another module! It happens.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

I'm attaching a new patch that reverts the previous commit and still makes my change from above.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

You committed something different than my patch, now we have duplicate dependencies in the definition.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

I'm closing this issue, as it was more of a support request that has been answered.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

I fixed the typo in the table name, thanks!

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

Uploading the same patch, but without the do-not-test name, since this is now a 7.x ticket.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

Updated patch applies cleanly to 7.98

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

Attached is an equivalent fix for D7.

I do wonder if the preExecute() should be done on the original query before clone is called, so that the hook_query_tag_alter()'s get called once instead of twice, when the same query is used for both a count and an execute... But the D9 version does it here in the counting, so I've kept it the same here.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

See attached

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

douggreen β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

yes we are using this in production (for several years now)

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

Backport for Drupal 7

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

Oops, that last comment was meant to update the issue summary.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

Problem/Motivation

When multiple files in a directory use the protected pages password, the user must re-enter the password for every page they want to access. It would be a better user experience if they only had to enter the password once.

Proposed resolution

Add a new option "validate per section" which would remember if the user has entered a valid password already and not require it for all pages in the section.

User interface changes

A new admin option is added. See https://www.drupal.org/files/issues/2023-04-19/validate_per_section_chec... β†’

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

Updated patch is relative to the module and not docroot, so that it applies cleanly.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

Stripped ^M off of patch so that it applies.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

I didn't use all these changes, but I did use some of these fixes, so credit was given, thanks!

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

committed, thanks

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

Committed, thanks

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

Oops, I missed a jQuery.once(), see attached.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

Oops, this fixes a syntax error.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

You can ignore my comment about the private function. I was confused. I thought from the description that someone wanted to use this function with other modules, and make it more flexible at the same time. If the purpose of this change is to just make _allowed_formats_field_types more flexible with configuration, then this is great!

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

I've been running this on a production site since making the patch. A link to IpUtils makes sense. Can you add that? And for tests, I'd prefer that someone else writes them ... if you require them and don't want to write them yourself, I'll ask someone else on my team to do it, but I'd also prefer that you do that too ... since this patch does what I need.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

Attached patch fixes issue raised by the bot. It also cleans up the comment a tad.

* The both complained about the isset() which was introduced in #28, I've replace it with a simple condition because IMO neither isset() nor !empty is necessary here.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

Attached patch is a small code cleanup to not duplicate calling $this->getField($row_index, $this->options['link_field']) twice.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

The test form earlier patches was lost, and need to be restored.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

This function begins with an "_" which means that it is meant to be private.

I suggest that you either:

(a) add a service to expose it so that we aren't using a private function, or
(b) rename the function here and everywhere it's used.

We need the core maintainer to weigh in on what they'd prefer.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

wfm too

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

Ping, I made a PR a while ago, please re-review.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

Yes, the expired IP addresses are now no longer banned.

This module is best used in conjunction with something like perimeter, which will automatically ban an IP, so that you don't have to do this manually.

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

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

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

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

πŸ‡ΊπŸ‡ΈUnited States douggreen Winchester, VA

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

Production build 0.69.0 2024