🇺🇸United States @Luke.Leber

I, the module maintainer, would like to opt out of automated suggestions for this module. It's highly unlikely that anything that the bot produces will actually work here.

See https://www.drupal.org/project/diff_plus/issues/3447124 📌 [PP-1] Drop support for Drupal 9; Add support for Drupal 11 Postponed to track compatibility.

Setting postponed on https://www.drupal.org/project/diff/issues/3429862 📌 Automated Drupal 11 compatibility fixes for diff Needs review

Luke.Leber → created an issue.

Thanks for the bug report. Unfortunately, I wasn't able to reproduce a White Screen of Death as in #2962941, but I was able to trigger an E_WARNING by manually editing the view mode to a nonsensical value.

Warning: Undefined array key "this-view-mode-does-not-exist" in Drupal\diff_plus\Plugin\diff\Layout\VisualInlineHtml5DiffLayout->getDisplayModeOptions() (line 154 of modules/contrib/diff_plus/src/Plugin/diff/Layout/VisualInlineHtml5DiffLayout.php).
Drupal\diff_plus\Plugin\diff\Layout\VisualInlineHtml5DiffLayout->getDisplayModeOptions(Object, Object, Object, 'this-view-mode-does-not-exist') (Line: 234)
Drupal\diff_plus\Plugin\diff\Layout\VisualInlineHtml5DiffLayout->build(Object, Object, Object) (Line: 168)
Drupal\diff\Controller\PluginRevisionController->compareEntityRevisions(Object, Object, Object, 'visual_inline_html5') (Line: 49)
Drupal\diff\Controller\NodeRevisionController->compareNodeRevisions(Object, Object, Object, 'visual_inline_html5')
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 592)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 53)
Asm89\Stack\Cors->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 704)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Is this what you were seeing?

In order to fix a bug, we'll first need a failing test. Can you update the steps to reproduce so that I can write such a test case?

Thanks!

Looking at the upstream issue, there was a valid point made that <h2><div>content</div></h2> isn't legal html.

That may help explain why this hasn't been addressed yet. Given how long this has been open, might it be an option for folks suffering from this to consider migrating to markup like <h2><span>content</span></h2> instead? I'm not convinced that cksource will make divs in headings work as a product feature.

!48 looks good to me. I understand this is basically impossible to test, but there shouldn't be any harm in delaying the collection of settings.

We've sporadically experienced weird "should never happen" things on the platform, so after this lands, I'll keep this issue in mind and chime in if another inexplicable thing (i.e. cache corruption) happens afterwards. I think that's the only reasonable evaluation we can do against the theory written out in the I.S.

A b/c break was inadvertently added by https://git.drupalcode.org/project/drupal/-/commit/6a43b511df9e26aa5abf0... when jquery was swapped out for vanilla JS.

Previously, jQuery was more tolerant of an element not found condition. The vanilla JS results in an uncaught exception.

I was going to remove the needs tests tag, but Dave beat me to it :D

Ahh, I see - the legacy option can still use the new placeholder strategy effectively. I'll get a patch whipped up when I have a free moment.

Hey Eduardo,

I see you're still using the legacy formatter option. To preserve backwards compatibility until I can roll a 3.x branch, I added some additional field formatter settings that should be used to leverage the new thumbnails. Can you try any of the other configurations out to see if they work for your theme?

I know it can be a little confusing; I'm trying to balance not breaking backwards compatibility for existing users with providing intuitive defaults for new users.

Luke.Leber → created an issue.

Fairly safe to assume this one's outdated, as Google Optimize isn't a product anymore :-).

Thanks, @greggles.

Which page?

https://developers.google.com/tag-platform/devguides/gtag-integration

For the record, I wholeheartedly believe that the use of the developer_id property is innocuous here on the part of the module maintainers. I was simply seeking more information on it.

The Google documentation uses language like "your customers", which implies somewhat of a different relationship with end-users than is traditionally implied by FOSS Drupal modules that have fluid maintainers.

Updated I.S. with answers (best that can be done with known information).

There may be some level of difference being that the Magento developer ID is likely under the umbrella of the Adobe Corporation whereas the google_tag module module doesn't really have a well known "legal entity" -- for lack of a better term -- behind it.

I appreciate anything you can dig up on this. Thanks

It's something new that's never been seen before on our team and there is precious little information available on this other than one page from Google explaining how, but nothing beyond that.

The developer ID request form is seemingly bound to a Google account of an individual.

Matt, can you link to any google.com hosted documentation that confirms what is said in #5 about access policies?

Luke.Leber → created an issue.

Potentially related issue on the same note as #22, albeit not in Middleware, but a different service constructor.

NW for test fails.

Let's see what the test bot thinks.

Luke.Leber → created an issue.

Luke.Leber → created an issue.

So the reproduction steps include needing an end-user to tamper with a request outside of the normal user agent controls? If that's the case, I'm not quite sure it qualifies for a critical triage. Sliding back down to normal. If there's a way for an end-user to hit this without request tampering, please feel free to re-triage accordingly!

