Problem

To ensure that all form elements are accessible, we need a mechanism to ensure that #title is set up for all the form elements that should use it, so that defining that form element without using #title is not possible.

Proposed resolution

Instead of validating only #title in form_builder(), it has been proposed to create an abstracted "Property Validation API" that can be used to verify that all the required properties of a certain element have been set properly.

This patch will add the "property validation API" and will implement it for the #title property.

Remaining tasks

Original report

In #882694: Add missing form element titles for accessibility @Sun suggested that a mechanism be put in place to ensure that all form fields have a #title.

the proper fix for D8 is to additionally require a non-empty #title for all form elements via form_builder(), throwing an exception or error message if one without is encountered, so as to ensure that every form is accessible.

EDIT: That is, because Form API is not able to throw a form validation error for a form element without #title. You simply see the same form, without any error message.(http://drupal.org/node/882694#comment-3513452)

Files: 
CommentFileSizeAuthor
#134 933004-134.patch11.39 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] 58,707 pass(es), 44 fail(s), and 1 exception(s).
[ View ]
#127 form-933004-127-do-not-test.patch17.42 KBtim.plunkett
#117 form_property_validation-117.patch68.37 KBfalcon03
PASSED: [[SimpleTest]]: [MySQL] 58,315 pass(es).
[ View ]
#116 form_property_validation-933004-116.patch68.21 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,477 pass(es).
[ View ]
#116 interdiff.txt726 bytestim.plunkett
#114 form_property_validation-114.patch67.84 KBfalcon03
FAILED: [[SimpleTest]]: [MySQL] 58,262 pass(es), 0 fail(s), and 72 exception(s).
[ View ]
#112 admin_people.zip567.56 KBfalcon03
#109 form-933004-109.patch67.53 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,475 pass(es).
[ View ]
#104 form-933004-104.patch68.27 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form-933004-104.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#99 form-933004-99.patch75.31 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,199 pass(es).
[ View ]
#99 interdiff.txt3.52 KBtim.plunkett
#93 form-933004-93.patch75.18 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,805 pass(es).
[ View ]
#93 interdiff.txt1.87 KBtim.plunkett
#91 form-933004-91.patch74.02 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,464 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#91 interdiff.txt10.86 KBtim.plunkett
#89 form-933004-89.patch64.46 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,335 pass(es), 0 fail(s), and 90 exception(s).
[ View ]
#89 interdiff.txt16.36 KBtim.plunkett
#87 form-933004-87.patch52.3 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,763 pass(es), 0 fail(s), and 326 exception(s).
[ View ]
#87 interdiff.txt17.75 KBtim.plunkett
#85 form-933004-85.patch36.61 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,076 pass(es), 1 fail(s), and 816 exception(s).
[ View ]
#85 interdiff.txt14.59 KBtim.plunkett
#83 form-933004-83.patch20.53 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,256 pass(es), 0 fail(s), and 5,142 exception(s).
[ View ]
#83 interdiff.txt11.5 KBtim.plunkett
#78 property-validate-933004-78.patch19.23 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,893 pass(es), 1 fail(s), and 5,254 exception(s).
[ View ]
#78 interdiff.txt10.81 KBtim.plunkett
#76 property-validate-933004-76.patch18.28 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,108 pass(es), 1,755 fail(s), and 210 exception(s).
[ View ]
#76 interdiff.txt664 bytestim.plunkett
#74 form_property_validation-74.patch17.77 KBfalcon03
FAILED: [[SimpleTest]]: [MySQL] 51,127 pass(es), 3,468 fail(s), and 455 exception(s).
[ View ]
#64 form_property_validate-64.patch13.26 KBfalcon03
FAILED: [[SimpleTest]]: [MySQL] 50,455 pass(es), 3,819 fail(s), and 495 exception(s).
[ View ]
#62 form_property_validate-62.patch12.52 KBfalcon03
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views_ui/lib/Drupal/views_ui/ViewAddFormController.php.
[ View ]
#59 form_property_validation-58.patch10.38 KBfalcon03
FAILED: [[SimpleTest]]: [MySQL] 50,393 pass(es), 3,840 fail(s), and 498 exception(s).
[ View ]
#51 form_property_validate_abstracted.patch10.02 KBfalcon03
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#29 drupal_933004_29.patch3.61 KBXano
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#27 drupal_933004_27.patch3.07 KBXano
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/form.inc.
[ View ]
#25 drupal_933004_25.patch1.07 KBXano
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/form.inc.
[ View ]
#22 Require-title-933004-22.patch936 bytesmgifford
FAILED: [[SimpleTest]]: [MySQL] 48,405 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#21 Require-title-933004-21.patch908 bytesmgifford
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to clear checkout directory.
[ View ]
#12 Require-title-933004-12.patch908 bytesmgifford
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Comments

Issue tags:+accessibility

Tagging

+1

+1

Would the best approach be to do this in form_builder() and to set a drupal error message, or to do this as a simpletest?

Form_builder will catch all form elements, be they core or contrib. Simple test is less obtrusive, as it will not throw errors in the face of developers of contrib modules with forms.

So, the question is, do we want a test for core, or a error message to educate developers and force them to add #title to all form elements, be they in core or contrib?

It would be good to have an error message or watchdog call set into form_builder, even if it gets taken out at release.

Everything in core's going to need to be run through SimpleTest before it gets in too, but I'm worried that there may just be too many to tackle with all of the initiatives.

Education is always good! But simpletest is also going to do that for core. I suspect that might be sufficient for serious modules in contrib too. Although I'm not certain how simpletest will do this.

Bringing it into Coder is also a good way for those who want to see the best practices get implemented.

