@odensc

Thanks for the rebase @mortona2k. I also added the fixes from #9 into the MR.

I rerolled the MR again and made a small change to use array_key_exists instead of isset to fix a rare edge-case where ['select' => null, 'other' => null] which can happen in some complex wizard forms with conditional _other fields.

@liam morland, Appreciate you taking the time to look at this issue earlier. Is there anything else I can do to get it in a mergeable state? Thanks!

I've created an MR which removes the escapeValue function and fixes the issue.

There is an existing test for HTML in captions, but it didn't catch this issue since it's just testing a raw <drupal-entity> tag that has a correctly encoded data-caption attribute. This issue only manifests if the caption is set via the entity embed dialog.

I'm not familiar enough with the tests here to know how to set up something that would cover this case, since it would need to interface with both the dialog and the CKEditor integration at the same time, and right now there only appear to be separate tests for each.

The source of this issue appears to be that the data-caption value is getting double-encoded, e.g. &<strong&> instead of <strong> as it used to be with CKEditor 4.

This is because the dataDowncast step in the CKEditor 5 plugin is encoding the already-encoded data-caption value. Removing the escapeValue validation function as @ericpugh mentions should fix this, since it's handled by CKEditor already like Drupal's Core media module.

Closing as a duplicate of 💬 HTML tags showing in Caption Active which has some possible workarounds in the comments.

Closing as a duplicate of 💬 HTML tags showing in Caption Active which has some possible workarounds in the comments.

The patch in #3459169-7: Protect against empty revision timestamps in VersionHistoryController::getRevisionDescription → is a great start, but it still errors when you go to revert to a revision with a missing timestamp, since it tries to put the revision timestamp in the page title.

In the spirit of #3459169-5: Protect against empty revision timestamps in VersionHistoryController::getRevisionDescription → , I've attached a patch with a proposed solution that will fix the issue at the source and prevent the value ever being null:

