It looks like perhaps an HTML table is needed in the markup to be diffed here? Can you please provide the content you're seeing this on? 🤞
We'll definitely need a failing automated test before committing any sort of bug fix to this module.
👋 diff 1.1.0 is now released.
If you run into any issues please don't hesitate to open another issue.
Thanks for nudging me into actually getting that done. 😅
Pushed to 1.0.x - thanks!
Good morning, I'm hoping to cut a new release this weekend with diff 2.x support. There's another issue that's RTBC in the queue that's ready to merge I feel 👍
We've started to use drupalSettings + js_settings_alter to provide server-side rendered templates to the frontend.
Our Drupal.theme.messages implementation pulls the markup from drupalSettings. Might not be robust enough for core, but has worked rather well for us...and even covers the obscure file.module validation cruft too.
Seeing as https://www.drupal.org/project/webform/issues/3460222#comment-15692042 📌 Consider deprecating Choices Closed: duplicate closed a related issue as a duplicate, I wanted to bring over the opinion that deprecating, discouraging the use of Choices, and ultimately removing it from Webform seems to be the most responsible path forward here.
The library is abandonware.
Agreed! This is actually a core issue I believe.
Added upstream issue and marked postponed 👍.
Luke.Leber → changed the visibility of the branch 10.1.x to hidden.
re: #22 - It looks like the patch from #5 was made against Drupal 9 (which shipped with *.es6.js files transpiled to es5). #20 seems to be for Drupal ^10.
MR !3355 just straight-up removes the javascript, which isn't a valid approach IMO.
I feel this should be addressed and have test coverage added in a follow-up. Javascript based testing like this is very hard relative to the simplicity of the fix.
Overriding constructors isn't recommended when the parent class implements the create method, as constructors aren't covered by B/C. Leaving as NW.
Please feel free to re-open if you are still experiencing issues after going through the docs :-).
Maybe next time, update bot.
All green on ^10.2 || ^11. Given the automated test coverage, and lack of non-test changes here, I feel it's safe enough to merge as-is.
Cumulative release testing must happen before a new stable is cut.
Luke.Leber → created an issue.
Injecting here that this would make styling file upload errors quite a bit easier!
It'd pay to take a hard look at the file module to determine how feasible the approach in the IS actually is.
Temp patch for 10.3.x that prevents a silly
1. Site is slow
2. Site is fast (after code deployed, before updb runs)
3. Site is slow (after updb runs, before config imports)
4. Site is fast (after config import turns the module back off
deployment strategy 🤭.
Intended use is to patch only during the 10.3 upgrade then remove. This can be helpful for users that consistently see database deadlock problems arising from huge block plugin cache writes...
diff --git a/core/modules/layout_builder/layout_builder.post_update.php b/core/modules/layout_builder/layout_builder.post_update.php
index ee735bf41b..6fd8891218 100644
--- a/core/modules/layout_builder/layout_builder.post_update.php
+++ b/core/modules/layout_builder/layout_builder.post_update.php
@@ -74,5 +74,6 @@ function layout_builder_post_update_timestamp_formatter(?array &$sandbox = NULL)
* Enable the expose all fields feature flag module.
*/
function layout_builder_post_update_enable_expose_field_block_feature_flag(): void {
- \Drupal::service('module_installer')->install(['layout_builder_expose_all_field_blocks']);
+ // It's silly to install this only to have config import uninstall it.
+ // \Drupal::service('module_installer')->install(['layout_builder_expose_all_field_blocks']);
}
Telephone numbers are very complex. A simple input mask isn't a reliable mechanism here, as number formats do vary wildly from country to country.
There's an international telephone mode feature for this that ships with country-specific masking capabilities 👍.
I can confirm some general weirdness with the i18n phone feature when used in conjunction with client side_validation.
It seems like both clientside_validation's and webform's JavaScript is both running against it and that can lead to some confusion when trying to theme things (the error messages can appear in different places in the DOM based on which validator "wins".
Luke.Leber → created an issue.
I'm not 100% sure if Drupal/yml marks a distinction between double quotes vs single quotes in yml unless there are escape characters involved. Might be something Wim or the config validation folks could answer pretty easily as they're the config wizards 😆.
Woo!
Indeed it has; adjusted the composer requirement...let's see what the test runner thinks.
The MR looks great; I'll try to get this into a stable d11-compatible release this week after some manual testing. Sorry it's taken this long.
Luke.Leber → made their first commit to this issue’s fork.
Savvy users can also utilize composer to further harden their applications:
Example: Nuke the vulnerable file from orbit on `composer install`
"scripts": {
"drupal-scaffold": "DrupalComposer\\DrupalScaffold\\Plugin::scaffold",
"post-install-cmd": [
"rm docroot/libraries/choices/public/index.html"
]
}
🛡️
Just chiming in here from a different perspective: our group has disallowed the use of Choices.js for years due to inherent accessibility issues.
Given that...
- The software hasn't had a commit in ~3 years
- The software ships with a vulnerability
- The Select2 implementation ranked higher in [our] a11y testing
Might it be an alternative to deprecate / disable its use in Webform by default? Shipping what is effectively abandonware doesn't seem sustainable.
Thanks Mark. I'll ping you in edudu on next steps for release.
Postponed on https://www.drupal.org/project/media_library_form_element/issues/3432301 📌 Request to remove build dependencies in stable releases Needs review
Opened drupal.org/project/media_library_form_element/issues/3457129 as a follow-up; re-closing :-)
Luke.Leber → created an issue.
Following up here - this is what I get on Drupal 10.1 when attempting to run a drush cr on a site with 2.x:
www-data@5bae608a62be:~/html$ drush cr
In YamlFileLoader.php line 161:
The configuration key "autoconfigure" cannot be used to define a default value in "modules/contrib/media_library_form_element/media_library_form_element.services.yml". Allowed keys are "public", "tags", "autowire".
I think we may have to declare ^10.3 only here?
Opened https://www.drupal.org/project/media_library_form_element/issues/3458296 📌 Update build dependencies to work with a supported Node version Active as a follow-up for a better long-term solution.
Luke.Leber → created an issue.
I've opened !13 against this issue, but it's not a good solution (imo).
I had to downgrade all the way to Node 10 to get this project to build. While !13 is likely better than HEAD, this project should be upgraded to work with Node 20 or later; perhaps in a follow-up.
Hey @millerrs - we're working toward a 2.1.0 release. There's an ongoing discussion right now that has to be resolved first.
I think we might need a follow-up to update the Choices library after https://github.com/Choices-js/Choices/pull/1162 lands upstream (Thanks
Jürgen!)
Presently, the choices.js library, as pulled in via `composer.libraries.json` will continue to place `./[web-root]/libraries/choices/public/index.html` on disk which loads scripts from polyfill.io. Given sophisticated enough malicious javascript, this file could potentially be used by threat actors to launch phishing attacks.
It looks like the change was originally reverted on 6.2.x back on April 23, but wasn't on 6.x until June 24. Seems 6.x and 6.2.x are kept somewhat in sync.
Timing on !5500 on 19621 blocks, 24290 revisions, and a batch size of 50 (default).
real 0m19.932s
user 0m11.936s
sys 0m0.955s
~20 seconds is a lot more acceptable than 30 minutes! What a difference the Database API makes versus the Entity API!
Just helping out with triage where possible. This seems to be definitively fixed by #20/21, no?
I feel there are actually a couple different issues with i18n telephone validation. The fixes should happen within the webform_clientside_validation submodule, no?
Hey Liam,
In the coming weeks I'll be doing a screen cast about our experience in theming out a somewhat complete feature set of Webform functionality.
There were a few other probable IE-isms that popped out at us (some datetime stuff, specifically). I wonder if a meta might be in order to group that effort?
I believe that the root cause is that Drupal's XSS utility is somewhat dated. There are a pile of "safe" HTML5 tags that are stripped out -- a subset of SVG's elements among them.
This is a somewhat heavy-handed approach that immediately comes to mind to resolve this in the mean-time:
$status_messages = ['#type' => 'status_messages'];
// Rather than concatenating things in `#prefix`, extract the
// existing prefix and only run XSS filter on that.
$prefix = $form['#prefix'] ?? '';
if (!($prefix instanceof MarkupInterface)) {
$prefix = Xss::filterAdmin($prefix);
}
// Then render a build array noting that...
// 1. $prefix has been sanitized.
// 2. $status_messages has been sanitized (via Twig)
$build = [
['#markup' => Markup::create($prefix)],
$status_messages,
array_filter($form, static fn($key) => $key !== '#prefix', ARRAY_FILTER_USE_KEY),
];
$output = $renderer->renderRoot($build);
Luke.Leber → created an issue.
Additionally manually confirmed attribute coverage against a "real world" site (via Webform).
- Audio files
- Document files
- Generic files (this is the core render element!)
- Image files
- Video files
Cheers
Fingers crossed, test bot.
Approach-wise, I feel that this logic belongs in \Drupal\file\Element\ManagedFile::processManagedFile class versus the FormBuilder because the file module provides the managed_file element plugin.
This change needs tests. It would also be great to test Webform to ensure that the elements that extend managed file are also swept up and fixed.
Luke.Leber → changed the visibility of the branch 2998857-additional-information-needed to hidden.
Luke.Leber → made their first commit to this issue’s fork.
saschaeggi → credited Luke.Leber → .
#68 seems to outline another instance where treating all oembed providers equally simply doesn't work in practice.
In my mind this adds a lot more weight to the issues floating around that support altering various inputs and outputs of the oembed media system.
joegl, feel free to open an issue with https://www.drupal.org/project/oembed_lazyload → and we may be able to make this accessibility enhancement available with no code and/or patches for end-users faster than core is able to adapt to the need. 👍
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:
- 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.
- The
resetmethod 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:
- Quicker loads overall
- 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.