Currently the Form API doesn't validate the arrays that it's passed, and so adding something that did would most likely be a big performance hit when, ideally forms should be valid because the module author will have added the correct code when writing the module.

So, I think that this issue is actually one of documentation and education, enforced by a coder module rule.

We should document that form element titles are required and why, and then have a coder module rule so that module authors can be notified when they don't comply.

Requiring #title seems a bit drastic. For example a form element that uses markup to add addition explanatory text should not need a title

@nevets

To ensure that all form fields have a programmatically determinable name (WCAG 2.0 success criteria 4.1.2) we need each form element to have a title. This title can be visually hidden with the system class .element-invisible. See the #title_display property in fapi for more information about visually hiding form element titles.

@Steven Jones

Actually FAPI validates things all over the place. As a quick example form_builder() validates that each $element has a #id property set, and if it does not it sets the #id. So, although there might be other reasons for not requiring #title, performance isn't one of them.

Issue tags:+FAPI

Just adding a FAPI tag.

Proposed Approach

Add validation into form_builder() and throw up a drupal_set_message error on any form that has a missing #title. We discussed watchdog errors, coder module, and simpletests, but none of those are likely to get noticed by most developers making forms with FAPI. So the thought here is that if we really want to make this useful, we should put up an on-screen error.

Of all the available FAPI field types, all of them need a #title except the following: tableselect, vertical_tabs, actions, button, container, image_button, submit, form, hidden, token, markup, item, value.

This approach should be easy to program, because it is just adding an IF statement inside form_builder() to look for #title and throw an error.

This was the consensus as discussed by @Everett Zufelt, @mgifford, and @bowersox at the Drupal Accessibility Sprint.

Status:Active» Needs review
StatusFileSize
new908 bytes
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

This is still pretty rough, but wanted to put the rough idea up as a patch.

Status:Needs review» Needs work
Issue tags:-FAPI, -accessibility

The last submitted patch, Require-title-933004-12.patch, failed testing.

Status:Needs work» Needs review

#12: Require-title-933004-12.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+FAPI, +accessibility

The last submitted patch, Require-title-933004-12.patch, failed testing.

Status:Needs work» Needs review

This approach is too hardcoded:
1) Form API should not be written to handle specific elements, but specific, abstract features. Also, does the title requirement really apply to form elements only, or to a subset of renderable elements (which do not necessarily have to be processed by FAPI).
2) Forms can contain *any* renderable element and not just form elements. As it happens, renderable elements do not necessarily have to have a human-readable title.

If we really want to make the renderable element API check this, a better approach would be to use a process callback that is applied for all elements that require a title. This would keep FAPI clean of element-specific behavior and allow the 'designers' of a specific renderable element to configure its behavior.

Status:Needs review» Needs work

@Xano

If I understand correctly, you are suggesting that each render element that "requires" a #title have a process callback added by default. The process callback receives the element and tests to see if #title is set. Future elements, or contrib elements, could also set a process callback to add the same function. Developers could optionally remove the callback from the element if they wish to avoid the check for #title.

If this is correct, then I agree with the suggested implementation. It is easy to understand, extend, and override.

@Everett Zufelt: you understand correctly.

Also, just to be clear: we shouldn't check if #title is set, but if it actually has a value, possibly using drupal_strlen().

@Xano, this approach sounds good to me too.

@Xano Can you write up a new patch? I think we also need tests.

StatusFileSize
new908 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to clear checkout directory.
[ View ]

Just re-rolling the patch from #12. Would be good to have the additions suggested by @Xano.

Status:Needs work» Needs review
StatusFileSize
new936 bytes
FAILED: [[SimpleTest]]: [MySQL] 48,405 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Noticed a bunch of notices that needed to be fixed.

Status:Needs review» Needs work

The last submitted patch, Require-title-933004-22.patch, failed testing.

Issue tags:+Needs tests

This should use trigger_error(), not drupal_set_message(). I believe drupal_render() has an example.

Also it needs tests.

Status:Needs work» Needs review
StatusFileSize
new1.07 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/form.inc.
[ View ]
  1. The patch uses an exception instead of a message or an error, as we want to require #title properties to be set.
  2. The code still applies to a fixed list of form elements. Instead, elements' info arrays need to get a flag, such as #title_required, or perhaps even a #required_properties array, so we can let developers choose which properties, if any, should be required for their elements.

IMHO, ideally, we'd do something like this by default:

<?php
$element_definition
['#required_properties'] += array(
 
'#title' => TRUE,
);
?>

This way, all elements have a required title by default, but this can be turned off and developers can add their own custom properties.
My question is: where should we merge in such default values? element_info()?

Attached a patch that just throws an error if a title is missing for most elements to see what will break in core.

Status:Needs review» Needs work

The last submitted patch, drupal_933004_25.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.07 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/form.inc.
[ View ]
  1. The patch adds the #property_validate property to all form elements, by replacing calls to element_info() with calls to the new form_element_info_merge(), which merges default values into form elements.
  2. The #property_validate property is an array of which keys are element properties, and values are callbacks that return a boolean to see if the property value is valid.
  3. Form element titles get strlen() as their validation callback.

Some examples of how this approach can be re-used:

<?php
array(
 
'#property_validate' => array(
   
'#title' => 'strlen',
   
'#options' => 'is_array',
   
'#currency_code' => 'currency_load',
   
'#node_id' => 'node_load',
  ),
);
?>

The alternative was to use empty() as a hardcoded callback, and make the #property_validate array values booleans. This is maybe twenty more characters, but adds an enormous amount of flexibility, without making the defaults any harder to work with for developers.

Status:Needs review» Needs work

The last submitted patch, drupal_933004_27.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.61 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

