Add a possibility to disable inline form errors for a complete form

In #2828092-58: Inline Form Errors not compatible with Quick Edit catch said

Was going to suggest a .yml file with a list of form IDs, then we just check in_array()/isset() rather than hard-coding knowledge of other module's forms here. Or a custom $form['#property'] to check would be more lightweight

It seems like he also supports this approach. As do I.

1. +++ b/core/modules/inline_form_errors/tests/src/Unit/FormErrorHandlerTest.php
   @@ -140,12 +140,12 @@ public function testSetElementErrorsFromFormState() {
   +   * Test that opting out for inline form errors works.
   

   opting out *of*

2. +++ b/core/modules/quickedit/src/Form/QuickEditFieldForm.php
   @@ -114,6 +114,10 @@ public function buildForm(array $form, FormStateInterface $form_state, EntityInt
   +    // Use the non inline form error display for Quick Edit forms, because in
   

   non-inline

Otherwise I think this is RTBC-worthy

The patch in #9 corrects the points from #7. I confirmed the interdiff after downloading both patches form # and #9.

Looks good. +1

It doesn't work completely yet for forms in general.

I updated @dmsmidt's errorstyle module so that it will optionally use the proposed new $form[#disable_inline_form_errors'] FAPI property.

See the GitHub pull request: Support #disable_inline_form_errors FAPI property

I tested the patch from #9 using the errorstyle module, in 3 ways (screenshots attached):

• With Inline Form Errors module disabled.
• With Inline Form Errors module enabled.
• With Inline Form Errors module enabled, and the new $form[#disable_inline_form_errors'] property set.

The patch in #9 only affects the error message at the top of the page, via Drupal\inline_form_errors\FormErrorHandler::Drupal\inline_form_errors(). When Inline Form Errors module is enabled, and $form[#disable_inline_form_errors'] is applied to the whole form, we get the normal core error message at the top of the page (i.e. a long list without links), but we still see inline error messages for each individual element.

I think we need to test for $form[#disable_inline_form_errors'] during form element preprocessing, in the _inline_form_errors_set_errors() helper function. I poked around dpm($variables) but couldn't find a quick way to read the parent form property.

Aside: the alternate proposal...

- @catch: Keep a .yml file with form_ids that should be excluded

... would make a good contrib module! Just a settings form with a big text area, listing one form ID per line. Then set #disable_inline_form_errors with hook_form_alter().

Removing novice tag, that was for the typos in #7

@dmsmidt as promised, the underscore prefixed function moved out of the .module file to a service, with some test coverage.

Looking good so far!

1. +++ b/core/modules/inline_form_errors/inline_form_errors.module
   @@ -49,6 +51,43 @@ function inline_form_errors_preprocess_datetime_wrapper(&$variables) {
   +  // Add a processor for form elements and pseudo form elements.
   ...
   +  foreach ($info as $element_type => $element_info) {
   +    if ($determiner->isFormElement($element_type)) {
   ...
   +    }
   +  }
   ...
   +function inline_form_errors_process_form_element(array &$element, FormStateInterface $form_state, array &$complete_form) {
   

   I am all for moving code out of the module into a service but why not make service in encapsulate more.

   You could use a service that does more maybe FormElementProcessor.

   Then this hook could just be 1 line.

   \Drupal::service('inline_form_errors.form_element_processor')->attachProcessCallback($info);

   Then in that method instead of setting #process to again a method in .module I think it would be ok to set the callback to method on the class. If it is ok practice to have a public static method on service. Like so:

   $info[$element_type]['#process'][] = [static::class, 'proccesElement'];

2. +++ b/core/modules/inline_form_errors/src/FormErrorHandler.php
   @@ -47,15 +47,18 @@ public function __construct(TranslationInterface $string_translation, LinkGenera
   +   * To disable inline form errors for a complete form set the
   +   * '$form[#disable_inline_form_errors]' property to TRUE. This should only be
   +   * done when another appropriate accessibility strategy is in place.
   

   Should we be explicit the $form must be the top level of the form?

3. +++ b/core/modules/inline_form_errors/tests/src/Kernel/FormElementInlineErrorTest.php
   @@ -0,0 +1,49 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   

   Can just use {@inheritdoc}

4. +++ b/core/modules/inline_form_errors/tests/src/Kernel/FormElementInlineErrorTest.php
   @@ -0,0 +1,49 @@
   +    $this->assertArraySubset(['#error_no_message' => TRUE], $form['test']);
   

   Could use a message or comment to tell why this proves it works.

+++ b/core/modules/inline_form_errors/src/FormElementDeterminer.php
@@ -0,0 +1,78 @@
+    if (!empty($element_definitions[$element_type])) {
+      $element = $this->elementInfoManager->createInstance($element_type);
+      return $element instanceof FormElementInterface;
+    }

This requires the form elements to be instantiated. Using class_implements() will be more performant.

I've tested the below code. It takes (approx.) only 30% of the execution time of the original code.

$interfaces = class_implements($element_definitions[$element_type]['class']);
return isset($interfaces['Drupal\Core\Render\Element\FormElementInterface']);

I'm working on this in the a11y sprint @Synetic.

Let's start with the low-hanging fruit: #22 points 2, 3 & 4, and one more documentation improvement in \Drupal\Tests\inline_form_errors\Unit\FormElementDeterminerTest::testIsFormElement().

Here's another patch which implements @Sutharsan's performance improvement and updates FormElementDeterminerTest to work with the class_implements method.

Created a follow-up: #2891648: Allow hiding of inline form errors on non-form elements when it is disabled for the complete form

As discussed with @Lendude and @dmsmidt at the code sprint, this patch changes the name of FormElementDeterminer to the more meaningful SupportedRenderElementHelper. It also improves some inline docs and cleans up some redundant use statements. Unfortunately interdiffs aren't very readable for changes like this.

This is probably my last patch on this issue for today, related to #22 number 1. The patch moves inline_form_errors_process_form_element() to a static method in a helper class \Drupal\inline_form_errors\FormElementProcessor. The FormElementProcessor class is not a service, because there is no need for it to be instantiated or to be present in the service container.

All feedback from #22 has been addressed, this looks really nice now.

I did read through the code and it didn't raise questions on my side. I also noticed that we've added test coverage so I removed the tag. I will let one of the other maintainers make the final call on this. Before one of the other maintainers will take a look at this, I think we totally should still create a small change record for this addition.

I've also updated the issue credits.

Thanks everyone!

1. +++ b/core/modules/inline_form_errors/src/FormErrorHandler.php
   @@ -47,15 +47,23 @@ public function __construct(TranslationInterface $string_translation, LinkGenera
   +   * #disable_inline_form_errors property to TRUE on the top level of the $form
   +   * array, ie:
   +   * @code
   +   * $form['#disable_inline_form_errors property'] = TRUE;
   

   Code example is incorrect.

2. +++ b/core/modules/inline_form_errors/src/SupportedRenderElementHelper.php
   @@ -0,0 +1,79 @@
   +  public function supportsInlineFormErrors($element_type) {
   +    return in_array($element_type, $this->pseudoFormElements) || $this->elementInstanceOfFormElementInterface($element_type);
   +  }
   

   Is there a use case to alter this? Would swapping the service serve that use case or would multiple sources want to alter it (if there is a use case to alter it in any case)?

@dmsmidt: great, we can definitely expand features later on if this implementation / API does not make further expansion without a BC break impossible / very hard. Not sure how would the expansion work, annotation additions would be backwards compatible I guess. Can you open that followup?

Mostly nits left:

1. +++ b/core/modules/inline_form_errors/src/SupportedRenderElementHelper.php
   @@ -0,0 +1,79 @@
   +   * SupportedRenderElementHelper constructor.
   

   Constructs a new SuppportedRenderElementHelper.

2. +++ b/core/modules/inline_form_errors/src/SupportedRenderElementHelper.php
   @@ -0,0 +1,79 @@
   +   * Determine if a render element type supports inline form errors.
   ...
   +   * Determine if a render element type implements FormElementInterface.
   

   Determines

3. +++ b/core/modules/inline_form_errors/src/SupportedRenderElementHelper.php
   @@ -0,0 +1,79 @@
   +    $element_definitions = $this->elementInfoManager->getDefinitions();
   ...
   +    if (!empty($element_definitions[$element_type])) {
   

   Should use ->hasDefinition() and ->getDefinition()

4. +++ b/core/modules/inline_form_errors/src/SupportedRenderElementHelper.php
   @@ -0,0 +1,79 @@
   +      return isset($interfaces['Drupal\Core\Render\Element\FormElementInterface']);
   

   FormElementInterface::class

5. +++ b/core/modules/inline_form_errors/tests/src/Kernel/FormElementInlineErrorTest.php
   @@ -0,0 +1,50 @@
   +   * Test that form elements get the #error_no_message property when opting out
   

   Tests

Re. #34 - #37:
We had already identified that need during the sprint on July 2nd and I created a follow up for it in #28: #2891648: Allow hiding of inline form errors on non-form elements when it is disabled for the complete form.

We should close either that issue or #2895882: Allow contrib modules to create Inline Form Errors supported RenderElements that are no Form Elements as duplicate.

PS re. #34.1 Good catch, can't believe I missed that :-)

This fixes the missing empty line that automated code sniffer found.

I haven't figured out the test failure, but it's to do with the changes for #38.3

1. +++ b/core/modules/inline_form_errors/inline_form_errors.services.yml
   @@ -0,0 +1,4 @@
   +    class: Drupal\inline_form_errors\SupportedRenderElementHelper
   
   +++ b/core/modules/inline_form_errors/src/SupportedRenderElementHelper.php
   @@ -0,0 +1,77 @@
   +class SupportedRenderElementHelper {
   

   Usually "Helper" classes only contain static methods.

   Also, services should have interfaces provided

2. +++ b/core/modules/inline_form_errors/src/FormErrorHandler.php
   @@ -47,15 +47,23 @@ public function __construct(TranslationInterface $string_translation, LinkGenera
   +   * array, ie:
   

   i.e.:

   (or don't use i.e.)

3. +++ b/core/modules/inline_form_errors/src/SupportedRenderElementHelper.php
   @@ -0,0 +1,77 @@
   +   * @var \Drupal\Core\Render\ElementInfoManager
   ...
   +  public function __construct(ElementInfoManager $element_info_manager) {
   
   +++ b/core/modules/inline_form_errors/tests/src/Unit/SupportedRenderElementHelperTest.php
   @@ -0,0 +1,56 @@
   +    $element_info_manager = $this->getMockBuilder(ElementInfoManager::class)
   

   These should all be ElementInfoManagerInterface

4. +++ b/core/modules/inline_form_errors/tests/src/Unit/SupportedRenderElementHelperTest.php
   @@ -0,0 +1,56 @@
   +      ->setConstructorArgs([
   +        $this->getMock(\Traversable::class),
   +        $this->getMock(CacheBackendInterface::class),
   +        $this->getMock(CacheTagsInvalidatorInterface::class),
   +        $this->getMock(ModuleHandlerInterface::class),
   +        $this->getMock(ThemeManagerInterface::class),
   +      ])
   +      ->setMethods(['getDefinitions', 'createInstance'])
   +      ->getMock();
   ...
   +    $element_info_manager->expects($this->any())
   +      ->method('getDefinitions')
   +      ->willReturn([
   

   Ideally these would be using Prophecy

I don't think that allowing others to add more $pseudoFormElements is in scope for this issue. Plus, only one module per site could swap out the service, so a service isn't an appropriate solution.

This fixes my concerns from above.

The interdiff is bigger than patch, sorry.

The change to ElementInfoManagerInterface is a necessary part of this patch, but opened a separate issue in case it is deemed out of scope:
#2899082: ElementInfoManagerInterface must extend DiscoveryInterface

@dmsmidt is working on the CR.

Addressed all 3 points, thanks!

Isn't $form['#disable_inline_form_errors'] = TRUE; an API shouldn't we have a inline_form_errors.api.php Otherwise it seems like it would be really hard to figure this out.

I guess we don't have don't have a practice for making *.api.php files for # properties.

It is documented here https://www.drupal.org/docs/8/core/modules/inline-form-errors/inline-for...

Looks great! RTBC 🏅

Some feedback on the change record. It says:

It is now possible to disable Inline Form Errors (IFE) for a specific form when the module is enabled. This is an addition to the Form API.

Isn't it more "for a specific form or any form element"? Actually, now that I'm looking at it, the keys are #disable_inline_form_errors and #error_no_message. That's a bit confusing.

I also agree with @tedbow's first instinct that this needs to be documented in the API documentation. The handbook page is good to have, but it's not API documentation. We've definitely gone through a lot of different best practices for where form elements and properties should be documented, and it's probably not a *.api.php; I can't remember right now though how we document them normally.

This was to be the stop-gap solution. The added property ONLY works when used on the top-level $form element.
#2891648: Allow hiding of inline form errors on non-form elements when it is disabled for the complete form is the issue for individual elements. The property to be added in that issue would be documented directly on \Drupal\Core\Render\Element\RenderElement.

The only other properties that exclusive to the top-level $form element I can think of are internal-only: #cache_token, #build_id, #action
They are not documented anywhere.

Rebase since #2899082: ElementInfoManagerInterface must extend DiscoveryInterface went in.

Hmm, what is #2891648: Allow hiding of inline form errors on non-form elements when it is disabled for the complete form postponed on? This doesn't seem like it's all that hotfix-y and the other doesn't really propose anything particular yet.

The comment from @catch that spawned this issue is:

I think it's OK for the patch to go in like this, but inline_form_errors should eventually be not-a-module whereas quickedit is going to stay one. So I think we need a follow-up to remove the hard-coded dependency.

Was going to suggest a .yml file with a list of form IDs, then we just check in_array()/isset() rather than hard-coding knowledge of other module's forms here. Or a custom $form['#property'] to check would be more lightweight.

#2848319: Find a way to make it easier to hide Inline Form Error messages on child elements was suggested as the issue for that, but not sure that's about this issue at all? Looks like it was talking about other bugs which are already fixed, might just need re-scoping though.

Might just need an issue summary update but it's not clear to me what the next steps are here.

(From #2828092-58: Inline Form Errors not compatible with Quick Edit .) So I guess I was imagining this issue's scope to be somewhat wider than it actually is. (For some reason I was thinking the #error_no_message thing was IFE, but it's just generic FAPI that's also used on a few datetime things.) However, I'm wondering what the value of this is? I could see disabling IFE with a particular module, but an API (a FAPI key, no less) to disable IFE for just a single form seems pretty edgecase to me.

I did commit #2899082: ElementInfoManagerInterface must extend DiscoveryInterface separately (thanks for the separate issue; I was about to push back on that change but then the separate issue answered my questions).

In terms of documenting this bit, I guess @tedbow's inline_form_errors.api.php suggestion is as good as any, but I do think we need something more than a handbook page.

So, the summary for the other issue is:

I'm creating this issue as 'Postponed' because #2856950: Add a possibility to disable inline form errors for a complete form should be possible for 8.4.0, but it's probably too late to be changing render element annotations now.

That's different from the scope in the changed title, though.

My question is, if this issue was just intended as a "stopgap" that we could land by 8.4.0, and we had a more complete fix in mind, how about we instead just go for the complete fix? What would it look like? As currently titled, #2891648: Allow hiding of inline form errors on non-form elements when it is disabled for the complete form is unrelated to the scope of this issue, but do they have some underlying shared solution?

Usability is preferred over UX, D7UX, etc. See https://www.drupal.org/issue-tags/topic

This looks great! And there are no remaining tasks.

+++ b/core/modules/inline_form_errors/src/FormErrorHandler.php
@@ -47,15 +47,23 @@ public function __construct(TranslationInterface $string_translation, LinkGenera
+   * $form['#disable_inline_form_errors'] = TRUE;

Should we document this as an allowed key in https://www.drupal.org/node/2117411

I couldn't find anywhere in the code that we document allowed keys for top-level form and I don't think this fits on FormElement as it only applies to the top level form. Does anyone know if we have that somewhere?

Nothing there yet, but \Drupal\Core\Render\Element\Form might be a place to document that?

What about the login and password recovery form and similar forms that only have 1-2 form elements? Possibly disabling just the summary there might make more sense than disabling it completely (#2880011: Add a #disable_inline_form_errors_summary property to disable the Inline Form Errors summary but it is IMHO quite confusing that the message is on the username field when it could just as well be the password being wrong.

Also did a re-test, looked like random fails.

I've no objection to disabling IFE on the user login page. The error message is a general one which applies to both fields, and has a call to action ("forgot?") which deserves prominence in the main messages area. It also serves as a good demonstration of the new FAPI property.

Should it be a blocker to this issue, or a follow-up?

forgot aside:
it's confusing that the link is on the username field when it's not even the password that has an error; in this situation. But, i know, this message is set for it's own purpose..

Queued another test run

Updating review credits

In code documentation about features added by a module should not be put in code that is not of that module.

As discussed on slack, I agree

Committed as 128cb26 and pushed to 8.5.x.

As per https://www.drupal.org/core/release-cycle-overview#current-development-c... 8.4 is in 'critical fixes only', so this can't be backported to 8.4

Published change record