πŸ‡ΊπŸ‡ΈUnited States @Luke.Leber

Pennsylvania
Account created on 1 December 2016, about 7 years ago
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

This was 100% self inflicted by a module hijacking the textarea template! Sorry for the noise.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Re-targeting to 2.1.x-dev.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Hi marcoka, I'm switching gears to investigate this issue now, as I have hours devoted to ckeditor5 now. Sorry for the delay.

Are you still experiencing problems?

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania
πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

This actually squeaked into 2.1.x with a different issue!

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

A solution for this behavior has been committed to 2.1.x, so marking as fixed.

Thanks!

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

This behavior now has a solution and has been committed to 2.1.x, so marking as fixed.

Thanks!

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

This feature has been committed to 2.1.x, so marking as fixed.

Thanks!

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Committed and pushed to 2.1.x! Thanks for everything, Eduardo.

I'll be cutting a 2.1.0 feature release in the near future. Take care.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Switching over to RTBC given a green pass against 6.2.x and successful manual testing against a vanilla D10 site as well as on a highly customized site!

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

I reverted this back to the state it was on previously. Sorry for the unwarranted noise here.

The regression above is already filed in https://www.drupal.org/project/webform/issues/3419372 πŸ› Manage files js: BlockSubmit rename, missed caller Needs review

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

This seems to be caused by a missing function in webform.element.managed_file.js -- perhaps a partial refactoring took place at some time in the past and one spot was missed. Anyhow, swapping out the blockSubmit function, which does not exist, with Drupal.webformManagedFileBlockSubmit seems to work like a charm.

Setting to NR for !403.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Updated issue summary with clear steps to reproduce.

This may not be something that's easy to test on very fast connections. Network throttling may make it easier to submit the form while a file is in the process of being uploaded.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Clarified accessibility considerations with an example.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

I'm satisfied with the functional pieces and test coverage here. The only remaining task is an upgrade path + upgrade path testing.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania
πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Luke.Leber β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

I'm switching the status here to PMNMI - I'm very happy to commit bug fixes with test coverage, so if you can give an example of the input that is causing a deprecation notice, please chime in.

Thanks

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Hey Ignacio,

I invite you to check out the flexible-metadata-alter-architecture branch in the issue fork of https://www.drupal.org/project/oembed_lazyload/issues/3413985 ✨ Provide an extensible mechanism for thumbnail configuration and customization Needs work and see if it meets your needs. I've added support for more off-the-shelf rendering mechanisms while providing full backwards compatibility.

Let me know if this helps. I'm going to try to get #3413985 added to a 2.1.0 release in the coming weeks after tying up some loose ends.

Thanks

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Hey Omar,

I invite you to check out the flexible-metadata-alter-architecture branch in the issue fork of https://www.drupal.org/project/oembed_lazyload/issues/3413985 ✨ Provide an extensible mechanism for thumbnail configuration and customization Needs work and see if it meets your needs. I've added support for more off-the-shelf rendering mechanisms while providing full backwards compatibility.

Let me know if this helps. I'm going to try to get #3413985 added to a 2.1.0 release in the coming weeks after tying up some loose ends.

Thanks

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Hey Juan! Sorry for the lack of activity here. As it turns out, there was quite a bit to improve with the front-endy bits of this module.

I invite you to check out the flexible-metadata-alter-architecture branch in the issue fork of https://www.drupal.org/project/oembed_lazyload/issues/3413985 ✨ Provide an extensible mechanism for thumbnail configuration and customization Needs work and see if it meets your needs. I've added support for more off-the-shelf rendering mechanisms while providing full backwards compatibility.

Let me know if this helps. I'm going to try to get #3413985 added to a 2.1.0 release in the coming weeks after tying up some loose ends.

Thanks

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

I've just pushed a minimal viable product for the front-end bits. It's still not 100% useful, as the front-end still coerces everything to use a 16:9 aspect ratio.

