Invalid markup in ImageItem (causes test failures with symfony/dom-crawler:4.3+)

I think the #field_prefix and #field_suffix parts in ImageItem::fieldSettingsForm() and editor_image_upload_settings_form() are simply a misusage of Form API, and should be completely fine to be fixed in a minor release.

Here's a fix for them and also for StyleTableTest::testAccessibilitySettings().

+++ b/core/modules/editor/editor.admin.inc
@@ -93,8 +93,6 @@ function editor_image_upload_settings_form(Editor $editor) {
-    '#field_prefix' => '<div class="container-inline clearfix">',

@@ -110,6 +108,7 @@ function editor_image_upload_settings_form(Editor $editor) {
+    '#prefix' => '<div class="form--inline clearfix">',

Nice.

form--inline was added in #2333719: Abstract Views Exposed Form styling out into a reusable class in 2015. This code was originally added in #1932652: Add image uploading to WYSIWYGs through editor.module in 2013.

I manually tested this and it continues to look the same:

Since all changes are in forms, and then even admin forms, and no form structure changes, only some styling-related markup changes, I am confident this change is acceptable, at least in Drupal 8.8.

This clean-up is great but it needs CR about element

Can you elaborate on why you think this needs a change record, @andypost?

Build arrays are not a part of API but affects every custom(probably contrib) fields which extends core field type for image

+++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
@@ -209,8 +209,6 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
-      '#field_prefix' => '<div class="container-inline">',
-      '#field_suffix' => '</div>',

@@ -220,6 +218,7 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
       '#field_suffix' => ' × ',
+      '#prefix' => '<div class="form--inline clearfix">',

@@ -228,6 +227,7 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
       '#field_suffix' => ' ' . t('pixels'),
+      '#suffix' => '</div>',

@@ -236,8 +236,6 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
-      '#field_prefix' => '<div class="container-inline">',
-      '#field_suffix' => '</div>',

@@ -247,6 +245,7 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
+      '#prefix' => '<div class="form--inline clearfix">',

@@ -255,6 +254,7 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
+      '#suffix' => '</div>',

this formally not BC break but good to know... it's hard to catch if no regression testing

@Wim Leers: In my understanding, despite being just admin forms, there still may be people form altering for field prefix / suffix and now will not override what they used to override. So I think that is where @andypost is coming from. Better be safe than sorry :D

Thanks everyone!

In this case I think we can forego the CR since the markup change is purely removing broken markup from a render array in one element (without any changes required to templates etc.) It's sort of on the edge, but these changes are allowed in minors and typically don't need a CR. According to the frontend BC policy, this type of change is OK in a minor release.

That said, it's a bit too disruptive to backport. Therefore, @mikelutz is going to file a followup issue for 8.7 to add a conflict for dom-crawler 4.3 so that sites don't accidentally update to a library that will cause breakage.

And it's here: #3074039: Add a composer conflict for symfony/dom-crawler >=4 to 8.7 only.

This looks straightforward. Thanks also for the manual testing.

The one question I had about this is that the opening tag is now attached to one element as the prefix, and the closing tag is attached to the next element as a suffix, without any structure/nesting and so just relying on the default order of the two render elements. It's not a big deal since these essentially form one conceptual unit but it does cause psychological tension.

While we could change the render array to get around this (e.g. with #type => 'html_tag' or such), @mikelutz, @samuel.mortenson, and I discussed the issue and agreed that would actually be more disruptive. I thought of looking for other places in core where an opening tag and a closing tag are attached to sibling elements but that's difficult to grep for... and after all, markup that might be broken under theoretical alters is less bad than markup that's 100% broken now.

I think put more words into thinking about this than are in the patch. ;) Anyway, this seems alright, so committed to 8.8.x.

Thanks!

but it does cause psychological tension.

https://twitter.com/wimleers/status/1160569258256011264 😁

Per a Slack discussion with Gábor Hojtsy regarding usage of D9 tags (Drupal 9, Drupal 9 compatibility, Drupal 9 readiness, etc.), "Drupal 9 compatibility" should be used for contributed projects that need updating and "Drupal 9" was the old tag for D8 issues before the D9 branch was ready. Doing tag cleanup here based on that discussion.

Closed a duplicate issue #2667340: Usage of field_prefix and container-inline creates invalid markup..