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

Account created on 1 October 2009, almost 15 years ago
#

Merge Requests

Recent comments

πŸ‡ΊπŸ‡ΈUnited States chrisolof

Notes from testing this MR against real traffic for about two weeks now:

This is currently performing very well / as designed against real traffic. The addition of the ASN lookup on those requests where it is actually necessary (requester not openly identifying as bot and not already blocked at the visitor-level) is so fast, even without the optional C extension, that it is imperceptible to our end-users. On the other hand, rate-limiting all crawlers, including those that horizontally spread out across multiple IPs and/or user agent strings, is a perceptible performance boost for our end-users.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

Here's an initial patch that makes rescheduling after webform / source entity updates optional. Needs review.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

I like this idea. I don't think number is the right type (we'd lose the excellent input format flexibility we have today), but I see no downsides to switching to tel. It sounds like it would have client-side UX benefits and supports all phone number formats (assuming we don't specify a pattern).

I think this also opens up some neat future possibilities. The patch looks fine but needs testing.

Also, related, over in πŸ› Undefined array key "extension" in /src/Element/PhoneNumber.php on line 237 Needs review I'm proposing we switch the extension sub-field type to number.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

The attached patch resolves the issue in the affected environments.

I've made three changes in here:

1. Switch the extension sub-field type from text to numeric.
2. Remove the custom numeric extension validation code in Drupal\phone_number\Element\PhoneNumber::phoneNumberValidate() that is no longer needed.
3. Ensure the array key "extension" is actually set before trying to access its value in Drupal\phone_number\Element\PhoneNumber::phoneNumberValidate().

I suspect the third change is most responsible for eliminating the error here, and the patch could certainly be reduced to just that change. However while examining the validation of our extension sub-field it felt like a good opportunity to correct its field type and reduce our element's validation code a little bit. It could be this validation area was written before core had a numeric field type, or perhaps the original author was unaware of it. At any rate, this also enables client-side validation of the extension field, which is nice and something we've never had.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

Looks good to me. Thanks Vaish!

πŸ‡ΊπŸ‡ΈUnited States chrisolof

Add Webform Element Values Limit under Enhancements section.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

Merged latest 6.2.x changes into MR branch. Removed unused use statement. Added note about the default timezone that will be used to interpret the custom send-on date if not provided in the date string (so a source-code-dive isn't necessary to figure that out). Current state of the MR is attached, along with a screenshot of the changed field.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

MR 425 opened. Status changed to needs-review. Proposed resolution and UI changes sections updated to match the resolution proposed in MR 425. Immutable diff (patch) attached here capturing the current state of this MR.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

After diving into the code a bit more I've realized, rather than broadening out to accepting just ISO 8601 and MYSQL DATETIME formats, it probably makes more sense to accept anything PHP's strtotime() function can consume. The downstream code just throws this value against strtotime(), which accepts many formats, including both MYSQL DATETIME and ISO 8601. As such, limiting entry to just MYSQL DATETIME and/or ISO 8601 seems unwarranted and adds what feels like unnecessary complexity to the validation method.

The approach I'm taking with the MR currently is to drop the "ISO date" stuff and instead ask for a date or date-time string in a format compatible with PHP's strtotime() function, keeping the current MYSQL DATETIME YYYY-MM-DD or YYYY-MM-DD HH:MM:SS as the suggested format. This seems to fix the bug in the least disruptive way, bringing in actual ISO 8601 date format support, keeping MYSQL DATETIME support, slightly reducing validation complexity, and adding support for many other date formats.

I'll post the MR for review after I've confirmed tests pass locally.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

Yes - confirming this looks great and works well. Thanks Vaish!

πŸ‡ΊπŸ‡ΈUnited States chrisolof

Excellent improvements here. The current state of the MR seems to work well and does not appear to be disruptive to existing installations.
Thanks everyone!

πŸ‡ΊπŸ‡ΈUnited States chrisolof

This looks good and works well. Thanks Vaish!

I think we should remove the 'country-code' part, since it seems to just duplicate the "country" part. With that change we would match up to what data parts exist at the field's schema. Maybe someday we can open up another property here to expose country name, but probably not necessary at this point.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

I tried applying the patch from #22, but ran into errors due to the removal of Drupal\Core\Security\TrustedCallbackInterface. I think the removal of TrustedCallbackInterface was likely unintentional.

Attached is a re-roll of the patch in #11 against 10.2.x, with the requested post_update hook added so a cache clear is forced.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

Adding interdif.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

After posting I noticed the presence of the "Multiple entities" config option, and realized the prior patch could be improved by eliminating the canonical rerouting behavior (and hasMultiEntityPreviewLinks() checks) one level up when preview_link.settings:multiple_entities is FALSE. I think this will have a small performance benefit for sites that do not utilize multi-entity previews.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

Attached is an alternative patch that attempts to resolve the issue here by restricting the canonical rerouting behavior to just multi-entity preview links, where it seems to serve a purpose.

It may make sense to go even further with another restriction: only apply the reroute behavior if the entity's latest revision is unpublished. That would allow this rerouting behavior on multi-entity preview links to drop out automatically as preview-versions of entities become published (and thus the preview link is no longer needed to see the latest version).

I'm also re-titling the issue to "Canonical reroute behavior is confusing & feels unnecessary in many situations", switching it to a bug report, and adding these alternate resolution ideas into the list of proposed resolutions.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

I just ran into this odd default behavior while evaluating the module. It looks like it's useful (perhaps needed?) when granting a multi-entity preview via a single link. More details in #3155009: Grant access to multiple entities by using a single preview link β†’ .

For single-entity preview links, however, this redirection behavior feels like a bug. I would expect to see the preview-version through the preview link and the regular version through the regular link.

I'm tempted to provide an alternative patch that leaves this redirection in place for multi-entity preview links, but removes it for single-entity preview links where it seems unnecessary and confusing.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

I just ran into this bug while attempting to display ECK entities in a views block using the swiper formatter views plugin. The changes in MR 2 fix the issue for me. Thanks @bbu23!

Attached is an immutable patch snapshot of MR 2, as it stands today.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

Looks great - thank you for your contribution! This has been merged into our 1.0.x branch and will be part of our next release.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

The latest patch (in comment #8) has been merged into our 2.x branch and is included in the latest 2.0.0 release. Thanks for the help everyone!

πŸ‡ΊπŸ‡ΈUnited States chrisolof

This has been merged into our 2.x branch and is included in the latest 2.0.0 release.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

This has been merged into our 2.x branch and is included in the latest 2.0.0 release.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

The attached patch adds scrollbar support into the module. Default settings match to API defaults. Empty checking is in place to avoid the necessity of an update hook to backfill existing swiper template config. I think the addition of an update hook here would be an improvement, but this is a start.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

The attached patch adds basic mousewheel support.

It also removes some unnecessary trailing whitespace from the edited files (this was done automatically by my editor).

I almost went with an update hook to back-fill the new config parameter, but seeing no prior update hooks / pattern of this in the module, I decided instead to just handle the situation in the form via a null coalescing operator.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

chrisolof β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

This is really neat. The flexibility to add multiple types of each property (say a work and home phone number, for example) and configure them all independently per your unique requirements immediately struck me as a nice improvement over the previous, fairly hard-coded model.

I haven't done a full implementation test against this branch yet, but after some light testing and code review I spotted a few potential improvements we might want to make at the early stages here:

- Omit rendering of empty property lines. I believe the prior setup accomplished as much via conditional output in the template. I just pushed this change into the issue fork.
- I suspect the render() method in the Photo plugin is not necessary, given the render method in VcardPropertyPluginBase.
- I wonder if supportsHomeWorkType might be better held in the annotation config / plugin definition...
- It seems to me that the plugin interface could drop prepareOutputString(), as it seems to be more of just a helper-method in VcardPropertyPluginBase, with nothing outside of VcardPropertyPluginBase depending on it. The render method seems like the one to keep in the interface since it's what the rest of the system hooks up to for output from the plugin. It's possible a plugin could decide to render by other means (or more directly), without following the prepareOutputString() then render() model.
- Document return values on prepareOutputString() and render(). I took a stab at this in my push to the issue fork since the empty-checking was related.

Again, I haven't done extensive testing, but the basics appear to be in place and working here. I would say this still needs work, especially given the lack of an upgrade path, and it probably deserves a major version change when it lands in a release, but it is definitely all coming together here.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

Oh this is very cool! I'll try and get you a review here next week. I really like the new plugin design.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

Just pulled in the latest 3.x and tested. It does appear to be working - thank you!

πŸ‡ΊπŸ‡ΈUnited States chrisolof

New patch attached, created against the latest 3.x branch code.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

Oh bummer. I was incorrectly assuming this module was open to complete v4.0 property support.

At any rate I've already created a patch to add NOTE property support, so I'll post it here for your consideration. Maybe it will help others with similar needs.

Thank you for the work-around advice.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

The attached patch reduces the required properties to just full name (FN), leaving everything else optional.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

The attached patch against 2.x adds the missing config schema file.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

Confirming that the patch in #5 fixes the bug in my tests on a PHP 8.1 environment.

πŸ‡ΊπŸ‡ΈUnited States chrisolof

The patch in #2 is working in my tests on a PHP 8.1 environment. The only change I would suggest is moving from intval() to typecasting (int) since:

- We don't need to adjust the base for the conversion
- Typecasting to int is quite a bit faster

πŸ‡ΊπŸ‡ΈUnited States chrisolof

This is still an issue in the 2.x branch. However, setting the facet operator to AND appears to fix it. For my use case switching from OR to AND was acceptable, but I think we still have an issue here when OR is the facet operator. When OR is the operator, the chosen facet will not show if it normally falls outside of the hard-limited choices, regardless of sorting by active state.

Production build 0.69.0 2024