Agreed it needs a new test case and some additional logic to check not only for emptiness, but data type. A single new case should do the trick.

Some feature suggestions for 2.x:

A more extensible twig template based UI.

Presently, the UI can be rather spartan and cold compared to the implementations in other content management systems. Something as simple as user avatars can make a world of difference to non-technical users. Giving themes the ability to customize how the Diff UI looks is a sorely needed UX enhancement. This is how I heavy-handedly tackled this in diff_plus: https://git.drupalcode.org/project/diff_plus/-/blob/1.0.x/src/EventSubsc...

The default HTMLPurifier configuration just doesn't work very well sometimes.

I ditched HTMLPurifier in diff_plus and added some tweaks and toggles to make higher fidelity HTML5 complex content renderings ( https://git.drupalcode.org/project/diff_plus/-/blob/1.0.x/src/Plugin/dif... ).

There has never been a more opportune moment to make Diff entity agnostic.

Drupal isn't all about nodes anymore. We have all sorts of entity types in contrib and custom-land, now with a generic revision UI available, a major version bump seems like the right time to target getting all modernized here. This has not been tackled in diff_plus yet.

If these are too extreme, I'll gladly continue maintaining things in diff_plus, but just wanted to weigh in and let the maintainers know that I'm very open to collaboration.

Thanks.

Great to see some movement in this module. I invite ya'll to check out https://drupal.org/project/diff_plus for anything that might be beneficial to include in a new major.

I've opened a merge request, but I do think there may be deeper problems related to the international telephone input validation holistically including:

1. There seems to be some repetition / contention between the error messaging added by the webform.element.telephone.js library and that which clientside validation adds.
2. The reset method is called on keyup -- this can effectively clear validation messages for keyboard users before they have a chance to hear what the errors are.

Leaving at Needs Work for the above points -- really need maintainer feedback, I think :-).

Cheers!

Fixed issue title...it's been quite a morning!

Luke.Leber → created an issue.

I may have some tangentially related info here. We recently found that due to how layout builder works (prior to Drupal 10.3), the accumulation of derived field blocks resulted in a single cache item in `cache_discovery` having a size of over 6mb.

This _may_ result in extreme performance problems for memcache users, because the default config won't store individual items over 1mb in size.

Just a theory, but such a large, virtually uncatchable, record could explain a lot of the "this should not be able to happen under normal circumstances" responses to this issue. Systems break down in wild ways when the underlying infrastructure doesn't behave as expected.

I'm curious if this happens in 10.3+ with most derived field blocks disabled so the block plugins CID falls below 1mb?

Problem/Motivation

Living example of how this issue impacts a "real" site: https://dev9.worldcampus.psu.edu/form/example-ranges

When configuring a Range element to Output the range's value to the Left AND there is not a prefix set on the range element itself, the output element is erroneously rendered as a field suffix!

This is the code in question:

https://git.drupalcode.org/project/webform/-/blob/6.2.x/src/Plugin/Webfo...

      if ($element['#output'] === 'left') {
        if (isset($element['#field_prefix'])) {
          $element['#field_prefix'] = [
            'output' => $output,
            'delimiter' => ['#markup' => '<span class="webform-range-output-delimiter"></span>'],
            'content' => (is_array($element['#field_prefix'])) ? $element['#field_prefix'] : ['#markup' => $element['#field_prefix']],
          ];
        }
        else {
          $element['#field_suffix'] = [  // <-- Shouldn't this be $element['#field_prefix'] instead?
            'output' => $output,
            'delimiter' => ['#markup' => '<span class="webform-range-output-delimiter"></span>'],
          ];
        }
      }

Steps to reproduce

See attached webform; note that the Range (value to left) element renders incorrectly.

Proposed resolution

Super simple! Let's change '#field_suffix' to '#field_prefix'!

User interface changes

If anyone has worked around this by using something like `flex-direction: row-reverse`, their hack-around will break.

API changes

None

Data model changes

None

Hey Jacob,

I may not have communicated things as clearly as intended, so I recorded a quick screencast that walks through what I found (~3 minutes). If I'm still not understanding correctly, please feel free to re-close and I won't re-open it again.

Thanks!

This change makes total sense and the CR reads very well.

As one of the few that have already hijacked/turned off some of the library assets in scope here, the organization seems much more closely aligned with a component driven system.

Is Rector capable of working with theme info files to update the paths automatically for upgrading users? A buttery smooth automated upgrade path is the only not-strictly-positive feedback here.

Ran against a page on a site with ~230 CSS-only component libs and ~5 JS-only libs, and ~10 CSS+JS libs.

Single page

Without MR Applied:

    DOM: 65kB
    CSS: 25.4kB
    JS: 96.9kB

With MR Applied:

    DOM: 64.4kB
    CSS: 25.4kB transferred
    JS: 97.1kB transferred

Delta:

    DOM -.6kB
    CSS 0kB
    JS +.2kB

Net: -.4kB

No clue why / how an extra 200 bytes of JS was delivered. That shouldn't have been possible. In theory, less variance means:

