Maintenance Mode message is shown at multiple places on the page on Form API Ajax Callbacks

Error is related to element rendering. By some reason AJAX wrapper with same ID applied twice http://take.ms/tux7C, also validation callback triggered twice too.

This same issue can be observed while in the configuration export (single) page.

Screenshot:

Uploading a new patch which fixes this in the \Drupal\Core\EventSubscriber\MaintenanceModeSubscriber

@aneek, thanks for the report and the patch.

I tested this on Drupal 4.x and confirmed the problem still exists. In order to test the patch I rerolled it. I'm attaching the patch but not running tests because this also needs a test. Adding a tag for the test.

Updated the IS. Un-assigning because it has been more than 5 years since this was working ed.

Moving back to NR. Test fails were caused by broken dev branch.

Verified and tested patch #18 on Drupal 9.5.x-dev. Patch applied successfully and looks good to me.
Testing steps:
1. Add Content Article type.
2. Add an image.
3. Save the page.
4. Put the site in maintenance mode.
5. Edit the node and remove the file attachment by clicking on the "Remove" button beside the file.
6. Observe that the "Operating in maintenance mode. Go online." message appears twice at the top and in the image section.
Testing Result:
1. After applying the patch the "Operating in maintenance mode. Go online." message appears at the top only.
Can be move to RTBC

@yashingole, thanks for the manual testing! It would help if you added the screens shots to the issue summary.

The remaining tasks in the issue summary need to be updated.

What I don't see here is any review of the code changes. My comment below is not a full review of the code.

