Add assertion helpers to AssertContentTrait for disabled/enabled fields

A) I thought we stopped wrapping test assertions in t().

B) Seems like the text assertion for assertNoFieldDisabled() should say "Field @name is enabled" (not "disabled").

Otherwise, +1. Seems a lot better to have these assertions available to anyone who wants them than to duplicate them all over the place.

Thanks!
-Derek

Re: t() -- you're probably right. I haven't been following that closely.

Re: "not disabled". Ugh. The double negative is killing me. ;) Can't we go with assertFieldEnabled() and "Field @name is enabled"? I think it'd be a lot easier to use correctly (and debug) this way.

Please excuse me for butting in. I just wrote this because I needed it and wanted to contribute it, and then I found this issue.

I modeled my version closely after assertFieldChecked(), except I reversed the logic and implemented assertFieldEnabled() rather than assertFieldDisabled(). This takes care dww's comments.

The other significant difference to mikey_p's patch in #2 is that I'm using id rather than name, as does the existing assertFieldChecked().

Rerolled against HEAD.

+1 to #6. That's what I meant at #3. I think it's safe to break the "No" convention for this, since (en|dis)abled are so obvious and self-documenting.

Ok, assertFieldEnabled() and assertFieldDisabled(), and re-rolled.

The word 'Fields' is a bit confusing as it is used in many different ways throughout Drupal.

It looks like you want to check whether Form Elements are enabled/disabled. If so, how about we call them assertFormElementEnabled() and assertFormElementDisabled()?

We already use "Field" everywhere for (form) elements throughout DrupalWebTestCase.

However, we also already know that selecting fields by HTML ID is poor, because most of our HTML IDs are autogenerated. When dealing with form elements, assertion functions should always be based on the "name" attribute, since that is truly unique and won't change all of a sudden.

Overall, however, I don't find these assertion helpers really helpful.

assertFieldAttributesByName()
assertNoFieldAttributesByName()

i.e., not limited to "disabled", would be much more helpful.

Given how important the enabled/disabled case is for testing access control, I think having these two special cases and shortcuts makes good sense and more readable code in the tests too.

Let's resurrect this.

> The word 'Fields' is a bit confusing as it is used in many different ways throughout Drupal.

True, but all the other form element-related assertions use 'Field' too: assertNoFieldById(), assertFieldChecked(), etc. I think it's better to be consistent within the assertion method names.

> Well I was going write some test for this, but I don't see any places where all the assertions in simpletest are checked.

Same here. WebTestBaseTest has testAssertFieldByName(), but doesn't test any of the other AssertField*() assertions.

Here's a reroll of the most recent patch.

It would be great if we don't introduce a method which supports IDs rather than names, given that IDs might vanish or be more or less random now.

Just a nit.

+++ b/core/modules/simpletest/src/AssertContentTrait.php
@@ -1173,6 +1173,44 @@ protected function assertNoFieldById($id, $value = '', $message = '', $group = '
+    return $this->assertTrue(isset($elements[0]) && empty($elements[0]['disabled']), $message ? $message : t('Field @id is enabled.', array('@id' => $id)), t('Browser'));
...
+    return $this->assertTrue(isset($elements[0]) && !empty($elements[0]['disabled']), $message ? $message : t('Field @id is disabled.', array('@id' => $id)), t('Browser'));

s/@id/%id

Thanks for the reviews. I've made those changes, and also changed to use constructFieldXpath() rather than make my own xpath expression.

Thanks a ton!

BTW primary credit should go to salvis, I just did rerolls and tweaks.

+++ b/core/modules/simpletest/src/AssertContentTrait.php
@@ -1173,6 +1173,44 @@ protected function assertNoFieldById($id, $value = '', $message = '', $group = '
+   *   messages: use \Drupal\Component\Utility\SafeMarkup::format() to embed
...
+   *   messages: use \Drupal\Component\Utility\SafeMarkup::format() to embed

SafeMarkup::format() is deprecated...

   * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.
   *   Use \Drupal\Component\Render\FormattableMarkup.

I'm not entirely convinced we should be adding yet more API surface because:

• There are no usages in core so we're adding unused and untested methods
• we're (slowly) moving BrowserTestBase

Thank you for picking this up, joachim!

SafeMarkup::format() is deprecated...

As long as the methods above and below recommend SafeMarkup::format() there's no point in diverging here.

Replacing references to SafeMarkup::format() should be a separate issue, at least in file scope or better for all of core.

I'm not entirely convinced we should be adding yet more API surface because:

There are no usages in core so we're adding unused and untested methods

If core has no disabled controls, that's nice, but in the real world you cannot do without them, and if you want to test the state, you simply need these methods.

we're (slowly) moving BrowserTestBase

I've given up five and a half years ago and just keep hacking core...

I think the RTBC still stands.

I'm not entirely convinced we should be adding yet more API surface because:

+1

But more importantly: nothing is using this! I think we should have at least a few tests actually using this, no?

> I think we should have at least a few tests actually using this, no?

Agreed.

> But more importantly: nothing is using this!

That's just because core doesn't have any forms that are complex enough. Lots of contrib does.

No changes, just re-rolling patch for 8.4.x

+++ b/core/modules/simpletest/src/AssertContentTrait.php
@@ -1168,6 +1168,44 @@ protected function assertNoFieldById($id, $value = '', $message = '', $group = '
+    return $this->assertTrue(isset($elements[0]) && empty($elements[0]['disabled']), $message ? $message : t('Field %name is enabled.', array('%name' => $name)), t('Browser'));
...
+    return $this->assertTrue(isset($elements[0]) && !empty($elements[0]['disabled']), $message ? $message : t('Field %name is disabled.', array('%name' => $name)), t('Browser'));

Should these actually return anything? AFAICT, assertFoo() methods in PHPUnit don't return anything.

Given that Simpletest is now deprecated, I am inclined to mark this as WONTFIX.

PHPUnit browser tests have this functionality already, provided by Mink: https://api.drupal.org/api/drupal/core%21tests%21Drupal%21Tests%21WebAss...

@joachim
Thank you for following up here. I totally agree, we will not enhance simpletest anymore.

Outdated, rather than fixed, surely?