Problem/Motivation

FormErrorHandler::displayErrorMessages calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

  • Remove the call by refactoring the code.

Remaining tasks

  1. Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123
  2. Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.
  3. If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.

Manual testing steps (for XSS and double escaping)

Do these steps both with HEAD and with the patch applied:

  1. Clean install of Drupal 8.
  2. Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.
  3. If there is any user or calling code input in the string, submit
    alert('XSS');

    and ensure that it is sanitized.

User interface changes

N/A

API changes

N/A

Files: 
CommentFileSizeAuthor
#68 2550981-3.68.patch14.22 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,445 pass(es). View
#68 67-68-interdiff.txt467 bytesalexpott
#67 2550981-3.67.patch14.18 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#67 63-67-interdiff.txt434 bytesalexpott
#63 2550981-3.63.patch13.75 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,374 pass(es). View
#63 61-63-interdiff.txt1.35 KBalexpott
#61 2550981-28.patch13.75 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,385 pass(es). View
#57 2550981-2.55.patch13.87 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,396 pass(es). View
#57 49-55-interdiff.txt1.95 KBalexpott
#49 2550981-49.patch14.08 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,397 pass(es). View
#49 interdiff-46-49.txt721 bytesstefan.r
#47 2550981.46.patch15 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,393 pass(es), 1 fail(s), and 0 exception(s). View
#47 interdiff-45-46.txt4.1 KBstefan.r
#45 stark.png52.47 KBalexpott
#45 bartik.png69.17 KBalexpott
#45 seven.png48.08 KBalexpott
#45 2550981.45.patch17.47 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,379 pass(es), 8 fail(s), and 0 exception(s). View
#45 35-45-interdiff.txt3 KBalexpott
#39 remove_safemarkup_set-2550981-39.patch14.61 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,125 pass(es). View
#29 Form_error_style_test___Site-Install.png41.1 KBjoelpittet
#29 Form_error_style_test___Site-Install.png37.16 KBjoelpittet
#28 2550981-28.patch13.75 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,950 pass(es). View
#28 interdiff-17-28.txt4.09 KBstefan.r
#24 without.png64.49 KBstefan.r
#24 with.png56.49 KBstefan.r
#19 2550981-17.patch13.28 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,939 pass(es). View
#17 2550981-17.patch4.52 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,629 pass(es), 16 fail(s), and 0 exception(s). View
#14 2550981-9.patch4.52 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Form/FormErrorHandler.php. View
#8 2550981-8.patch4.53 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,619 pass(es), 16 fail(s), and 0 exception(s). View

Comments

josephdpurcell created an issue. See original summary.

josephdpurcell’s picture

Issue summary: View changes
akalata’s picture

akalata’s picture

Status: Active » Postponed
Related issues: +#2505931: Remove SafeMarkup::set in ViewListBuilder

Postponed on #2505931: Remove SafeMarkup::set in ViewListBuilder. Whatever pattern we find works there can most likely be used here as well.

kgoel’s picture

Status: Postponed » Active

Per @xjm - this is actionable issue now.

kgoel’s picture

Status: Active » Postponed

This is wrong one and still postponed.

xjm’s picture

#2505497: Support render arrays for drupal_set_message() would have helped with this, but that issue is in no danger of being solved and is probably going to be wontfixed.

So one option here is to do a renderPlain() on a render array with the items before returning to the dsm().

stefan.r’s picture

FileSize
4.53 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,619 pass(es), 16 fail(s), and 0 exception(s). View

This may work once #2505931-170: Remove SafeMarkup::set in ViewListBuilder is in, just needs unit tests.

Wim Leers’s picture

Status: Postponed » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine for me. Did you checked whether we have a test for that?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2550981-8.patch, failed testing.

stefan.r’s picture

Let's manually test?

stefan.r’s picture

Also, we need to update the unit test

stefan.r’s picture

Issue tags: +Needs tests, +Needs manual testing
FileSize
4.52 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Form/FormErrorHandler.php. View

Marking needs tests as we still need to do the unit tests

Wim Leers’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: 2550981-9.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
4.52 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,629 pass(es), 16 fail(s), and 0 exception(s). View

Let's fix that fatal error

Status: Needs review » Needs work

The last submitted patch, 17: 2550981-17.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
13.28 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,939 pass(es). View
stefan.r’s picture

The 3 spaces after the string "@count errors have been found:" are a bit ugly but let's see if this gets us closer to green, would rather not change the template in this issue.

stefan.r’s picture