This should work. Instead of just merging arrays, I used \Drupal\Utility\NestedArray, which allows for simpler code.

Status:Needs review» Needs work

The last submitted patch, drupal_933004_29.patch, failed testing.

Status:Needs work» Needs review

There may be some more occurrences of form functions calling element_info() rather than form_element_info(), though.

// Edit: so apparently the patch from #29 works. The installation fails, because some form elements don't have their required properties set.

// Edit 2: now that I think of it, is there a central element processing function we can add the validation to? I can imagine this is useful for non-form elements too.

#29: drupal_933004_29.patch queued for re-testing.

Yes, it applies fine locally. There was one whitespace issue

drupal_933004_29.patch:23: trailing whitespace.
      )));
warning: 1 line adds whitespace errors.

Thanks for all your work on this Xano. Hopefully the bot just had a bad cycle.

Status:Needs review» Needs work

The last submitted patch, drupal_933004_29.patch, failed testing.

The fail is expected, because the code works, but the element definitions have not been updated. This means that any form element that is not a will cause an exception if its title is not set.

Ultimately I'm trying to bring this into Core. The test above says "Detect a Drupal installation failure." in the details. That's not usually the error message I see when it's simply about updating the tests.

Although, I suppose if it's for the installation scripts, that would make sense... Ok, slowly cluing in here.

That being said, how do we take this to the next step.

The test fails are entirely expected.

We don't need to update tests. We need to update the element definitions and usage in core. If we agree that this is the right approach to solve our problem, then we can start updating those elements. When we've done that, the testbot will be able to install Drupal again.

Status:Needs work» Needs review
Issue tags:-Needs tests, -FAPI, -accessibility

#29: drupal_933004_29.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs tests, +FAPI, +accessibility

The last submitted patch, drupal_933004_29.patch, failed testing.

Priority:Normal» Major
Status:Needs work» Needs review

This is at least major IMO. FAPI should "enforce" usage of #title.

I could work on this issue, but only after July 1st. And maybe it will be too late to get this in at that time...

Will try and work on this next week.

Ok, so I spent some time working on this patch today. But, unfortunately, I think that the code is not working properly, or maybe there's something that I'm not understanding.

After applying the patch, if you try to install drupal, you get a message saying that install_language_select_form has a non-valid #title property. But here's the code of the language select component in install_language_select_form() in install.core.inc:

<?php
  $form
['langcode'] = array(
   
'#type' => 'select',
   
'#title' => t('Choose language'),
   
'#title_display' => 'invisible',
   
'#options' => $select_options,
   
// Use the browser detected language as default or English if nothing found.
   
'#default_value' => !empty($browser_langcode) ? $browser_langcode : 'en',
  );
?>

So, could someone explain me why the error is shown, please?

Status:Needs review» Needs work

Thanks for working on this issue, here's a quick visual review of the patch in #29.

+++ b/core/includes/form.incundefined
@@ -1781,18 +1781,18 @@ function form_builder($form_id, &$element, &$form_state) {
+    if (!isset($element[$property]) || call_user_func($callback, $element[$property])) {
+      throw new \UnexpectedValueException(t('@type element in form @form_id has a non-valid @property property.', array(
+        '@type' => $element['#type'],
+        '@form_id' => $form_id,
+        '@property' => $property,
+      ))); ¶
+    }

If I'm reading this if statement correctly then the Exception will be thrown when the $callback returns TRUE, but it looks like the default callback for validating the #title element will return FALSE when it's invalid, not TRUE. I'd suggest adding a negation here.

Also, call_user_func is super slow, and adding lots and lots of calls to it will certainly effect page load time. I would imagine that it's going to be worth adding a check to see if $callback == 'strlen' and calling strlen directly.

+function form_element_info_merge(array &$element) {
+  if (isset($element['#type']) && empty($element['#defaults_loaded'])) {
+    $element = NestedArray::mergeDeep(form_element_info($element['#type']), $element);
+  }
+}

The naming of this function doesn't really reflect what it does IMHO, I would expect this function to say, take two parameters of form elements and merge them together in some way, not magically add in form element defaults. Maybe this could be re-named to form_element_info_ensure_defaults or similar?

Title:Require #title for all form elementsSome form elements should require #title to ensure their accessibility
Category:task» bug
Issue tags:-Needs tests+Needs architectural review

@Steven Jones: Thank you very much for your in-dept review.

But unfortunately the patch has an issue: not all form elements, in fact, should require the #title property (e.g. actions) as the patch currently assumes. So, I was thinking of a new patch that:

  1. Adds a "#requires_title" index set to true for form elements that should require #title in system_element_info();
  2. Adds a function to check wether a certain element requires the #title;
  3. Checks in form_builder() wether a form element requires the #title and, if it is not set or it is empty, throws an exception;

I think we need the function to check wether a certain form element requires #title because we should also enforce elements like #submit to have a #value set up; having this checks in a separate function instead of adding them directly in form_builder() should improve code readability.

Can you (or anyone else) validate this "battle plan", so that I can start working on a patch?

But unfortunately the patch has an issue: not all form elements, in fact, should require the #title property (e.g. actions) as the patch currently assumes. So, I was thinking of a new patch that:

It doesn't assume that. That's why the #property_validate property is introduced, which allows you to configure what properties should be validated and how. See #27.

@Xano: thanks for clarifying that. I didn't read all comments and I'm dealing with this issue as a novice developer. :-)

There's something that is not working properly, though: the patch throws an exception for the language_select element in the first installation screen, but that form element has a proper #title set up. In addition to this, patch seems to require that #actions and other elements (e.g. submit) require the #title and it is throwing exceptions for them when it shouldn't. How could we fix it?