Additional formatter settings are still required to make this a 100% turn-key solution. Luckily though, I think we can do all of this while still retaining backwards compatibility. It looks like we'll need a conditional container sizing mechanism that lets users choose between rendering things with...

  • An aspect ratio (has to be the default option, for b/c purposes
  • A maximum width / height (the options are already in the formatter settings, but are presently ignored!)

I'll take a peek at how the off-the-shelf oembed formatter does the CSS bits for the max width/height rendering strategy, but as the branch now stands, an end-to-end test should be possible after sprinkling some custom CSS on things.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

The contrib template seems to work just fine! Woohoo!

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Luke.Leber β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

I've pushed out a branch called flexible-metadata-alter-architecture that scaffolds in some API additions such that...

  1. Users may opt into customizing oEmbed metadata altering (it won't be activated by default)
  2. If enabled, all ProviderEnhancer plugin types will get a chance to customize metadata through a new method: public function alterOembedMetadata(array &$data, $url)
    For example, the YouTube provider enhancer can download thumbnails of various resolutions based on what's available on the remote end
  3. Speaking of YouTube...the stock enhancer now ships with a setting that allows end-users to opt into downloading the "highest resolution thumbnail available"

Work yet to be done:

  1. Continue adding test coverage -- there are now significant gaps.
  2. Figure out how to plumb this into the front-end while retaining API backwards-compatibility
πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

I have some performance details on mass update hooks to layout overrides. Over in the issue to add UUIDs to sections, updating 50,000 layout overrides can put a site into maintenance mode for a whopping approximately 15 minutes.

For stakeholders, that means downtime. $.02

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

I'd be happy to discuss this further. I have some ideas, but am sadly short on time due to #dayjob.

If you're on slack, feel free to hop into #oembed-lazyload and we can chat :-). I'm drafting up a quick and dirty of what I feel might be the most holistic approach to making thumbnails the most useful to the most users, hopefully finishing up within the next 12 mins or so.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Added a related issue. This one is really tricky.

I really feel that making blocking web requests on the server side in the formatter isn't the direction that should be taken, as it opens up the potential for denial of service security concerns.

Ideally this information would be captured when the oembed resource is created and then able to be referenced from the formatter. Obviously that's a much wider scope issue, but it's the only safe way I can see to accomplish this.

Thoughts?

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Closing this out as the automated updates patch provided here isn't required for compatibility and is creating unnecessary noise.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

There are known issues with iframes, filters, and 10.2. See https://www.drupal.org/project/drupal/issues/3410303 πŸ› FilterHtml data loss when iframe and/or textarea is allowed Active .

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Update issue title / summary.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

FYI - <textarea> has the same effect here as <iframe> does.

That's the only other HTML5 element that I've found to be problematic. Like my comment in the test, it might be worth taking a "kitchen sink" approach with test coverage, given this is largely governed by a third party library nowadays.

Cheers -- thanks for facilitating everything today.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Thanks for everything here. 1.0.2 coming shortly.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Let's see how the test bot likes this one...

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Hi! Can I ask what the oembed resource URL is that triggers this?

Typically bug fixes include test coverage. I would be happy to write a failing test if steps to reproduce could include the input that's causing the message.

Thanks!

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Perhaps this should be a must remove security coverage and flag it with a warning/control that requires the Project Ownership Queue to only allow adoption by a vetted user?

That makes 100% sense to me. The security advisory opt-in is, more or less, a social contract with the security team. If there's not a body to uphold that half of the contract, it's effectively null and void.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Hi all! I've moved all issue credit from here to https://www.drupal.org/project/juicebox/issues/3350883 🌱 Towards a stable Drupal 10+ release Active .

A stable release seems to be within sight. Please follow #3350883 for updates!

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Updated the Issue Summary / retargeted issue to work towards a stable release under the Security Advisory project.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Rebased on 4.0.x-dev - this should be easily resolved. Merge requests are welcome!

Added the Novice tag to open credit eligibility wider!

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Postponed on https://www.drupal.org/project/juicebox/issues/3407770 πŸ“Œ Move continuous integration testing to Gitlab Needs review

Since Gitlab CI is the new long term support CI provider, we should probably fix things against that versus Drupal CI.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

These changes should get us up and running on the new, non-deprecated continuous delivery platform!

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Add issue credit for help on slack!

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Hello! This should be resolved with https://www.drupal.org/project/juicebox/issues/3372033 πŸ› Error with layout builder Fixed which was just committed to 4.0.x-dev.

I plan on cutting a 4.0.0-alpha2 release within the week. Sorry for the delay in resolving this.

If there's still a problem after updating, please don't hesitate to reopen this issue. In the meantime, I've moved issue credits over as appropriate :-).

Thanks!

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

I've reached out on #gitlab on drupal slack for assistance here. Not quite sure why the gitlab runner is choking up.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

This was resolved through https://www.drupal.org/project/juicebox/issues/3372033 πŸ› Error with layout builder Fixed .

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Resolve violations added by last 2 commits..my bad there.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Let's try to forget that #15 ever happened.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Stop overriding constructors (let's see how the test bot does).

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Committed and pushed to 4.0.x - thanks all.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Committed and pushed to 4.0.x - thanks all.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Uniquify the juicebox preview ID (needed if there is more than one gallery rendering in preview mode on any given pageview).

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

HUGE +1! I do have one question regarding

If the module has opted in to Security team coverage, the member of the Project Update group MAY opt the module out of this coverage

Is there a communication that should come from the security team regarding a change to the advisory coverage? Some users may re-think the use of a module that is "on its way out" so to speak.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

FWIW - I managed to stumble in here while trying to add an error to an #item FAPI element. My particular case (heavy in custom code territory) is that I have a third party javascripty thing (reCAPTCHA) that needs to block form submit in certain cases.

$element['error'] = [
  '#type' => 'container',
  'recaptcha_error_message' => [
    '#type' => 'item',
    '#markup' => ''
  ]
];

// Sometime later...

$form_state->setError($element['error']['recaptcha_error_message'], $error);

I wonder -- are there certain FAPI element types that simply weren't intended to support error messages associated with them?

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

LGTM for an MVP.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

This is a very logical feature.

In order to land this feature, we'll need:

  1. A decision: are blocks are opt-in or opt-out by default?
  2. If required, based on previous decision, an upgrade path to ensure existing sites have no functional changes. This will likely be in the form of a post-update hook.
  3. Test coverage.

Setting to NW on #2

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Added test coverage and a more holistic solution to resolve layout builder issues.

Let's see if the test bot agrees.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Ah, there were two separate stack traces in the OP. (One regarding preg_replace and one regarding preg_match in different parts of the codebase!). I completely missed the preg_match notice.

That likely means that there are two different problems described in this issue.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

One improvement that I could see is having a hook_requirements implementation that checks for misconfigured breakpoints that overlap.

I don't have the bandwidth to add that feature at this time though, unfortunately.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Wow, I'm sorry. This issue never reached my inbox!

I'll try to find some time to get this evaluated. My gut is saying we skip the empty image (since it's an inline data: source -- preloading won't make any difference here) and treat the original image as the patch in #3 suggests.

This also needs some extended test coverage to trigger a failure in Drupal CI before a patch can be committed to fix it.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Hello, please check out the configuration guide for this module. Specifically https://www.drupal.org/docs/contributed-modules/responsive-image-preload... β†’

Let me know how that goes for you πŸ™‚.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

We've seen all manner of random, inexplicable weirdness with multi-web-server setups in Acquia Cloud Enterprise. We've blamed Memcache primarily, but this could be equally as likely to toss monkey wrenches around if it can be reproduced.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Quite a bit to unpack here :D. Thanks for the bugs.

TypeError: Exception::__construct(): Argument #2 ($code) must be of type int, array given in Exception->__construct() (line 368 of modules\contrib\juicebox\src\Plugin\views\style\JuiceboxDisplayStyle.php).

This one makes sense! It looks like the module is sort of half-implementing a translation here:

throw new \Exception('Empty or invalid field source @source detected for Juicebox view-based gallery.', ['@source' => $source_field]);

Luckily, Drupal's coding guidelines says that we should not translate exception messages, so this can be resolved simply via
throw new \Exception('Empty or invalid field source ' . $source_field . ' detected for Juicebox view-based gallery.');

--

For the other ones, it would be really awesome if someone could upload the configuration for the view that was causing the snag.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

It looks like this issue stems from the fact that when generating previews in Layout Builder, there is not always an entity ID available (due to the fact that some layouts use default layout storage.

While we can fix this by simply null-coalescing the argument like so...

      $arg = preg_replace('/[^0-9a-zA-Z-]/', '-', $arg ?? '');

That doesn't solve the underlying UX issue in that Juicebox fails to provide a useful preview in contexts like Layout Builder. I've opened https://www.drupal.org/project/juicebox/issues/3403266 ✨ Provide a useful "preview" UX for layout builder Active as a follow-up, since resolving this issue won't really make the UX any better for production users.

I've attached a patch that should maintain perfect backwards compatibility with the ID generator while also preventing the notice.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

...and one more change to rename the test case to something more appropriate.

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Must be getting tired. Here's the correct patch...heh. #18 had an improperly named test module (from another issue in Juicebox :-) ).

πŸ‡ΊπŸ‡ΈUnited States Luke.Leber Pennsylvania

Resolve coding standards violations that were in scope for this changeset.

Production build https://api.contrib.social 0.61.6-2-g546bc20