+++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
@@ -128,14 +128,17 @@ public function onKernelRequestMaintenance(RequestEvent $event) {
+        // maintenance mode.
+        // But don't show maintenance message while an AJAX or Iframe upload.
+        // However, suppress it on the maintenance mode settings page.

This is not wrapped at 80 columns

Fixed the wrapping text. Not sure what needs to be updated in the IS though? It's got clear steps and screenshot.

ignore please

#24 patch working fine.

Reviewing RTBC issues to check they pass the core gates.

An issue summary update was asked for in #23, that has not happened, adding tag. There is no evidence of a code review here.

There are several steps, or gates, that an issue must pass before it is marked RTBC. For most issues following step 10 in the Review a patch or merge request task of the Contributor guide is sufficient. The complete list of core gates has more topics. Also, check the tags on the issue and make sure they are complete.

Therefor, credit has been removed per How is credit granted for Drupal core issues. And remember to read the issue summary and the comments before working on issue.

Patch #24 tested and Applied successfully on Drupal 9.5.x.

Before patch:

After Patch:

Patch #24 working fine on drupal 9.5

1. We shouldn't be putting this test coverage in the file module we should be putting this in the System module.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
   @@ -127,15 +127,18 @@ public function onKernelRequestMaintenance(RequestEvent $event) {
            if ($route_match->getRouteName() != 'system.site_maintenance_mode') {
   -          if ($this->account->hasPermission('administer site configuration')) {
   -            $this->messenger->addMessage($this->t('Operating in maintenance mode. <a href=":url">Go online.</a>', [':url' => $this->urlGenerator->generate('system.site_maintenance_mode')]), 'status', FALSE);
   -          }
   -          else {
   -            $this->messenger->addMessage($this->t('Operating in maintenance mode.'), 'status', FALSE);
   +          if (!$event->getRequest()->isXmlHttpRequest() && $event->getRequest()->get('ajax_iframe_upload', FALSE) === FALSE) {
   +            if ($this->account->hasPermission('administer site configuration')) {
   +              $this->messenger->addMessage($this->t('Operating in maintenance mode. <a href=":url">Go online.</a>', [':url' => $this->urlGenerator->generate('system.site_maintenance_mode')]), 'status', FALSE);
   +            }
   +            else {
   +              $this->messenger->addMessage($this->t('Operating in maintenance mode.'), 'status', FALSE);
   +            }
              }
            }
   

   Let's change this to:

           $show_message = $route_match->getRouteName() != 'system.site_maintenance_mode' &&
             !$event->getRequest()->isXmlHttpRequest() && 
             $event->getRequest()->get('ajax_iframe_upload', FALSE) === FALSE;
           
           if ($show_message) {
             if ($this->account->hasPermission('administer site configuration')) {
               $this->messenger->addMessage($this->t('Operating in maintenance mode. <a href=":url">Go online.</a>', [':url' => $this->urlGenerator->generate('system.site_maintenance_mode')]), 'status', FALSE);
             }
             else {
               $this->messenger->addMessage($this->t('Operating in maintenance mode.'), 'status', FALSE);
             }
           }
   

   It's a bit more readable and less levels of ifs.

3. Also the comment is not that readable now... let's change

           // Display a message if the logged-in user has access to the site in
           // maintenance mode. But don't show maintenance message while an AJAX
           // or Iframe upload. However, suppress it on the maintenance mode
           // settings page.
   

   to

           // Display a message if the logged-in user has access to the site in
           // maintenance mode. Don't show maintenance message:
           // - on AJAX requests.
           // - on Iframe uploads.
           // - on the maintenance mode settings page.
   

Addressed the points in #33

+++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
similarity index 95%
rename from core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetTest.php

rename from core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetTest.php
rename to core/modules/system/tests/src/FunctionalJavascript/File/FileFieldWidgetTest.php

The rest of this test is file test... I'm not sure that using the file field to test this is correct... but even so the rest of the test should stay in the file module. There are a number of modules in system test modules that provide ajax routes.

See the ajax_test module for example and \Drupal\FunctionalJavascriptTests\Ajax\MessageCommandTest::testMessageCommand

Went back to the original patch in #24

Addressed 2 and 3 in #33

For #1 could this remain in the file module? The test was written to tie into an existing test.

Moved test

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/FileWidgetTest.php
   @@ -0,0 +1,96 @@
   +class FileWidgetTest extends WebDriverTestBase {
   

   We're not testing the file widget we're testing the maintenance mode with AjAX requests.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/FileWidgetTest.php
   @@ -0,0 +1,96 @@
   +  protected static $modules = ['node', 'file', 'file_module_test', 'field_ui'];
   

   I think we should change the test to use the ajax_test module.

Updated the tests to be more accurate.

Tried updating to ajax_test but all those URLs are not accessible when logged in. Could not figure that out.

Wrong namespace

Thanks for the patch 2724829-43.patch, Its working perfectly fine on 9.5.x-dev

Rerunning for 10.1

Hi i applied patch#43. it is work fine and looks good.

Steps of testing:-

1. Add image field on article content type.
2. Add content on using article.
3. Add an title and image or save the page.
4. Put the site in maintenance mode.
5. Edit the node and remove the file attachment by clicking on the Remove button beside the file.
6. Observe that the Operating in maintenance mode. Go online. message appears twice at the top and in the image section.

Result After Apply Patch:-

1. After applying the patch the Operating in maintenance mode. Go online. message appears at the top only.

Adding screenshot for the reference.
Thank You

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxMaintenanceModeTest.php
@@ -0,0 +1,96 @@
+class AjaxMaintenanceModeTest extends WebDriverTestBase {

Agree with @alexpott in #40,and the test could be simplified by using ajax_test, or ajax_form_test.

Will give it another shot but was having issues with those test modules

Thanks!

Fixed up tests.

Tested and verified patch #51 with drupal: 10.1.x and the patch applied successfully and fixed the issue of multiple message on page. adding screenshots for reference. RTBC+1

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxMaintenanceModeTest.php
@@ -0,0 +1,72 @@
+    // Turn on maintenance mode.
+    $edit = [
+      'maintenance_mode' => 1,
+    ];
+    $this->drupalGet('admin/config/development/maintenance');
+    $this->submitForm($edit, 'Save configuration');

Micro-optimisation, but I think we can enable this by setting the flag direct in state rather than needing to submit the form, will save a few HTTP requests and speed up the test

Other than that, makes sense to me

Updated.

The test-only patch is failing exactly as expected:

1) Drupal\FunctionalJavascriptTests\Ajax\AjaxMaintenanceModeTest::testAjaxCallMaintenanceMode
Behat\Mink\Exception\ResponseTextException: Failed asserting that the page matches the pattern '/Operating in maintenance mode/ui' 1 time(s), 2 found.
Failed asserting that 2 is identical to 1.

I closely reviewed the code here. Almost all looked great. Found a couple of very trivial nits. Fixing those as a new patch and RTBC'ing.

Committed dbd6132 and pushed to 10.1.x. Thanks!

diff --git a/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxMaintenanceModeTest.php b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxMaintenanceModeTest.php
index 4d4d72b8ad..335557cdba 100644
--- a/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxMaintenanceModeTest.php
+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxMaintenanceModeTest.php
@@ -18,13 +18,6 @@ class AjaxMaintenanceModeTest extends WebDriverTestBase {
   use FileFieldCreationTrait;
   use TestFileCreationTrait;
 
-  /**
-   * An user with administration permissions.
-   *
-   * @var \Drupal\user\UserInterface
-   */
-  protected $adminUser;
-
   /**
    * {@inheritdoc}
    */
@@ -40,26 +33,22 @@ class AjaxMaintenanceModeTest extends WebDriverTestBase {
    */
   protected function setUp(): void {
     parent::setUp();
-    $this->adminUser = $this->drupalCreateUser([
+    $this->drupalLogin($this->drupalCreateUser([
       'access administration pages',
       'administer site configuration',
       'access site in maintenance mode',
-    ]);
-    $this->drupalLogin($this->adminUser);
+    ]));
   }
 
   /**
    * Tests maintenance message only appears once on an AJAX call.
    */
   public function testAjaxCallMaintenanceMode(): void {
-    $page = $this->getSession()->getPage();
-    $assert_session = $this->assertSession();
-
     \Drupal::state()->set('system.maintenance_mode', TRUE);
 
     $this->drupalGet('ajax-test/insert-inline-wrapper');
-    $assert_session->pageTextContains('Target inline');
-    $page->clickLink('Link html pre-wrapped-div');
+    $this->assertSession()->pageTextContains('Target inline');
+    $this->clickLink('Link html pre-wrapped-div');
     $this->assertSession()->assertWaitOnAjaxRequest();
     $this->assertSession()->pageTextContainsOnce('Operating in maintenance mode');
   }

I was going to make the above changes on commit. Not important enough to do a revert but maybe someone wants to do a followup. We don't need the adminUser variable on test object and the $page and $assert_session variables are unnecessary and not even used consistently.