Block config form does not show errors from blockValidate

Hi!

I've been reading the code to check why the error is not displayed and I found why, but I'm not sure yet how to fix it.

The problem is in the BlockForm::validateForm()method.

There we have this code:

 /**
   * {@inheritdoc}
   */
  public function validateForm(array &$form, FormStateInterface $form_state) {
    parent::validateForm($form, $form_state);

    // The Block Entity form puts all block plugin form elements in the
    // settings form element, so just pass that to the block for validation.
    $settings = (new FormState())->setValues($form_state->getValue('settings'));
    // Call the plugin validate handler.
    $this->entity->getPlugin()->validateConfigurationForm($form, $settings);
    // Update the original form values.
    $form_state->setValue('settings', $settings->getValues());
    $this->validateVisibility($form, $form_state);
  }

The important line is:

    $this->entity->getPlugin()->validateConfigurationForm($form, $settings);

Which basically execute BlockBase::validateConfigurationForm and this method finally execute our $this->blockValidate($form, $form_state); method.

The problem is the errors of the block form are in the $settings object which is the variable to we passed to the validateConfigurationForm method not in the $form_state itself, that is why the reference is lost.

What I tried to do to fix it was create a new method called setErrors() in the FormState class to we can do something like:

  $form_state->setErrors($settings->getErrors());

And this kind of work, it display the error but the input doesn't appear in red yet:

I will continue working on this to check what can be done in order to fix it.

David.

Not sure, but is this a similar bug as in #2645784: Error validation messages produced by image effect plugins not shown ?

Can you post the steps to reproduce this error?

@Mallezie Yes it seems the same problem.

@Claudio.cristea sure I will update the summary.

OK, first we need a patch that contains only the test, not the fix. That patch should fail and is the evidence of the error. Can you post such a test (maybe my test from https://www.drupal.org/node/2645784 is useful)?

Sure, I'll do it.

I will assign this ticket to me and I will work on this tonight.

Thanks for your help.

Great! This also needs a summary update to follow our summary template. See https://www.drupal.org/issue-summaries

Alright.

Here the test and the patch with the fix.

I fixed some problems in this new patch, the main fix is to now the inputs turn red when there is a validation problem (which is the expected behavior), and a few nitpicks.

Patch and interdiff attached.

This is a symptom of #2537732: PluginFormInterface must have access to the complete $form_state (introduce SubFormState for embedded forms), which if fixed properly, would make this obsolete.
If this is committed as a workaround, it should at least reference it in an @todo.

1. +++ b/core/modules/block/src/BlockForm.php
   @@ -288,6 +288,12 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +    // Check if the form has errors and copy them into the $form_state.
   

   Let's add here the @todo requested by @tim.plunkett. i added the same in #2645784: Error validation messages produced by image effect plugins not shown.

2. +++ b/core/modules/block/src/BlockForm.php
   @@ -288,6 +288,12 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +    if (FormState::hasAnyErrors()) {
   

   This can lie. For example the main form can have an error but the subform not. Then this will return TRUE. So, I don't see the gain. Since FormState protected $errors variable is initialised to an empty array, we'll have at least an empty array in $settings->getErrors(). I would remove the if().

3. +++ b/core/modules/block/src/Tests/BlockUiTest.php
   @@ -293,4 +293,22 @@ public function testBlockPlacementIndicator() {
   +    $url = 'admin/structure/block/add/test_settings_validation/classy';
   
   +++ b/core/modules/block/tests/modules/block_test/src/Plugin/Block/TestSettingsValidationBlock.php
   @@ -0,0 +1,58 @@
   +/**
   + * @file
   + * Contains Drupal\block_test\Plugin\Block\TestSettingsValidationBlock.
   + */
   

   Hmm. Sure we cannot perform this test with an existing block? The only condition would be that the block would embed an form, right?

4. +++ b/core/modules/block/src/Tests/BlockUiTest.php
   @@ -293,4 +293,22 @@ public function testBlockPlacementIndicator() {
   +    $edit = [
   +      'settings[only_numbers]' => 'abc',
   +    ];
   +    $this->drupalPostForm($url, $edit, 'Save block');
   

   You can insert directlly the array in ->drupalPostForm().

5. +++ b/core/modules/block/src/Tests/BlockUiTest.php
   @@ -293,4 +293,22 @@ public function testBlockPlacementIndicator() {
   +    $arguments = [
   +      ':message' => 'Only numbers are allowed',
   +    ];
   

   Can go inline.

Discussed on the other issue to focus on the issue that solves all cases like this. Instead of committing a workaround let's focus on the parent issue. Closing this as duplicate of #2537732: PluginFormInterface must have access to the complete $form_state (introduce SubFormState for embedded forms).