1. In RevisionLogEntityTrait:getRevisionCreationTime(), we check if revision_created is null.
2. If null, we fallback to changed (credit to @connbi for that idea).
3. If changed is somehow null (I'm not even sure if this would ever happen?), we fall back to 0 which will get rendered as January 1, 1970 (a pretty universally recognizable "missing" timestamp)

If this feels like a good solution, I can start an MR and add tests.

+1 for Option 1. I concur that the original fix in 🐛 [view:total-rows] problem in Display a 'Specified number of items' pager Postponed: needs info was flawed and seems to be tailored to a pretty rare use-case, where a Result Summary with @current_record_count could have simply been used. That change broke all the views on our site that use the pattern described in comment #6 → with a summary "displaying X of X" and a link to view all results. So while this change would break some backwards compatibility, it's also fixing backwards compatibility...

@delacosta456, yup same thing here. Only $view->total_items is stored in the cache and not the new $view->totalItemsBeforePagerLimit added in this patch, so if caching is enabled on the view it won't work.

I believe @tame4tex is onto the right solution here, the original MR that caused this regression should be reverted, and the [view:total-rows] token should be clarified. Patch from Option #1 in https://www.drupal.org/project/drupal/issues/3265798 🐛 [view:total-rows] problem in Display a 'Specified number of items' pager Postponed: needs info should work correctly with caching.

@morvaim: The issue is in my case at least, is that I'm trying to make an attribute more permissive than the CKEditor plugin allows, so it needs to be added to "Manually editable HTML tags".

I have a custom text filter that handles some custom attributes that can be set in data-align. This issue restricts that field to only the values that the CKEditor media plugin allows (left, right, etc). So if I want to allow additional values, I'd have to create a dummy CKEditor plugin even if I don't need to do anything client-side.

I'll note there is an open MR attached to this issue with commits from 9 months ago that has already fixed a lot of the issues from the last several comments/patches. I'd suggest anyone with issues test that MR and contribute back instead of creating separate patches.

This change appears to have caused an interesting regression that breaks Inline Entity Form support when the inline entity form has a conditional date field and the parent node edit form also has conditional fields:

Uncaught PHP Exception TypeError: "Drupal\Component\Utility\NestedArray::getValue(): Argument #2 ($parents) must be of type array, null given, called in /app/web/modules/contrib/conditional_fields/src/ConditionalFieldsFormHelper.php on line 894" at /app/web/core/lib/Drupal/Component/Utility/NestedArray.php line 69

Steps to reproduce:
1. Create new Drupal 10 site with Standard profile
2. Enable modules: inline_entity_form:3.0.0-rc20 and conditional_fields:dev-4.x#962c0a83
3. Add new Content entity reference field to the Article content type: field_pages
4. Set field to unlimited cardinality and limit references to the "Basic page" content type
5. Edit Article form display, set field_pages to Inline Entity Form - Simple widget
6. Add new condition to the Article content type: field_image visible when title has value "test"
7. Add new Date (Date-only) field to the "Basic page" content type: field_date
8. Add new condition to the "Basic page" content type: field_date visible when title has value "test"
9. Go to create a new Article. Scroll to field_pages and add two blank entries. After attempting to add a 3rd entity under field_pages, you will start receiving the error. If you satisfy the condition by setting the title to "test", it lets you add another entity, but if the title is anything else, you will get an error.

Repeating the above steps on the prior commit 25ef661e, it works fine.

Thanks for the tip, I tried a force push and changed the target branch of the MR to 6.3.x, but the MR was showing all of the changes from my fast-forward merge of 6.3.x in the diff for some reason and I couldn't get it to refresh.

Side note, the `WebformElementSignatureTest::testSignature` test is showing as a failure but it appears unrelated as the same thing is happening on other MRs.

Are we still escaping JavaScript if we remove that line?

Good question, it wasn't completely removed but just moved lower down.

So yes, it will still do the XSS filtering, but now after the placeholder title/regex replacements are done. I moved it because I noticed when using the placeholder, it wasn't getting filtered or trimmed.

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

Found an issue with the alternative title logic. Let me take that MR back for a bit.

Howdy neighbor! Appreciate your work as well.

I did some more digging on this, I think #3271576 🐛 Paths to replace with custom breadcrumbs is broken for Fixed (the source of this regression) was actually a "duplicate" bugfix. The same bug was fixed on Feb 3, 2022 in #3262378 → with a different approach in `getRequestForPath` of adding a leading slash if it doesn't exist.

That fix didn't make it into a release (v2.0.3) → until Jun 5, 2022. In the meantime, on Mar 24, 2022, #3271576 🐛 Paths to replace with custom breadcrumbs is broken for Fixed was opened. In July 2022, a couple users in the comments appear to have the same issue, I suspect due to an outdated module version. This bug should be theoretically impossible with v2.0.3, released a month prior.

This ultimately leads to a redundant bugfix being merged and released in v2.0.8. →

Anyway. Sorry for the unprompted history lesson. I submitted an MR that:

1. Adds a note to the help text that "Use the real page title when available" needs to be enabled to use the <title> placeholder. This appears to have been a missed followup from #3271576 🐛 Paths to replace with custom breadcrumbs is broken for Fixed .
2. Reverts the change that caused this regression.
3. Fixes another bug I found during testing where the title placeholder doesn't get trimmed, by doing the trimming and XSS filtering *after* the title/regex replacements are done.
4. Refactors TitleResolver so it takes in a route_match object instead of redundantly re-creating the route from the request URI. Additionally, skips all of the unnecessary entity loading if the "alternative title field" is not enabled. This should be a small performance boost.
5. Adds tests for this issue and #3271576 🐛 Paths to replace with custom breadcrumbs is broken for Fixed to prevent future regressions.

Appears to be caused by https://git.drupalcode.org/project/easy_breadcrumb/-/commit/e4109f3f4851... (issue #3271576). Now instead of trimming leading *and* trailing slashes from $path, it only removes trailing slashes.

This conflicts with line 321 where the $custom_path is trimmed on both sides.

So we run into a scenario where they don't match:

$path: path-1
$custom_path: /path-1

We could make line 321 only trim spaces from both sides, and then add an rtrim below for the slashes so it matches the behavior of $path, but that would be a breaking change if someone has a custom path config with multiple leading slashes. (unlikely, but possible).

@idebr Confirmed your patch fixes the OOM issue and about doubled the performance of a page with a large map. Thanks!

@itamair Would we be able to get the patch in #52 committed? I'd be happy to make an MR for it if that helps.

Open to any feedback on the approach in MR #5 or what needs to be done to get this committed! We have been using this patch in production now for a month and it has provided a considerable speedup to webform submission operations.

We've now been using the fix in MR #469 in production for a month without any errors, and have been able to remove our workarounds. It'd be great if someone else could test and change this issue to RTBC!

MR created with proposed resolution.

@jrockowitz Thanks for the repro config.

I've submitted an MR with tests that fixes this issue. Interestingly, there was already code to handle a similar bug in OptionsBase::getElementSelectorInputValue, from issue #3000202 → . I pulled that code out to an overriden getValue function which should fix the bug for all code paths including OptionsBase::formatTextItem/formatHtmlItem mentioned above, which is the source of the error in this issue.

odensc → created an issue.

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

Also verified this patch works with Drupal 10.2.2 and entity_embed 1.5.

Here's the most straight-forward solution I could come up with (patch attached).

This patch: Passes a boolean to the CKEditor 5 plugin of whether the Drupal core alignment filter is enabled. If it's disabled (i.e. we can safely assume that the `data-align` attribute is unused unless the site has a custom filter enabled), then we don't add data-align to the attribute ignore list so that any custom filters can handle it.

Another option would be to simply rename the attribute to `data-removed-align` or something rather than removing it completely, but that would require the filter to check both attributes and seems more convoluted overall.

Any input appreciated, thanks.

@bnjmnm Thanks for the MR! I've confirmed it fixes the issue described, but I did find some bugs:

1. If a wildcard is defined in the Source editing plugin's allowed tags, e.g. <drupal entity data-*>, then a TypeError: attr.key.replace is not a function is thrown.

2. If one of the default data attributes is defined in the allowed tags, e.g. <drupal-entity data-entity-type data-entity-uuid>, then the media doesn't render after embedding. I believe this is because the default key in the attrs object, e.g. "drupalEntityEntityType", gets superseded by the now-automatically-added "dataEntityType", causing some other part of the code to break.

I've attached a patch to your MR that works around these two issues.

For #2, the fix is easy enough: add a check that the attribute is not already defined in the attrs array.

For #1, it's a bit trickier. It appears CKEditor's SchemaItemDefinition.register method does not accept RegExp objects in the allowAttributes property. That means there's no way to simply pass the wildcard along, AFAICT.

Therefore, I simply added a typecheck to simply skip if attr.key is not a string, but this is quite opaque - I'm not sure if it'd be better to actually throw an error instead, so the user isn't confused if their wildcard is not respected. That is, of course, unless you have a better idea of how to handle wildcards.

Comment #35 is exactly correct. There are two issues at play, there was a data loss issue that was fixed in https://www.drupal.org/project/drupal/issues/3410303 🐛 FilterHtml data loss when iframe and/or textarea is allowed Active , but there still exists a separate issue (fixed/reverted by the patch in comment #38), which is that saving a valid CKEditor "Source editing" configuration in the admin interface will cause a validation error. You can still technically edit it through drush, but it's not ideal.

Thanks @timodwhit! Confirmed this patch fixes the issue on Drupal 10.1.5, PHP 8.1.18, and diff 8.x-1.1.

This is due to a breaking change in Symfony 6 (used by Drupal 10) that no longer allows arrays as the return type of `InputBag::get`. See a similar fix that was made in another area of the Webform module: https://www.drupal.org/project/webform/issues/3335962 🐛 File input value contains a non-scalar value Fixed

This is a pretty high priority regression IMO, it will give an error and fail to render the webform on any link that is using prepopulated arrays in the query string.

@jrockowitz Attached is a repro webform. The difference here is that yours has the global `form_prepopulate` setting enabled, which does not trigger this error, as it follows a different codepath at `WebformSubmissionForm.php:L2651`. Disabling the global setting and enabling prepopulate on the individual form element allows the error to happen.

I ran into this issue too. Doing some debugging with XDebug, it seems the error is coming from Drupal\webform\Plugin\WebformElement\OptionsBase::formatTextItem/formatHtmlItem. The call to getValue is returning an array of ["select" => [...], "other" => "..."] for some reason, but only when called from inside a computed token/computed twig field and when the select element is conditionally visible.

When the select element isn't conditional/when the token is in the confirmation page, it seems to be returning a string (either the option selected, or the value of the Other field), as expected.

I was able to do an ugly workaround by using a computed twig field, by detecting if it's an array and returning an empty string, otherwise computing the token:

{{ data.select_element is iterable ? '' : webform_token('[webform_submission:values:select_element]', webform_submission) }}