1. Quicker loads overall
2. Fewer Drupal bootstraps (important for managed hosting customers who pay per view).

This improvement sounds good to me all around.

Note - I didn't review the code; just performed a basic sanity test against a highly customized 10.1.x site.

Seems HEAD was broken. Back to NR. 👍

This module is obsolete and will no longer receive any commits.

I think there's a better way to approach this.

See https://www.drupal.org/docs/develop/git/git-for-drupal-project-maintaine... → :-)

For users that can't wait for this to land and don't feel like patching, one can also use the libraries-override feature in their themes to remove it that way today.

libraries-override:
  webform/libraries.choices:
    js:
      'https://polyfill.io/v3/polyfill.min.js?features=es5%2Ces6%2CArray.prototype.includes%2Cfetch%2CCustomEvent%2CElement.prototype.closest%2CElement.prototype.classList': false

💪🛡️

Luke.Leber → created an issue.

Rebased against 11.x -- hopefully the test bot doesn't freak out 😅.

Additionally, the concern brought up in #98 is still valid. Upon manual testing, the following procedure results in data loss:

1. Create a media entity that implements oembed
2. Disconnect from the public internet
3. Update the metadata
4. Notice that the original thumbnail is gone and is not replaced with a new one

Luke.Leber → made their first commit to this issue’s fork.

IMO the batch strategy needs work.

It seems to batch on tables which doesn't account for HUGE tables. Consider the following:

• Layout field for `block_content` entities has 30 records
• Layout field for `node` has 300,000 records

The batch really doesn't help with memory constraints given it's not breaking up the CRUD operations on the excessively large table.

I've also re-ran the update and here were the results:

real    25m6.900s
user    1m12.659s
sys     0m7.445s

The end result still seems to be over 25 minutes of site downtime for ~50,000 entity revisions.

If (that's a big if) I have time tomorrow, I'll try to run this with xdebug profiling to help visualize what's taking the most time.

As somewhat of a side-note, I've found that the Entity API is not very well suited for crunching tons of data. I've found that using the database API for updates has been one of the most meaningful impacts on performance. Perhaps we can use the database API for setting the owner versus calling $blockContent->setOwnerId('blah')->setSyncing(TRUE)->save() to trim minutes off?

Setting back to NW based on the (flaky?) real world test runs.

I have some metrics. I don't know how meaningful they are, but...

The initial run that spurred me into making this issue:

real    28m32.144s
user    9m47.665s
sys     0m55.545s

Now that I've ran this a couple more times, I'm assuming that this long run can be attributed to Microsoft Defender running on the Linux environment that was running the update. (Don't ask me to explain why it's a good idea for Microsoft Defender to run on Linux environments -- ask the PSU security office...)

More closely monitored runs -- where MS Defender was not engaged:

Before the patch:

real    14m4.685s
user    5m2.253s
sys     0m11.007s

After the MR is applied:

real    16m12.257s
user    6m41.996s
sys     0m14.335s

I'm not sure that the optimization has a meaningful impact based on these numbers. Performance testing is pretty tough to nail down given the sample size and my ability / time to run upgrades against the environment.

Strangely enough with the MR changes applied I get:

>  [notice] Update started: block_content_update_10301
>  [notice] Installed uid field for BlockContent entities.
>  [notice] Update completed: block_content_update_10301
>  [notice] Update started: block_content_post_update_set_owner
>  [error]  SQLSTATE[42000]: Syntax error or access violation: 1140 In aggregated query without GROUP BY, expression #1 of SELECT list contains nonaggregated column 'drupal.bcr.revision_user'; this is incompatible with sql_mode=only_full_group_by: SELECT "bcr"."revision_user" AS "uid", MIN(revision_id) AS "revision_id"
> FROM
> "block_content_revision" "bcr"
> WHERE "id" = :db_condition_placeholder_0; Array
> (
>     [:db_condition_placeholder_0] => 21
> )
>  
>  [error]  Update failed: block_content_post_update_set_owner 
 [error]  Update aborted by: block_content_post_update_set_owner 
 [error]  Finished performing updates. 

real    0m11.731s
user    0m6.789s
sys     0m0.518s

Just using the off-the-shelf mysql configuration for Ubuntu 22.04; I've not ran this in a hosting provider environment.

Luke.Leber → created an issue.

Looks good to me - this is a non-disruptive feature.

Thanks!

Tests pass. Good enough for me.

Luke.Leber → created an issue.

Luke.Leber → created an issue.

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

Luke.Leber → created an issue. See original summary → .

Re-targeting to 2.1.x-dev.

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?

Luke.Leber → created an issue.

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

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

Thanks!

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

Thanks!

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

Thanks!

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.

Retargeting to 2.1.x.

Luke.Leber → created an issue.

NR for !3.

Luke.Leber → created an issue.

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!

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

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.

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.

Luke.Leber → created an issue.

Clarified accessibility considerations with an example.

Luke.Leber → created an issue.

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

Luke.Leber → created an issue.

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

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

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

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

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.

The contrib template seems to work just fine! Woohoo!

Luke.Leber → created an issue.

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

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

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.