Problem/Motivation

Splitting this from #3392196: Exceptions in batch no longer are shown on the page when Javascript is disabled because there are actually 2 reasons why this happens. Once if you have Javascript enabled and one if you do not.

Obviously most users will have JS enabled but the non-JS version is still important because it means you can't test this without being a JS tests.

@bnjmnm comment on #3392196-21: Exceptions in batch no longer are shown on the page when Javascript is disabled

Info on the FunctionalJavaScript test failure:

  • The test failure is in fact due to a change introduced [##2987444]
  • Prior to that change, AJAX errors were only reported in the JS console. Nothing reported in the Drupal UI. This was addressed by adding core/drupal.message as a dependency to core/drupal.ajax
  • Apparently core/drupal.message assumes theres a '#type' => 'status_messages' render element on the page. \Drupal\Tests\system\FunctionalJavascript\Batch\ProcessingTest is failing because it expects the markup that would be provided by the status_messages render element.

Clearly something needs to be addressed in core/drupal.message since JS since JS libraries can't declare dependencies on render elements. It's safe to say that isn't something that needs to be addressed in this issue's scope. Adding a '#type' => 'status_messages' element to the test page should get the FJS test working, and the underlying issue has its own issue now #3396099: The core/drupal.message library requires a status_messages render element

Steps to reproduce

Proposed resolution

Use the approach in 3406612-use-messages-theme merge request

In \Drupal\system\Controller\BatchController::batchPage simply add

// Directly render a status message placeholder without any messages.
      // Messages are not intended to be show on the batch page, but in the event
      // an error in a AJAX callback the messages will be displayed.
      $output['batch_messages'] = [
        '#theme' => 'status_messages',
        '#message_list' => [],
        '#status_headings' => [
          'status' => t('Status message'),
          'error' => t('Error message'),
          'warning' => t('Warning message'),
        ],
      ];

This intentionally does not use '#type' => 'status_messages', which automatically render the server side messages which is not expected behavior on the batch page.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#5 3406612-show-messages-5.patch500 bytestedbow

Issue fork drupal-3406612

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow’s picture

copying my response to @bnjmnm from the other issue

@bnjmnm thanks for the explanation!

Adding a '#type' => 'status_messages' element to the test page should get the FJS test working,

We actually aren't using a test page this if failing on the regular batch system. We just use a test form to start the batch process. But then it gets directed to the core batch controller. Thats is why this would cause a problem any core or contrib usage of the batch form system when JS enabled(also without JS but the other reasons described on this issue), any exception that happened in batch callbacks would no longer show up unless they specifically were caught and handled in the callbacks.

So this just needed '#type' => 'status_messages' in \Drupal\system\Controller\BatchController::batchPage see https://git.drupalcode.org/project/drupal/-/merge_requests/4957/diffs#9d... in the MR

This gets the JS test passing for locally. I pushed up this change to see if we have any core tests that will fail if the status message element. I wouldn't think so but you never know.

I think we should make fixing the JS version its own issue if it is really this simple

tedbow’s picture

StatusFileSize
new500 bytes

Looking at \Drupal\system\Controller\BatchController::batchPage

$page = [
        '#type' => 'page',
        '#title' => $title,
        '#show_messages' => FALSE,
        'content' => $output,
      ];

My guess is that #show_messages here is useless and a holdover from Drupal 7 or 8. https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_...

And the only reason messages aren't shown on the batch page and then display after the batch job is finished is that there is no

 '#type' => 'status_messages' 

element on the page.

Update loading a patch and my guess is that \Drupal\Tests\locale\Functional\LocaleConfigTranslationImportTest::testConfigTranslationModuleInstall won't fail like it does when I add the status_messages element.

tedbow’s picture

Priority: Normal » Major
Status: Active » Needs review
Issue tags: +Automatic Updates Initiative

Marking this as major because it would probably stop #3253158: Add Alpha level Experimental Update Manager module. Anyone who used the module would not see any errors when using the form. We did put a temporary fix in the contrib module to address this #3406812: Ensure that errors are shown on form updates in core 10.1.x which just does

// Ensure error messages will be displayed on the batch page.
  // @todo Remove this work around when https://drupal.org/i/3406612 is fixed.
  if (\Drupal::routeMatch()->getRouteName() === 'system.batch_page.html') {
    // Directly render a status message placeholder without any messages.
    // Messages are not intended to be show on the batch page, but in the event
    // an error in a AJAX callback the messages will be displayed.
    $page_top['messages'] = [
      '#theme' => 'status_messages',
      '#message_list' => [],
      '#status_headings' => [
        'status' => t('Status message'),
        'error' => t('Error message'),
        'warning' => t('Warning message'),
      ],
    ];
  }

in hook_page_top(). If this fix is good enough to alpha commit of Automatic Updates in core then we could bump this issue down.

tedbow’s picture

Issue summary: View changes

I pushed another branch with a different approach 3406612-use-messages-theme
Instead of using the StatusMessages render element and adding changes that would allow it to not actually render server side messages this branch just adds and element with '#theme' => 'status_messages',, therefore just putting the mark up on the page.

This may be simpler than changing StatusMessages for the very unique case of the batch page were you don't want the messages to form the server to show

tedbow’s picture

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

I think this is a reasonable, well-scoped fix for this problem until the more general #3396099: The core/drupal.message library requires a status_messages render element lands. It's good to finally have test coverage around this, too.

  • lauriii committed d562edaa on 11.x
    Issue #3406612 by tedbow, phenaproxima: Exceptions in batch no longer...

  • lauriii committed ff0d0f48 on 10.2.x
    Issue #3406612 by tedbow, phenaproxima: Exceptions in batch no longer...

lauriii’s picture

Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed d562eda and pushed to 11.x. Also cherry-picked to 10.2.x. Thanks!

Status: Fixed » Closed (fixed)

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