+++ b/core/includes/form.inc
@@ -4843,6 +4838,48 @@ function _form_set_attributes(&$element, $class = array()) {
+/**
+ * Returns default properties for any form element.
+ *
+ * @return array
+ */
+function form_element_info_defaults() {
+  return array(
+    '#array_parents' => array(),
+    '#defaults_loaded' => TRUE,
+    '#required' => FALSE,
+    '#attributes' => array(),
+    '#property_validate' => array(
+      '#title' => 'strlen',
+    ),
+    '#title_display' => 'before',
+  );

The patch required that all elements have a title, unless they explicitly say they don't. What you can do is remove these default values, but then we need to add the property validation to every element that needs it explicitly.

ok, I'm starting to understand. Basically, we're trying to accomplish the same thing, but your solution is more abstracted than mine.

So, understanding how the patch works, I think that any element should implement #property_validate in system_element_info() declaring what properties it requires to work. We can start with the #title property for form fields that should require it and then we can extend to other properties.

However, I think that we need "abstracted" callbacks for validation. E.G. for "title" I think that we should add somewhere (maybe in a separate file) a element_validate_title() function, so that the validation can take advantage of a more complex logic (element_validate_title, for instance, should "trim" the string before passing it to strlen()).

At this point, though, I'm wondering:

  1. How would this kind of validation impact performance?
  2. Are there any other properties that should be validated by Form API and that are not validated anywhere else? If they're already validated, should we convert their validation to #property_validate?
  3. Can we implement property_validate in D8, given that code freeze is on July 1st?

It's July 1st. So, before working anymore on this, I think we need feedback from committers.

Can we go for the abstracted solution proposed by the current patch abstracted as per #48 or should we go for the simpler solution that I proposed in #44?

If needed, I'll update the issue summary as soon as we get feedback.

And ya, here's another option: we could add a #pre_render callback on form elements that require the label in system_element_info() that checks wether a form element has a label. If the answer is yes, then returns $element; otherwise, it throws the exception.

Considering this and #49 (my previous comment), I'm not sure which is the best way to go right now...

Status:Needs work» Needs review
StatusFileSize
new10.02 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

So, I wrote a patch to demonstrate @xano's approach abstracted as my proposal. This patch is completely untested.

Disclaimer: I strongly need some advice on the proper way to go (@see #49) before working anymore on this.

Status:Needs review» Needs work

The last submitted patch, form_property_validate_abstracted.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Needs architectural review

I was expecting failing tests, but I need some kind of review (see previous comment). So setting status back to NR.

Issue tags:+API change

Note that I listed this issue on both #2034999: [meta] Comply with WCAG 1.1: Text Alternatives and #2035139: [meta] Comply with WCAG guideline 4.1: Compatibility.
This issue is crucial for at least passing "A" level of WCAG guidelines.
Also, the API change is quite minor, given that providing a #title was already strongly recommended.

Issue tags:-API change

So if we're using custom validation callbacks anyway, then maybe we should pass the entire element, because otherwise you wouldn't be able to do validation based on other properties in the element, which one may want to do at some point.

@Steven Jones: Are you confirming that this is the most appropriate approach to solve this issue (@see my previous comments)?

If so, I'll be more than happy to work on this. Implementing your suggestion would require just a few seconds! :D

Status:Needs review» Needs work

I'm saying that it would be a shame to introduce an abstraction that doesn't allow for some possibly useful use-cases.
It would seem to me to be better to do this in an abstract way rather than just checking the #title property only.

I would suggest that getting a working patch written asap would be best, as at the moment all of the patches posted have stopped Drupal being installed.

Attaching an updated patch that incorporates feedback from @Steven Jones in the previous comment. I've managed to successfully install Drupal after applying this patch, let's see what testbot thinks! :-)

Disclaimer: I expect this patch to fail testing. But, if I am right, all failing tests will uncover accessibility issues! :-)

Seems a critical task to me. This is a huge step towards WCAG compliance!

Title:Some form elements should require #title to ensure their accessibilityAdd a form property validation API and implement it for #title property to ensure form accessibility
Priority:Major» Critical
Status:Needs work» Needs review
StatusFileSize
new10.38 KB
FAILED: [[SimpleTest]]: [MySQL] 50,393 pass(es), 3,840 fail(s), and 498 exception(s).
[ View ]

forgot patch.

Priority:Critical» Major

Thanks for the new patch, and yes, this is quite important for WCAG.
But while WCAG compliance is a stated aim, it currently is not a release criterion for D8, so this doesn't block D8 from being released.
Therefore setting back to major.

Status:Needs review» Needs work

The last submitted patch, form_property_validation-58.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new12.52 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views_ui/lib/Drupal/views_ui/ViewAddFormController.php.
[ View ]

So, we're getting to the hardest part: adding #title where it is missing. This is what I've got after digging into code for some hours. It is not too much, but I am not an experienced drupal developer.

I am not able to make the form in user.admin.inc work at all. I also have serious troubles understanding where the form to add a block is generated. Reality is: I don't know anything about our controller/interface/all_kind_of_abstraction that we have in core. Is there somewhere a good tutorial that explains it, let's say, to someone that is not too familiar with OOP?

In any case, if someone wants to work on this, please feel free to do it; I'd be happier to review an eventual patch, at this point! :D

Status:Needs review» Needs work

The last submitted patch, form_property_validate-62.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new13.26 KB
FAILED: [[SimpleTest]]: [MySQL] 50,455 pass(es), 3,819 fail(s), and 495 exception(s).
[ View ]

Fixing stupid errors of mine! :-)

Status:Needs review» Needs work

The last submitted patch, form_property_validate-64.patch, failed testing.