Title: Remove or document SafeMarkup::set in FormErrorHandler::displayErrorMessages » Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages
Issue summary: View changes
Wim Leers’s picture

Title: Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages » Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages()

Looks great. I'd RTBC, but I don't know how to manually test this?

+++ b/core/lib/Drupal/Core/Form/FormErrorHandler.php
@@ -24,16 +24,26 @@ class FormErrorHandler implements FormErrorHandlerInterface {
+   * The Renderer service.
...
+   *   The Renderer service.

Nit: s/Renderer/renderer/

stefan.r’s picture

Trigger several errors on a form, or alternatively download and install https://github.com/dmsmidt/errorstyle and go to /error-style/form

stefan.r’s picture

Issue summary: View changes
Issue tags: -Needs manual testing
FileSize
56.49 KB
64.49 KB

Tested manually and this looks OK. The only thing is the inline comma list will still need some CSS love but that's a general problem, not sure if that's in scope here?

With patch:

Without patch:

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks @stefan.r and @Wim Leers!

  1. +++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
    @@ -589,13 +589,13 @@ function testDatelistWidget() {
    -      [['year' => 2012, 'month' => '', 'day' => '', 'hour' => '', 'minute' => ''], '4 errors have been found: Month, Day, Hour, Minute'],
    +      [['year' => 2012, 'month' => '', 'day' => '', 'hour' => '', 'minute' => ''], '4 errors have been found:   MonthDayHourMinute'],
    

    Ah, the weird lack-of-spaces in DateTimeFieldTest is a quirk of assertText() and the way the data provider is used.

  2. +++ b/core/modules/shortcut/src/Tests/ShortcutSetsTest.php
    @@ -132,7 +132,7 @@ function testShortcutSetSwitchCreate() {
    -    $this->assertRaw('1 error has been found: <a href="#edit-label">Label</a>');
    +    $this->assertText('1 error has been found:   Label');
    
    +++ b/core/modules/system/src/Tests/Form/ValidationTest.php
    @@ -289,10 +289,10 @@ protected function assertErrorMessages($messages) {
    -      $error_links[] = \Drupal::l($message['title'], Url::fromRoute('<none>', [], ['fragment' => 'edit-' . str_replace('_', '-', $message['key']), 'external' => TRUE]));
    +      $error_links[] = $message['title'];
    ...
    -    $this->assertRaw($top_message . ' ' . implode(', ', $error_links));
    +    $this->assertText($top_message . '   ' . implode('', $error_links));
    
    +++ b/core/modules/user/src/Tests/UserBlocksTest.php
    @@ -48,7 +48,7 @@ function testUserLoginBlock() {
    -    $this->assertRaw('1 error has been found: <a href="#edit-name">Username</a>');
    +    $this->assertText('1 error has been found:   Username');
    

    So these hunks are actually removing a bit of the test coverage. In HEAD, they assert both that the error is found and that there is a correct jump link for it (see #1493324: Inline form errors for accessibility and UX).

    So, instead of doing assertRaw() on the whole thing, to avoid unnecessarily testing the template markup, I'd suggest that we split this into separate assertions: one for the error count text, and one for the links to the fragments.

    We could also use that strategy to avoid the weirdness with the spacing in the DateTime test.

  3. +++ b/core/tests/Drupal/Tests/Core/Form/FormErrorHandlerTest.php
    @@ -39,9 +40,6 @@ public function testDisplayErrorMessages() {
    -    $form_error_handler->expects($this->at(3))
    -      ->method('drupalSetMessage')
    -      ->with('3 errors have been found: Test 1, Test 2 &amp; a half, Test 3', 'error');
    

    This is also removing test coverage.

xjm’s picture

Forgot to say, thanks also for the manual testing -- I think the before-and-after screenshots look okay.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
4.09 KB
13.75 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,950 pass(es). View
joelpittet’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs followup
FileSize
37.16 KB
41.1 KB

Before

After

The CSS can be improved in a follow-up.

joelpittet’s picture

We did that loop link checking strategy earlier in this patch, so it just came back around into the patch:)

joelpittet’s picture

Issue tags: -Needs followup
alexpott’s picture

I think we should address the visual regression in the issue that introduces it - no?

catch’s picture

Status: Reviewed & tested by the community » Needs work

Yes I think that's worth fixing either here, or postponing on #2557367: Fix inline list CSS if it needs more discussion.

stefan.r’s picture

The problem here is that a) the div will need a display: inline-block; if it's inside messages__list and b) the margin from this style:

.item-list ul {
    list-style-type: disc;
    list-style-image: none;
    margin: 0.25em 0px 0.25em 1.5em;
}

overrides this one:

ul.item-list__comma-list, ul.item-list__comma-list li, [dir="rtl"] ul.item-list__comma-list, [dir="rtl"] ul.item-list__comma-list li {
    margin: 0px;
    padding: 0px;
}

Should we just fix this with a !important after the 0px in the margin?

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.92 KB
16.67 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,103 pass(es), 24 fail(s), and 6 exception(s). View

The patch attached fixes the issue - it removes the indent and makes comma lists properly inline in all themes.

Status: Needs review » Needs work

The last submitted patch, 35: 2550981.35.patch, failed testing.

davidhernandez’s picture

+++ b/core/modules/system/css/components/item-list.theme.css
@@ -17,17 +17,23 @@
+.item-list ul.item-list__comma-list {

You can't do that. The item-list wrapper doesn't exist in the core template.

davidhernandez’s picture

We're just looking at the form errors list correct? I'll take a look.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
14.61 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,125 pass(es). View
877 bytes
44.55 KB
52.12 KB
50.75 KB
51.89 KB
43.65 KB
56.09 KB
47.71 KB
46.76 KB
44.09 KB
51.62 KB
50.24 KB
46.49 KB
50.83 KB
47.02 KB

Grr, these things would be easier if we can get that CSS moved to Classy.

I didn't make screenshots for all the RTL but I did look. As usual, someone manually test and doublecheck that it's ok.

p.s. multiple file upload would be really nice to have.

davidhernandez’s picture

p.p.s I know adding it to messages contradicts my comment in #37, but it's the simplest fix without making out of scope changes. It should be fine if/when we move that file to Classy, which we have an open issue to do.

davidhernandez’s picture

No margin should be needed LTR or RTL because displaying the comma list inline should get the natural spacing of the text and line height.

Joel, how did you make the "error not related to a real element"? I did not test that. I imagine it would still be ok since the spacing is there in the regression. It should be on the wrapper, not the comma list.

joelpittet’s picture

+++ b/core/themes/seven/css/components/menus-and-lists.css
@@ -38,6 +38,7 @@ ul.inline li {
+[dir="rtl"] .item-list .item-list__comma-list{

Needs a space there, but thanks @davidhernandez, that is a bit of a more direct change I had in mind. also +1 to multiple uploads, almost always adding a patch + interdiff, if not crap tone of screenshots;)

A bit of why I opened a follow-up was to avoid bloating the core of this commit/patch with fixing problems that were not part of the IS and weren't a direct cause of this issue.

joelpittet’s picture

@davidhernandez used the module mentioned in #23 for the test.

joelpittet’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
40.84 KB

Other than that space mentioned in #42 that can be fixed on commit. I think this is good to go.

Here's a new screenshot set.

Before

After

If this get's kicked back on the CSS one more time, please commit #28 and we can move the CSS to the follow-up.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3 KB
17.47 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,379 pass(es), 8 fail(s), and 0 exception(s). View
48.08 KB
69.17 KB
52.47 KB

The fix in #39 is not correct either. The comma list could be used in a success message or any other item list. We need a general fix because of the general styling of lists within lists.

You can't do that. The item-list wrapper doesn't exist in the core template.

I actually think this is wrong. The core styling that is causing problems is

.item-list ul {
  margin: 0 0 0.75em 0;
  padding: 0;
}

so core already has the concept of ul's in item-list classed elements. The answer is to remove this styling and add it to classy where the .item-list wrapper is added and then handle comma lists inside lists there.

Status: Needs review » Needs work

The last submitted patch, 45: 2550981.45.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
4.1 KB
15 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,393 pass(es), 1 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 47: 2550981.46.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
721 bytes
14.08 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,397 pass(es). View
stefan.r’s picture

The CSS/template changes in #45 look good and 48/49 just did a s/ / / on the error messages to fix tests.

Screenshots from manual testing in #45 look good and did one last manual test on the last patch to confirm (including on the comma-separated item list in the views listing).

The visual regression seems fixed, so this looks ready to me if tests are green now, considering how trivial the last 2 interdiffs are.

davidhernandez’s picture

The answer is to remove this styling and add it to classy...

Yes, but I agree with Joel that moving all the CSS around is too much to roll into this issue. We can fix that with the plan to move all the CSS.

The comma list could be used in a success message or any other item list.

I targeted the error messages specifically, but that can be changes to just .messages. However, I'm don't think that is correct. All item lists are not inline, this is really only a problem with the comma list, in conjunction with the error message which has the inline label. Other item lists are block displayed uls, so that line .messages--error .item-list has no affect which is fine.

edit: made a typo that hid everything.

alexpott’s picture

@davidhernandez but the comma list needs to be displayed inline regardless of where is it. And I think the changes to the tests in #45 and #47 show that current approach is correct.

dawehner’s picture

+++ b/core/modules/system/css/components/item-list.theme.css
@@ -2,25 +2,12 @@
+ul.item-list__comma-list,
+.item-list ul.item-list__comma-list {
...
+ul.item-list__comma-list li,
+.item-list ul.item-list__comma-list li {

I'm confused, isn't the selector the same twice?

Wim Leers’s picture

  1. +++ b/core/lib/Drupal/Core/Form/FormErrorHandler.php
    @@ -24,16 +24,26 @@ class FormErrorHandler implements FormErrorHandlerInterface {
    +   *   The Renderer service.
    

    Nit: s/Renderer/renderer/.

  2. +++ b/core/tests/Drupal/Tests/Core/Form/FormErrorHandlerTest.php
    @@ -41,7 +42,14 @@ public function testDisplayErrorMessages() {
    +      }));
    +
    

    Nit: Extraneous newline added here.

  3. +++ b/core/themes/classy/classy.libraries.yml
    @@ -3,6 +3,9 @@ base:
    +      # We can not attach this in the item-list.html.twig template since it is
    +      # often used during RendererInterface::renderPlain().
    +      css/dataset/item.list.css: {}
    

    And why is that a problem? Attachments are simply ignored/discarded when using renderPlain().

  4. +++ b/core/themes/classy/classy.libraries.yml
    --- /dev/null
    +++ b/core/themes/classy/css/dataset/item.list.css
    

    What's this "dataset" subdirectory?

xjm’s picture

To clarify #52, this is additional CSS that probably should have been introduced in #2505931: Remove SafeMarkup::set in ViewListBuilder with the original comma-separated list stuff, and we only missed it in that case because the list was on a line by itself. So it makes sense to add it here, though I'd also be okay with a general followup for it. But I agree with @alexpott that we shouldn't add the CSS only for messages.

Nice work on the manual testing!

davidhernandez’s picture

The comma list is already inline. The issue is the wrapper only because in the context of the error messages that text label is there outside the wrapper. I'm saying I agree there there needs to be a wrapper class for it, that is the root cause (and I new that in the views list builder issue,) but doing so requires making all those changes. Those changes then have to be reviewed outside the context of just the form errors. So if you really want to go that route we have to look at more than form errors.

alexpott’s picture

FileSize
1.95 KB
13.87 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,396 pass(es). View

@dawehner good point! All the .item-list blah belongs in classy.
@Wim Leers
1. Fixed
2. Fixed
3. Yes but we need to add item.list.css when rendering a comma separated list... so doing {{ attach_library('classy/item.list') }} in the item-list.html.twig does not work.
4. The dataset directory is just to match the template's directory... core/themes/classy/templates/dataset/item-list.html.twig

alexpott’s picture

#56 the wrapper is an issue everywhere in classy - it just so happens that so far we have only used comma lists in table columns so we had not noticed.

Wim Leers’s picture

#57.3: aahhh! So this is basically a case where a message for drupal_set_message() does need to bubble attachments. That was not at all clear to me on my first reading.

+++ b/core/themes/classy/classy.libraries.yml
--- /dev/null
+++ b/core/themes/classy/css/dataset/item.list.css

Shouldn't this be item-list.css instead of item.list.css?

stefan.r’s picture

Rolling a new patch on the assumption the answer to that question is yes as it's consistent with the naming of the other CSS files (seems we use dots for sub-elements and dashes for words).

alexpott’s picture

FileSize
13.75 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,385 pass(es). View

Given the concerns @davidhernandez, @joelpittet and @xjm (in conversation) are both pointing that the css changes could possibly have a bigger impact.

Uploading #28 again.

xjm’s picture

So the suggestion I made to @alexpott is that we temporarily let the small visual regression for menus messages in and then fix and test .item-list more broadly in #2557367: Fix inline list CSS since the scope of that is much wider; he'll add the additional work from this issue over there.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.35 KB
13.75 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,374 pass(es). View

Let's fix @Wim Leers feedback in #54 that applied to #28 too.

Also considering that I've reviewed this patch extensively whilst trying to resolve the css issues and did contribute before #28 I feel okay to rtbc this. Even though the changes to FileFieldValidateTest make my eyes bleed.

Let's be done with SafeMarkup::set()

Wim Leers’s picture

#63++

davidhernandez’s picture

I fixed those regressions in #39. It's a small change and I don't see why it's an issue if you think it should all be refactored later anyway. I am not confident in any follows getting done, and I don't think we should live with such an obvious visual regression, especially the indent, that we may get stuck with in 8.0.0.

xjm’s picture

@davidhernandez There is a patch in #2557367: Fix inline list CSS already; hopefully that issue can go forward easily without blocking the security-related critical?

+++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
@@ -589,13 +589,13 @@ function testDatelistWidget() {
-      [['year' => 2012, 'month' => '', 'day' => '', 'hour' => '', 'minute' => ''], '4 errors have been found: Month, Day, Hour, Minute'],
+      [['year' => 2012, 'month' => '', 'day' => '', 'hour' => '', 'minute' => ''], '4 errors have been found:   MonthDayHourMinute'],
       // Year and Month selected, validation error on Day, Hour, Minute.
-      [['year' => 2012, 'month' => '12', 'day' => '', 'hour' => '', 'minute' => ''], '3 errors have been found: Day, Hour, Minute'],
+      [['year' => 2012, 'month' => '12', 'day' => '', 'hour' => '', 'minute' => ''], '3 errors have been found:   DayHourMinute'],
       // Year, Month and Day selected, validation error on Hour, Minute.
-      [['year' => 2012, 'month' => '12', 'day' => '31', 'hour' => '', 'minute' => ''], '2 errors have been found: Hour, Minute'],
+      [['year' => 2012, 'month' => '12', 'day' => '31', 'hour' => '', 'minute' => ''], '2 errors have been found:   HourMinute'],
       // Year, Month, Day and Hour selected, validation error on Minute only.
-      [['year' => 2012, 'month' => '12', 'day' => '31', 'hour' => '0', 'minute' => ''], '1 error has been found: Minute'],
+      [['year' => 2012, 'month' => '12', 'day' => '31', 'hour' => '0', 'minute' => ''], '1 error has been found:   Minute'],

+++ b/core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php
@@ -275,7 +275,7 @@ public function testFieldAdminHandler() {
-    $this->assertText(t('1 error has been found: Test Entity Reference Field'), 'Node save failed when required entity reference field was not correctly filled.');
+    $this->assertText(t('1 error has been found:   Test Entity Reference Field'), 'Node save failed when required entity reference field was not correctly filled.');

@@ -286,7 +286,7 @@ public function testFieldAdminHandler() {
-    $this->assertText(t('1 error has been found: Test Entity Reference Field'));
+    $this->assertText(t('1 error has been found:   Test Entity Reference Field'));

+++ b/core/modules/file/src/Tests/FileFieldValidateTest.php
@@ -35,7 +35,7 @@ function testRequired() {
-    $this->assertText('1 error has been found: ' . $field->label(), 'Node save failed when required file field was empty.');
+    $this->assertText('1 error has been found:   ' . $field->label(), 'Node save failed when required file field was empty.');

@@ -57,7 +57,7 @@ function testRequired() {
-    $this->assertText('1 error has been found: '  . $field->label(), 'Node save failed when required multiple value file field was empty.');
+    $this->assertText('1 error has been found:   '  . $field->label(), 'Node save failed when required multiple value file field was empty.');

@alexpott and I discussed whether to change these assertions in this issue to do multiple assertText(), but the problem with that is that that also reduces the test coverage. Ideally these tests should check for the jump links like the others do, but it makes sense to do that in a followup as well.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
434 bytes
14.18 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Discussed at length with @davidhernandez and @xjm in IRC. @davidhernandez suggested just fixing the indentation in seven. That seems a good compromise.

alexpott’s picture

FileSize
467 bytes
14.22 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,445 pass(es). View

Missed a bit. Thanks @davidhernandez.

davidhernandez’s picture

I'm fine with that in #68. I'm not RTBCing because I don't know anything about the rest of the patch.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

The last submitted patch, 67: 2550981-3.67.patch, failed testing.

xjm’s picture

That works for me for now too.

Thanks everyone and see you in #2557367: Fix inline list CSS! Committed and pushed to 8.0.x.

  • xjm committed 248506e on 8.0.x
    Issue #2550981 by alexpott, stefan.r, davidhernandez, joelpittet, Wim...
cilefen’s picture

Should this be marked "Fixed"?

xjm’s picture

Status: Reviewed & tested by the community » Fixed

Yes. :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.