@falcon03 thanks for all the hard work. Are module developers going to feel this much pain if the patch lands? We need to be able to make it really obvious what the error is, and where the place to fix it is, if it's not already obvious. Keep going! I'd imagine that the high numbers are just because the same forms are used over and over?

All, I'm assuming that we can just look at the exceptions in the Simpletest output really, and not worry about the failures too much until the exceptions are fixed, as those are the things we've introduced?

I wonder if we can't use Symfony constraints for this instead of developing a Drupalism that will require developers to write a completely new kind of validation function for their form elements.

@Xano do Symfony constraints apply to arrays? I thought it was just objects?

I've never used it for arrays yet, but there is \Drupal\Core\TypedData\Plugin\DataType\Map, which can represent arrays and be validated using Symfony's constraint validator, for instance.

Okay, well if there's a nice way to do this with Symfony validators then let's do that, but converting FormAPI entirely to objects isn't at all possible this late in the development cycle of D8.

@Steven Jones, @xano: not sure that using Symfony constraints is doable with our form API and, most of all, if this will impact on performance more than our current API implementation. But I am the less experienced person to talk about this! :-)

Drupal's form API will be converted entirely to Symfony Forms during D9 development (there's already an issue for that).

We have a lot of work to do, but the high number of failures is related to all the exceptions. But, honestly, I see some fails that I cannot understand (e.g. the book module). I've tried it, but no exceptions were thrown during my test.

Also, there should be one or more forms related to entities (e.g. config Entities, entity translation, etc) that are abstracted in some place and reused in various places. I haven't still found them! :D

If the #68 road works out that would be amazing!
We should then check our codebase for more possible Symfony constraints conversions.

[edit:] sorry, crossposted with #72, but anyway.

Status:Needs work» Needs review
StatusFileSize
new17.77 KB
FAILED: [[SimpleTest]]: [MySQL] 51,127 pass(es), 3,468 fail(s), and 455 exception(s).
[ View ]

Worked a bit more on this; let's see how far from passing tests we are.

It's really hard form me to work on this, because I don't still understand a lot of things about the Drupal architecture; so, finding the proper places to fix errors is really time-consuming. In any case, it has a really positive impact on my Drupal skills: I am reading a lot of code.... And (maybe) I am learning a lot of things by doing it! :D

I don't expect the patch to pass tests, though, because there are some paths that do not work after applying it:

  • /block/add
  • /admin/content
  • /admin/people
  • /admin/config/content/formats/manage/full_html
  • /admin/structure/types/manage/article

The problem is... That I don't have any idea on how/where to fix the errors related to them. Any help/suggestion will be greatly appreciated! :D

Status:Needs review» Needs work

The last submitted patch, form_property_validation-74.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new664 bytes
new18.28 KB
FAILED: [[SimpleTest]]: [MySQL] 55,108 pass(es), 1,755 fail(s), and 210 exception(s).
[ View ]

One bug, which seems to be causing many of these failures, is that a details element inside a vertical_tabs element never uses its title, so we shouldn't require one.

Status:Needs review» Needs work

The last submitted patch, property-validate-933004-76.patch, failed testing.

Status:Needs work» Needs review
Issue tags:-Needs architectural review
StatusFileSize
new10.81 KB
new19.23 KB
FAILED: [[SimpleTest]]: [MySQL] 56,893 pass(es), 1 fail(s), and 5,254 exception(s).
[ View ]

Last change from me. I think this should be nested one layer further, so that a single property like #title could have more than one validator.

Also, this should be using trigger_error() like element_children() does.

Status:Needs review» Needs work

The last submitted patch, property-validate-933004-78.patch, failed testing.

Many of these errors are from dynamically generated elements, not hand-coded ones. So they'll have a single generic solution that will solve many of these errors.

@tim.plunkett: Thank you very much for working on this.

After applying the latest patch you posted I can't see the text of the error message when I visit a page where some #title were missing. Instead, I can see some errors related to the database. I'm wondering if this is due to the trigger_error() usage...

If the patch is intended to work this way, I think we have a problem: as a novice developer, it is really hard to understand that those database-related errors are due to a missing #title definition on a form element (to see the errors I am talking about you can visit admin/people or admin/content)...

@falcon03 Is it possible for you to paste the database errors here? I'm not experiencing those.

Throwing an exception will halt all processing of the page, and masks further errors. You would have to fix one at a time, never knowing how many errors might be on a page.

Triggering an error is supposed to print the same message, but in the usual errors section, and the page will finish loading. This also means you can know how many errors are on a page.

Also, I'm sure this page is all but unusable with a screen reader, but https://qa.drupal.org/pifr/test/575478 (the "View details" link on the failing patch) now lists all 5000+ errors as exceptions.

Status:Needs work» Needs review
StatusFileSize
new11.5 KB
new20.53 KB
FAILED: [[SimpleTest]]: [MySQL] 57,256 pass(es), 0 fail(s), and 5,142 exception(s).
[ View ]

I found another one of the big causes of failures, in WidgetBase.
Also I renamed our validate method and moved it into the section of form.inc with other validators.

Status:Needs review» Needs work

The last submitted patch, form-933004-83.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.59 KB
new36.61 KB
FAILED: [[SimpleTest]]: [MySQL] 57,076 pass(es), 1 fail(s), and 816 exception(s).
[ View ]

Okay, another major problem is that #process callbacks weren't being evaluated first.

It turns out we've been specifying the #title for machine_name all this time with no need to.

Also fixed views exposed forms and the permissions page.

This issue is blocked by #1812048: Build the exposed form using form API functions, so I've combined both patches for now.

Status:Needs review» Needs work

The last submitted patch, form-933004-85.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new17.75 KB
new52.3 KB
FAILED: [[SimpleTest]]: [MySQL] 56,763 pass(es), 0 fail(s), and 326 exception(s).
[ View ]

Whew. There are quite a bit of these.

Status:Needs review» Needs work

The last submitted patch, form-933004-87.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new16.36 KB
new64.46 KB
FAILED: [[SimpleTest]]: [MySQL] 57,335 pass(es), 0 fail(s), and 90 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, form-933004-89.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new10.86 KB
new74.02 KB
FAILED: [[SimpleTest]]: [MySQL] 56,464 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, form-933004-91.patch, failed testing.

Status:Needs work» Needs review
Issue tags:-Needs committer feedback
StatusFileSize
new1.87 KB
new75.18 KB
PASSED: [[SimpleTest]]: [MySQL] 56,805 pass(es).
[ View ]

Drumroll...

@tim.plunkett Congrats on the tests passing, awesome work!

The patch in #93 introduces a call_user_func, it was my understanding that such calls are very slow and should be avoided if possible? Are we introducing a performance regression here? There are a lot of forms in Drupal.

Okay fair enough, this is going to be a general performance issue for D8 then, and we don't need to worry about it in this issue.

I didn't get to review the entire patch as my train will pull into the station shortly.

And ya, here's another option: we could add a #pre_render callback on form elements that require the label in system_element_info() that checks wether a form element has a label. If the answer is yes, then returns $element; otherwise, it throws the exception.

This suggestion seems to have been ignored. I'm just quoting it to see if it may have been overlooked accidentally.

+++ b/core/includes/form.inc
@@ -1907,6 +1907,23 @@ function form_builder($form_id, &$element, &$form_state) {
+  if (isset($element['#property_validate']) && is_array($element['#property_validate'])) {

We should throw an exception if the property value is not an array.

+++ b/core/includes/form.inc
@@ -1907,6 +1907,23 @@ function form_builder($form_id, &$element, &$form_state) {
+        if (!isset($element[$property]) || !call_user_func($callback, $element)) {

I like that we don't use is_callable()/function_exists() like we do everywhere else. If the callback does not exist, this *should* raise a warning.

+++ b/core/includes/form.inc
@@ -3360,6 +3377,20 @@ function form_pre_render_actions_dropbutton(array $element) {
+  // Ensures that $title does not start with, end with or contain only spaces,
+  // and that it contains a string by verifying its length.

Should be simpler and more accurate along the lines of "Ensures that $title is a string that does not only contain whitespace."

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigImportForm.php
@@ -17,12 +17,15 @@ public function buildForm(array $form, array &$form_state) {
-    $form['submit'] = array(
+
+    $form['actions']['#type'] = 'actions';
+    $form['actions']['submit'] = array(
       '#type' => 'submit',
       '#value' => t('Upload'),

Mix-up with another patch?

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigImportForm.php
@@ -17,12 +17,15 @@ public function buildForm(array $form, array &$form_state) {
-    $form['submit'] = array(
+
+    $form['actions']['#type'] = 'actions';
+    $form['actions']['submit'] = array(
       '#type' => 'submit',
       '#value' => t('Upload'),
+      '#button_type' => 'primary',

Mix-up with another patch?

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetBase.php
@@ -180,10 +180,19 @@ protected function formMultipleElements(EntityInterface $entity, array $items, $
+          '#title' => '',

This shouldn't work, as the property validation checks for string length.

+++ b/core/modules/locale/css/locale.admin.css
@@ -85,9 +85,10 @@
-#locale-translation-status-form label {
+#locale-translation-status-form .label {
   color: #1d1d1d;
   font-size: 1.15em;
+  font-weight: bold;

Patch mix-up?

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.php
@@ -167,7 +167,7 @@ function testUpdateImportSourceRemote() {
-    $this->assertRaw('<label for="edit-langcodes-de" class="language-name">German</label>', 'German language found');
+    $this->assertRaw('<label class="visually-hidden" for="edit-langcodes-de">Update German</label>', 'German language found');

Patch mix-up?

+++ b/core/modules/locale/locale.pages.inc
@@ -544,13 +545,16 @@ function locale_translation_status_form($form, &$form_state) {
+        'title' => array('class' => array('label'), 'data' => array('#title' => $title, '#markup' => $title)),

Can we make this multiline for readability?

The ['#type'] = 'actions'; stuff is a bit out of scope.

The test change and CSS change are required. The language form was not using tableselect properly.

The empty #title = '' is followed by '#property_validate' => array(), which should be more specific, but disables the validation. Otherwise you'd have two titles there.

There is no reason to throw any exceptions here.

We don't do is_callable() checks first anymore in D8. That's a D7 only thing.

StatusFileSize
new3.52 KB
new75.31 KB
PASSED: [[SimpleTest]]: [MySQL] 57,199 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Seems to apply nicely to SimplyTest.me. Applied this patch and navigated about to see if I could see any new errors... I also applied it locally and it worked fine there too.

In looking at Views, I discovered this error in Views UI #2044511: Label missing in Page: The title of this view & The menu path or URL of this view then applied the patch in #99 and the title was there.

Forcing the title and ensuring that users hide the label the proper way is going to be a great improvement!

@mgifford: to make the patch pass tests, we had to ensure that there weren't any form items without a label. @tim.plunkett did a great job in fixing views and a lot of other stuff!

Did you try to define a form in a module and add a form component that the patch introduces validation for (e.g. '#type' => 'text') without defining the '#title' property? If so, what behavior did you get?

Anyways, I'll test the patch tomorrow and see if I can get again the SQL errors I mentioned previously in this issue.

Oh, and I was forgetting:

  1. This patch still contains patch from #1812048: Build the exposed form using form API functions which is RTBC as well. So, not sure what to do here: do we need to wait for that patch to be committed and, after that, re-roll this patch to get it in?
  2. This kind of validation IMO is going to have a significant negative impact on performance. Do we need profiling for this patch? Or can we simply add it to the "known performance regressions from D7" meta issue?

Issue tags:+Needs profiling

This has to wait for that issue to get in.

We should profile admin/content/comment with 50 comments on it.

StatusFileSize
new68.27 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form-933004-104.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled, the blocker has gone in.

@falcon03 - "Did you try to define a form in a module and add a form component that the patch introduces validation for (e.g. '#type' => 'text') without defining the '#title' property?"

I was actually just looking at the Views UI. I'd previously found a number of instances where the UI didn't have a label so I could just apply the patch from here and see that there was a label.

Challenge is that it made the Title visible since Views hadn't removed the label properly, but instead had just deleted the title.

This patch made the title visible on the page.

Issue tags:+TwinCities

This would be a great for accessibility.

#104: form-933004-104.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+FAPI, +accessibility, +Needs profiling, +TwinCities

The last submitted patch, form-933004-104.patch, failed testing.

Status:Needs work» Needs review
Issue tags:-TwinCities
StatusFileSize
new67.53 KB
PASSED: [[SimpleTest]]: [MySQL] 57,475 pass(es).
[ View ]

I swore I rerolled this. Bizarre.

@tim.plunkett: thanks for rerolling this patch. I am going to profile it. This is the first time profiling Drupal for me, so sorry for these questions but:

  1. Should I profile using xhprof and the Xhprof kit used to test twig-related patches?
  2. What should we profile? In a previous comment, you suggested profiling admin/content/comment with 50 comments on it; do you confirm that? If so, is it enough?

Oh, and I was forgetting... In comment #81, I mentioned MySQL-related errors thrown after applying this patch and viewing a page that contains a form with an element that doesn't specify a label using #title when it should do it.

It is very hard for me to report those errors because Voiceover goes really crazy on that page and doesn't allow me to read it. This is weird, and I really don't know why it happens. Since I can't take a screenshot, here with enclosed you can find a zip file containing the same page (admin/people) output in Safari's webarchive format when it works and when it doesn't. The second one is 2,9 MB bigger than the first one: is it normal?

StatusFileSize
new567.56 KB

Uffff, d.o removed the attachment from the previous comment. Here it is :-)

@falcon03
I think xhprof-kit is a great start. And yes, the comment administration page should be a fair indicator.

StatusFileSize
new67.84 KB
FAILED: [[SimpleTest]]: [MySQL] 58,262 pass(es), 0 fail(s), and 72 exception(s).
[ View ]

Rerolled patch. Please forgive any eventual mistake I did -- it's the first time I'm doing it! :-)

Profiling to come soon (no later than tomorrow), once I understand how to use the xhprof-kit; otherwise I'll use xhprof through the devel module.

Status:Needs review» Needs work

The last submitted patch, form_property_validation-114.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new726 bytes
new68.21 KB
PASSED: [[SimpleTest]]: [MySQL] 58,477 pass(es).
[ View ]

StatusFileSize
new68.37 KB
PASSED: [[SimpleTest]]: [MySQL] 58,315 pass(es).
[ View ]

@tim.plunkett: thank you for fixing the patch.

Once again the patch needed a re-rol: here it is.

Category:bug» task
Priority:Major» Normal

There's two separate issues here - that some form elements don't have titles, and adding a validation API.

I'm not at all convinced about adding a form property validation API at this stage, to the point where I'd consider bumping the patch to 9.x. For specific accessibility issues we should split those into a separate issue (which should be major).

I actually agree with catch. :\ It's too late to be introducing new APIs like this in core, IMO, especially given where we are with technical debt cleaning up the things we already committed to shipping with.

However, I'd rather not ice this initiative for 3+ years, because obviously it's finding bugs, and that's awesome. I wonder if this patch couldn't fit into something like Devel module or Coder Review module? Then it could work even for D6/D7 sites, and it would catch the problem right at the point it needs to: while the module developer's writing code, not after the code has been published as a project and downloaded onto someone's site.

Assigned:Unassigned» tim.plunkett

I can reroll without the API addition and we can at least fix the missing #title properties we've found.

Assigned:tim.plunkett» Unassigned

Disclaimer: I didn't mean at all to offend anyone with this comment. If you feel this way, please contact me and I will be happy to clarify what I mean't to say (I'm not a native English speaker). This comment also contains *a lot* of personal opinions: I'm not an experienced developer and, if I am wrong, I'm sorry and I'm happy to understand the reason why I'm wrong. The comment is *not so short* but I felt I needed to give my two cents.

@catch, @webchick: well, I don't really agree. First off, this patch is not changing any API usage -- it's simply enforcing the correct usage of the current Form API. So, according to http://buytaert.net/drupal-8-api-freeze I would consider it an API addition. At the same time, though, even if this would be considered an API change, "It could solve a critical/major issue.". Another point is that if we want Drupal to pass WCAG 2.0 AA validation we should *make sure* that each form element is properly labeled. And according to the current patch we were not in such a situation -- this underlines once again how much we need this kind of validation to enforce Drupal accessibility.

However, I can understand that this change could have a significant impact on module developers, especially in contrib. But, the consequences of this change (apart from any module that does not use FAPI properly stop working) would be -- you can easily guess -- accessibility improvements. So, I am of course in favor of them. Drupal APIs changed really a lot from D7 to D8 (e.g. let's simply think to the hook_menu -> routing system stuff). Porting a module from D7 to D8 requires a lot of work; using the classic form API properly (that is something that any developer is supposed to do even right now) is a relatively simple task compared to making a module work with all the new D8 APIs.

Finally, if the patch can't really make into D8, I would suggest committing all the accessibility improvements we did while working on this issue in a single patch.

On a side note: IMHO this situation is a bit frustrating. This issue sat for 16 days as needing committer feedback. And the feedback has come only more than a month after those 16 days, when the patch is ready and passes tests -- it only needs profiling (that I was going to do before replying to your comments). I've started working on this issue and I've done anything I could to push it forward; I don't know how @tim.plunkett (he made the patch pass tests and wrote most of it) feels, but if he's feeling the same as myself it's not a good feeling: we wasted time on this effort while we could have worked on something else. And regarding accessibility there is really a lot of work to do: we can't afford wasting time.

I strongly understand that committer's time is really limited, but I'm convinced that committer's feedback is even more important than committing a patch itself. And you guess, during the 16 days the patch has been sitting as "needs committer feedback" a lot of patches have been committed.

@tim.plunkett: I didn't mean to unassign the issue from yourself. Feel free to re-assign; if this is the way to go I'll review your patch as soon as possible.

Sorry, I don't follow that tag, only the RTBC queue. :( That's the easiest way to attract the attention of a core maintainer, since most RTBC issues get dealt with within a few days, or a week at the latest. If there is documentation that says that tag should be used, we should update it, IMO. Sorry for your frustration. :(

Let's leave aside the release cycle timing question altogether for a moment, though, and discuss this patch on its own merits. Here's my concern with the approach. It's causing ~15 lines of code to run every single time any form in Drupal is built. And while that might not seem like a lot (and in fact, really isn't when we're just talking about #title properties on form elements), what happens when other rules get added to this validator, both in core and contrib, like making sure #type select and radios has #options and #type foo has bar?

Where we need to be catching this problem is at development time, when the module developer is first building out and testing their forms. Once all of the elements on a form have the proper properties, this code is just wasted processing.

That's why I feel Devel/Coder Review is actually a better fit for this code than in core itself. We don't typically do this kind of "code babysitting" in core, and definitely not during runtime code, where it becomes a performance impact.

That said, yes absolutely let's get a patch to fix all of the transgressions you've found!

@webchick: See https://drupal.org/needs-tags I learnt there about the "needs committer feedback" tag existence. And since there are other issues opened with this tag applied, maybe we should "fix" them as well. :-) In any case, moving an issue to RTBC just to get feedback seems quite a "hackish" workaround, but I don't have any problem with it if you and other maintainers agree that this is the proper way to ask for feedback! :D

Now, speaking of this patch, you outlined a problem that I've had in mind since I started working on the patch. The real problem is that if we add this kind of validation (now I'm talking about #title specifically) in coder module/devel module, it won't get the same visibility and it won't really enforce the proper API usage. Maybe I do a mistake, but I never validate my custom modules using the coder module. And since neither core passes coder review, I'm wondering how many developers use it...

So this could fall back to one of my previous proposals: what about not adding an abstracted API and simply checking that #title is set up in form.inc, making a special case because of the #title accessibility implications?

Anyways (ok maybe I'm giggling, I don't know very well the Drupal architecture), isn't there any way to cache the form structure and use it while rendering the page instead of building the form each time? Maybe it's a stupid question, but maybe it could be worth asking....

On a side note: I think that a framework should do this kind of code-babysitting as much as possible and, of course, use some kind of caching so that code-babysitting doesn't impact on performance. I can see this as a seriously different architecture than the current Drupal's architecture, but maybe it could be worth discussing for D9... ;)

Assigned:Unassigned» tim.plunkett

Another possibility would be the API addition of the ability to do per-property validation, but don't actually use it outside of test modules, and put the implementation into devel module.

Issue tags:+API addition
StatusFileSize
new17.42 KB

#2074509: Add missing #title property to ensure form accessibility now contains the non-API addition parts.

This is the rest, which will fail until that's in.

I haven't thought this idea through completely, but a different approach would be to write a test, that automatically parses all of a module's forms and checks the relevant form elements for a title. We could either find all of a module's classes and see if they implement FormInterface or we could skip classes that don't have 'form' in their name for performance or something. Anyway, I think the benefit would be that this would be right in core, and anyone that tried to introduce an inaccessible form into core would have to fix it because the test would fail. We could also find a solution that enables contrib to have the same test coverage.

I like the sound of #128 a lot!

@tstoeckler Can you take this on? Agreed that this sounds like a great approach.

Issue summary:View changes

Clarifying issue summary.

Assigned:Unassigned» tstoeckler
Issue summary:View changes

Working on this now.

Found #2138151: ContentLanguageSettingsForm is unused (and broken) while working on this. The patch here will also need to contain that patch for now.

StatusFileSize
new11.39 KB
FAILED: [[SimpleTest]]: [MySQL] 58,707 pass(es), 44 fail(s), and 1 exception(s).
[ View ]

Here we go. This doesn't actually yet test anything, but if we get this to pass at least the discovery and instantiation works properly.

Status:Needs review» Needs work

The last submitted patch, 134: 933004-134.patch, failed testing.

Status:Needs work» Postponed

That was marked duplicate, so this is now postponed on #1987726: Convert language content page related callback to new style controller :-/

Title:Add a form property validation API and implement it for #title property to ensure form accessibilityTest that all form elements have a title for accessibility
Assigned:tstoeckler» Unassigned

Also re-titling for the new approach.
And unassigning. Don't know when the other issue will get in, and if I can get back to it then.

just updating issue summary for what this is postponed on.