When Batch ID doesn't exist, Drupal should emit a 404

I tried this out locally and it completely changed the way I think about batch.module. Drupal 7.23(24) needs this!!!!!! The logic in this patch aligns with my world view.

Update: Grammar fix

thanks for this patch,
worked for me, this was on my todo list for a long time!

It works perfectly and the code is beautifully written! I've been waiting for this since D3 ou D4... not sure, it's been a long time!

This looks like a good idea, but needs to go in Drupal 8 first.

Also, the code would need to exit or return something, since drupal_not_found() just prints HTML to the page but doesn't stop the rest of the code in the function from executing, at least I think...

I'll take a look at this for D8.

Here is a patch for D8.

A couple of considerations:

• The exception message is swallowed by the exception handling down the line and the resulting message "The requested page "/batch" could not be found." is suboptimal but I think that is outside the scope of this issue.
• There is no unit test for the change. BatchController calls batch_get() from form.inc via _batch_page(). which is not immediately available in the test cases. I'm unsure about the right way to go about this.

Hi, this is fixed for Drupal 8. Please review.

I'm slightly in favor of patch from #7, since it doesn't change an error message. Also, I don't see a reason to use format_string() instead of t()

#7: Error message should be wrapped with t() to allow translatable messages

It's done. Wrapped the message with t().

Ok, version 3 attached. Removed the message and t() as they are not required.

Please see v3 attached. I removed the t() and message as it's not required.

This is fixed.

This is fixed.

#8: In regarding to changing the error message I think it makes a difference. Better exception messages makes for better debugging and with the current exception handling further up the tree the additional technical information is not exposed to the user.

I know the NotFoundHttpException is usually used without any message throughout Drupal but ViewPageController does this the same way as my suggested patch.

Anyways I'll set this to needs review again.

In the current 8.x, text exposed to exception constructor, is not used at all (this indeed may be opened as a separate issue). So, for now, patches from #14 and #6 have the same effect.
Future-proof, I would prefer to have original error text, wrapped in t(), and passed to NotFoundHttpException() constructor.

So, for now, patches from #14 and #6 have the same effect.

Well therefore we would need to adapt our exception handling but I think we should really take that into account.

This issue needs issues summary update, and the fix itself looks like a good starter issue.

My suggestion is reroll of patch #6 + open followup issue for use of passed text in HttpNotFoundException

Ok, here is a reroll from #6.

Added #2372011: NotFoundHttpException handler should take into account it's parameters as a follow-up

+++ b/core/includes/batch.inc
@@ -44,8 +45,7 @@ function _batch_page(Request $request) {
+      throw new NotFoundHttpException(format_string('Page controller for batch %id requested, but batch was not found.', array('%id' => $request_id)));

let's use the utility object here, ie. String::format()

+++ b/core/includes/batch.inc
@@ -44,8 +45,7 @@ function _batch_page(Request $request) {
+      throw new NotFoundHttpException(format_string('Page controller for batch %id requested, but batch was not found.', array('%id' => $request_id)));

It would be awesome to use String::format() instead, but not setting to needs work for that.

Here is a update to #20 which uses String::format().

Awesome, thanks a lot!

It's just that there is an ongoing effort to replace the usage of deprecated functions so it's nice to not introduce new calls to them :-)

No worries. Happy to help.

It'd be good to add a test for this functionality. Obviously we did not have one for the redirect as that would be failing.

I am writing tests for this. This is what I thought:

I will write a simpletest where a user is trying to access a batch process with some arbitrary id. If in return he gets 404 error, that means test pass.

This is just an explanation about the test I am going to write. Please correct me if I am wrong. I am not much aware of simpletest or unit test :p :)

Thanks.

Here is the patch with tests. Please review :) I hope the test is alright. Not removing the "Need tests" tag, as I am not sure the test case is right.

I am going to review it and update the issue summary.

Try it manually and read all the lines in the patch. Seems to work fine.

+++ b/core/tests/Drupal/Tests/Core/Batch/BatchNotFoundTest.php
@@ -0,0 +1,50 @@
+    $batch_id = mt_rand(0, 1000);

why pick a random id?
Maybe we should pick one we know will not exist.

Also, I think we need a tests only patch here, to make sure it fails nicely with the testbot.

Small nitpick changes to documentation with new patch. These are shown in the interdiff

- "Id" to "ID"
- setUp should have {@inheritdoc} docblock
- empty line before the final } in the class: https://www.drupal.org/node/608152#indenting
- Summary needs to be one line: https://www.drupal.org/coding-standards/docs

Also adding a tests only patch to show that the patch works as intended.

1. +++ b/core/includes/batch.inc
   @@ -43,8 +45,7 @@ function _batch_page(Request $request) {
   +      throw new NotFoundHttpException(String::format('Page controller for batch %id requested, but batch was not found.', array('%id' => $request_id)));
   

   I would prefer something like 'Batch %id request, but not found.', there is no need to talk about 'Page controller' here.

2. +++ b/core/tests/Drupal/Tests/Core/Batch/BatchNotFoundTest.php
   @@ -0,0 +1,53 @@
   +  function testBatchNotFound() {
   

   Let's mark it to be a public function testBatchNotFound() already.

1. Removed assert message.
2. "Id" to "ID"
3. Added "public" to function.

needs work for #38 1.

and we should get an opinion on the random question from #36.

--

Just a note, about adding public visibility is from the standard: https://www.drupal.org/node/608152#visibility

And thanks for removing that assert message, it wasn't adding any information. the default assert message should be fine in this case.

Using an arbitrary batch id looks odd. It would be good if we are able to get last batch id, and then increment it by any number (say 1), and use that batch id for testing.

But in any case we are getting page not found, even with batch ids that have already been used.

Suggestion:
Start a batch process within test. Use db_next_id() to retreive the next batch id, and then use it for "batchnotfound" test.

I have tried this code for starting a batch process and doing db_next_id() to get next batch id.

  public function testBatchNotFound() {
    $this->drupalLogin($this->user);

    batch_test_stack(NULL, TRUE);

    batch_set(_batch_test_batch_0());
    batch_process('batch-test/redirect');

    // Wait for batch process to finish.
    sleep(120);

    $batch_id = db_next_id();

    $this->drupalGet('batch', array(
      'query' => array(
        'op' => 'start',
        'id' => $batch_id,
      ),
    ));

    $this->assertResponse(404);
  }

But I am getting this error message.

This may be because there is an ongoing batch process which has not finished. That is why I have used sleep(120), but I am still getting that error.

Any idea regarding this? Or is it the right way for starting batch process inside tests.

Invoking test scripts for #43

We can create a dummy user and delete it. The cancellation process of user will be a batch process. This way we can replicate batch process inside test. Writing a patch for this.

A test account is created and deleted. The deletion process will be a batch process. We then use db_next_id() to retrieve a non-existent batch id, and then use it for our test case. I hope this logic is enough for the test case.

I do not understand.. The same patch is passing the test in localhost.

Uploading patch with empty $edit in test. This patch passed clean in my localhost. I hope it passes here too...

Patch without fix. This patch should fail testing.

Desired result obtained. Moving this to "Needs review".

#49 gives 404 error as desired on visiting /batch?id=123 However, message 'Batch 123 requested, but not found.' does not show up.

+++ b/core/includes/batch.inc
@@ -43,8 +45,7 @@ function _batch_page(Request $request) {
+      throw new NotFoundHttpException(String::format('Batch %id requested, but not found.', array('%id' => $request_id)));

It is only an exception message.

The 404 page is rendered from this code in core/modules/system/src/Controller/Http4xxController.php

/**
   * The default 404 content.
   *
   * @return array
   *  A render array containing the message to display for 404 pages.
   */
  public function on404() {
    return [
      '#markup' => $this->t('The requested page could not be found.'),
    ];
  }

I guess this is what happening, though I would strongly ask some Drupal 8 expert guy would review this.

Also refer Symfony docs: http://api.symfony.com/2.1/Symfony/Component/HttpKernel/Exception/NotFoundHttpException.html

*bump* anybody wants to do review?

@subhojit777: can you please add Beta changes evaluation section?

I looked at the test-only patch at #50, read all the lines, the test seems
relevant to the test. Only thing to fix is the naming conventing for the class property.

index 0000000..341b9eb
--- /dev/null

@@ -0,0 +1,76 @@
+  protected $user_to_be_deleted;

https://www.drupal.org/node/608152#naming

"Methods and class properties should use lowerCamel naming"

The testbot is raising only on failure, which is good, since it demonstrates the bug exists.

Next, actually right now, I'm going to read whole patch and provide feedback.

After reviewing the whole patch at #49 and read the comment at #57, it makes sense to use default Drupal behavior than throwing a custom 404, which is not the case.

I will add the beta changes now.

I think develCuy has already added the beta evaluation changes. Thanks!

As develCuy has already said at #63, we should avoid using a custom 404 message in /core/includes/batch.inc. Instead, there is a function called on404() in /core/modules/system/src/Controller/Http4xxController.php which could be used to display a default 404 message. The problem is that this function is in a module, and as we need to use it inside batch.inc, it should be part of core. Bottom line is, we must solve this architectural problem and then use on404() for the 404 message.

at first I thought maybe do something like

+++ b/core/includes/batch.inc
@@ -20,6 +20,7 @@
 use Drupal\Core\Batch\Percentage;
 use Drupal\Core\Form\FormState;
 use Drupal\Core\Url;
+use Drupal\system\Controller\Http4xxController;
 use Symfony\Component\HttpFoundation\JsonResponse;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\RedirectResponse;
@@ -43,6 +44,7 @@ function _batch_page(Request $request) {
   if (!$batch) {
     $batch = \Drupal::service('batch.storage')->load($request_id);
     if (!$batch) {
+      Http4xxController::on404();

but then I searched for on404

ag on404
core/lib/Drupal/Core/EventSubscriber/CustomPageExceptionHtmlSubscriber.php
72:  public function on404(GetResponseForExceptionEvent $event) {

core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
81:  public function on404(GetResponseForExceptionEvent $event) {

core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
52:  public function on404(GetResponseForExceptionEvent $event) {

core/lib/Drupal/Core/EventSubscriber/ExceptionLoggingSubscriber.php
59:  public function on404(GetResponseForExceptionEvent $event) {

core/lib/Drupal/Core/EventSubscriber/Fast404ExceptionHtmlSubscriber.php
74:  public function on404(GetResponseForExceptionEvent $event) {

core/lib/Drupal/Core/EventSubscriber/HttpExceptionSubscriberBase.php
25: * public function on404(GetResponseForExceptionEvent $event) {}

core/modules/system/src/Controller/Http4xxController.php
35:  public function on404() {

core/modules/system/src/Tests/Routing/ExceptionHandlingTest.php
57:  public function testJson404() {

core/modules/system/system.routing.yml
21:    _controller: '\Drupal\system\Controller\Http4xxController:on404'

so it seems this is not directly called anywhere in core.

Maybe that is because it is triggered by an event.

I'm not sure what to do here. Maybe we want to trigger an event?

@Crell managed to review why in world core/modules/system/src/Controller/Http4xxController.php exists, and explained me that is being used somewhere else and we don't need to care about it, so let's forget about on404() since it has nothing to do here!

Following #73, the patch at #66 is good, so changed my mind about #63, since the custom message added by the patch makes A LOT of sense for the particular context of this issue.

Well this certainly makes a great deal more sense. Looks like Alex's feedback has been addressed.

Committed and pushed to 8.0.x. Thanks!

@webchick reverted this because the added test had a high random fail rate:

Drupal\Tests\Core\Batch\BatchNotFoundTest (17 pass(es), 2 fail(s), and 0 exception(s))
   - [fail] [Other] "Found the Cancel accounts button" in BatchNotFoundTest.php on line 62 of Drupal\Tests\Core\Batch\BatchNotFoundTest->testBatchNotFound().
   - [fail] [Other] "Found the requested form fields at " in BatchNotFoundTest.php on line 62 of Drupal\Tests\Core\Batch\BatchNotFoundTest->testBatchNotFound().

I didn't determine what was causing the random failure, but this looks really suspect:

$batch_id = db_next_id();

+++ b/core/tests/Drupal/Tests/Core/Batch/BatchNotFoundTest.php
@@ -0,0 +1,76 @@
+  /**
+   * Modules to enable.
+   *
+   * @var array
+   */
+  public static $modules = array('views');

Also (unrelated), it would be better to not couple this test to Views. We should look in existing batch tests to see how test batches have been implemented previously.

For whoever works on this patch, you can run the test repeatedly locally to check whether it's failing randomly:

php /path/to/core/scripts/run-tests.sh --color --url http://localhost/your_d8_site_here/ --repeat 100 --class 'Drupal\Tests\Core\Batch\BatchNotFoundTest'

And to debug the result that failed:

php /path/to/core/scripts/run-tests.sh --color --url http://localhost/your_d8_site_here/ --repeat 100 --die-on-fail --class 'Drupal\Tests\Core\Batch\BatchNotFoundTest'

Thanks @xjm for the pointer on how to run the tests. I found the bug and fixed it so the BatchNotFoundTest runs correctly.

With regard to #79 I have left the reference to views in since this is a 'real world' test using people admin pages which require views. In D7 there was a batch_test module which created test based scenarios and batches, only one of which, testPercentages, is recreated in D8. If these tests are all to be recreated for D8 then it is perhaps outside the scope of this issue.

not novice

thank you.

I wanted to see what the fix was, so I made an interdiff.
For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff

Interdiffs are really helpful to do. it makes it much more likely to get reviews and feedback.

New tests we add are to be done better than we have in the past.

I agree, there should be a way to test this that does not involve views. need work for that.

Lets see what I can do.

Just curious - Why can't we use views for the tests.

I just saw there is are tests called ProcessingTest.php, PageTest.php which involves batch_test module. I was thinking of using the same logic, but it *also* enables a module.

I have re-wrote the test using batch_test module .

The latest patch addresses all the issues raised. It needs a reroll and it is a suitable novice task.

Adding patch.

@shashikant_chauhan, thanks for the reroll! Please add an interdiff, it makes it a lot easier to review.

I tracked down the String error. According to #2476075: Remove usage of \Drupal\Component\Utility\String::format() String::format has been removed and replaced with \Drupal\Component\Utility\SafeMarkup::format(). And string is a reserved word in php7 #2454447: Split Utility\String class to support PHP 7 (String is a reserved word).

Updated patches to use SafeMarkup instead of String.

Created separate issue for D7 backport - https://www.drupal.org/node/2826866#comment-11776511

Screen shot of http://d8.local/batch?id=4

Screen shot of http://d8.local/xyzzy

Does it matter about the different theme for the batch URL and the non batch URL?

Bumping version

Tested on 8.3.x using Simplytest.me, patch still working.

Also re-ran tests against 8.3.x.

We need a change record for this change. Also we need to consider if this will break any expectations - I don't think so.

Added draft change record - https://www.drupal.org/node/2842748. First time I've done one, so hope it's ok.

+++ b/core/includes/batch.inc
@@ -42,8 +44,7 @@ function _batch_page(Request $request) {
+      throw new NotFoundHttpException(SafeMarkup::format('Batch %id requested, but not found.', array('%id' => $request_id)));

We can give a better error message:

Trying to execute an invalid batch ID : %batch_id

+++ b/core/includes/batch.inc
--- /dev/null
+++ b/core/tests/Drupal/Tests/Core/Batch/BatchNotFoundTest.php

+++ b/core/tests/Drupal/Tests/Core/Batch/BatchNotFoundTest.php
@@ -0,0 +1,45 @@
+ */
+class BatchNotFoundTest extends WebTestBase {

That test seems to be in the wrong location. The tests there should extend BrowserTestBase and not WebTestBase

Rerolled test into BrowserTestBase, passes.

I am not why this one is marked as Patch to be Ported @aron.beal, I am changing it to needs Needs Review for the tests to run.

Now we know why the first patch patch failed and second passed as first is test only! This should be clear to go :) marking it as RTBC

Hi, yes. Participating in a sprint yesterday, still a little unclear as to all the protocols for getting patches committed, but I was told by someone there that uploading tests only as a separate patch gave thread reviewers a clearer picture that the patch fixed the issue the test was testing. @navneet0693, in a similar vein, I thought because I was making changes that it should have been marked as a separate patch. I'll leave it as 'needs review' next time.

hey @aron.beal thanks for efforts and patch :)

+++ b/core/includes/batch.inc
@@ -42,8 +44,7 @@ function _batch_page(Request $request) {
+      throw new NotFoundHttpException(SafeMarkup::format('Trying to execute an invalid batch ID : %batch_id.', array('%batch_id' => $request_id)));

We should not use SafeMarkup in exception messages. Actually we should not use it at all, because it is deprecated. But either way, see here for the correct usage: https://www.drupal.org/node/608166

Thanks!

Updated patch to use sprintf and removed unwanted includes.

re-adding the same to add test, not sure if a potential bug but I did preview before saving and tests were not added.

Edit: Test not added again, not sure why.

Changing to "Needs Review" for the given tests to run.

Thanks @Navneet.

Added the error message to the User interface changes section in the IS. Changes have been made for all the suggestions so nearly there.

Just two questions and an action for the change record.

1. +++ b/core/includes/batch.inc
   @@ -42,8 +43,8 @@ function _batch_page(Request $request) {
   +      $exception_message = sprintf('Trying to execute an invalid batch ID : %s.', $request_id);
   +      throw new NotFoundHttpException($exception_message);
   

   For such a simple message why not do this?
   throw new NotFoundHttpException(sprintf('Trying to execute an invalid batch ID : %s.', $request_id))

2. +++ b/core/includes/batch.inc
   --- /dev/null
   +++ b/core/tests/Drupal/FunctionalTests/Core/Batch/BatchNotFoundTest.php
   

dawehner stated in #114 that the test was in the wrong location. Is this the correct location?

The change record needs to be updated to use the latest version of the error message.

Note that the patch in #125 is identical to the one in #126.

Cleaning the tags. We are working on this issue.

Added the change #1 suggested by quietone.

+++ b/core/includes/batch.inc
--- /dev/null
+++ b/core/tests/Drupal/FunctionalTests/Core/Batch/BatchNotFoundTest.php

Test location looks correct to me.

After installing the patch and navigating to /batch?id=123 I get "The requested page could not be found." instead of the new error message.

I tried but could not find anything why drupal is showing its default 404 message instead of new message passed to it.
@quietone, Any idea how can I fix this issue.

@quietone

I suggest we add a Drupal message if we really want to display the error message.

Checked the source, message is not used anywhere in NotFoundHttpException class, it could be used if the exception was really caught and $ex->message was read but here we simply display the 404 page.

Patch rerolled. However when I review, I noticed that this problem is already solved (please view interdiff) in code. So this patch is only the test for the feature.

Sorry. comment #141 has wrong patch.

Patch rerolled. However when I review, I noticed that this problem is already solved (please view interdiff) in code. So this patch is only the test for the feature.

I have re-added the Drupal message, as the exception message was not being used.

Also updated the test to meet current standards.

Would this patch also handle the scenario where user hits /batch?id=123&op=start. This happens when users try to navigate to a batch page by clicking the back button of the browser.

Thank you everyone for working on this.

@kswamy, this patch does work with /batch?id=123&op=start. The 'op' parameter is not used when finding the batch in Drupal, only the 'id'. This means that setting or changing the value of 'op' does not affect the result.

Unfortunately, this still needs work. The error message displayed on the site is 'Trying to execute an invalid batch ID : {ID}.' but the change record states that the message will be 'Batch {ID} requested, but not found.' (#2842748: Return 404 if Batch ID does not exist)

The message from the change record is better as it moves the emphasis on to Drupal rather than the user. ('Invalid' means the user did something wrong, but 'not found' means Drupal did something wrong.)

Even so, that message is doesn't flow well. Maybe something like 'The requested batch ({ID}) could not be found.' would be better?

This would need to be changed:

• in the issue summary
• in the change request
• and in the code.

All these can be novice tasks.

Otherwise this look ready to go.

I've updated the messages in the previous patch #143 as I agree that the suggested message sounds more natural.

Hi @smagdits.

Thank you having a go at this.

It looks like your patch has some extra code that wasn't in the previous one which is why the patch is failing. Maybe you were working on another issue?

Thanks for updating the issue summary. Now we all know what we are expecting the final patch to do.

My apologies, looks like you're right about the extra code being from a different issue. I'll be sure to check my patches more thoroughly in the future, thank you. I've remade the patch and tested applying it.

Thanks @smagdits.

There's still a few more things to do.

1. +++ b/core/includes/batch.inc
   @@ -42,8 +43,8 @@ function _batch_page(Request $request) {
   +      \Drupal::messenger()->addError(t('The requested batch @batch could not be found.', ['@batch' => $request_id]));
   +      throw new NotFoundHttpException(sprintf('The requested batch %s could not be found.', $request_id));
   

   The message by testing is "The requested batch {ID} could not be found." but the issue summary states that it should be "The requested batch ({ID}) could not be found." - the brackets are missing.

2. +++ b/core/tests/Drupal/FunctionalTests/Core/Batch/BatchNotFoundTest.php
   @@ -0,0 +1,40 @@
   +  public static $modules = array('batch_test');
   

   There's a coding standards error. "Short array syntax must be used to define arrays" - this should be public static $modules = ['batch_test'];

3. The change record, Return 404 if Batch ID does not exist, needs changing to reflect the new text as well.

Back to needs work for these points.

Hi @john-cook,

updated with the point no 1 and 2.

Thank you @andriansyah for working on this.

Points 1 and 2 from #150 have been addressed.

I think the code is done now.

The only thing left is to alter the change record so that it states what is shown.

I've added the "documentation" tag and set back to "Needs work" because of this.

The documentation for the change record has been updated using 'The requested batch ({ID}) could not be found.'

All the points from #150 have now been done.

Marking as RTBC.

Wanted to see what this looked like and since there are no screen shots I added them to the IS.

Two questions 1) should the page be themed, I found it a surprise to get a mostly white screen. And 2) Should the batch id be surrounded by parenthesis or single quotes? I only know that in error message single quotes are used for the variable and I was wondering if it is the same in UI text. I looked at Interface text but didn't see anything. Could have missed it.

I have re-rolled patch #151 on Drupal 8.9.x-dev

Add default theme in BatchNotFoundTest.php class

/**
* {@inheritdoc}
*/
protected $defaultTheme = 'stark';

Hope this will help you.

Re-rolling patch for 8.9.x-dev

+++ b/core/tests/Drupal/FunctionalTests/Core/Batch/BatchNotFoundTest.php
@@ -0,0 +1,45 @@
+  /**
+   * Modules to enable.
+   *
+   * @var array
+   */

This should be {@inheritdoc}

Changed to {@inheritdoc}. Here is the patch.

Here I have made changes as suggested in comment #164.

Thanks for the patch.

Still one more question.

+++ b/core/includes/batch.inc
@@ -42,8 +43,8 @@ function _batch_page(Request $request) {
+      \Drupal::messenger()->addError(t('The requested batch (@batch) could not be found.', ['@batch' => $request_id]));
+      throw new NotFoundHttpException(sprintf('The requested batch %s could not be found.', $request_id));

The error message is slightly different than the exception message. Should they be the same? What is 'correct'?

Since this is making a change visible to the user, lets get a usability review and perhaps that will answer this question too.

Rerolled #166 patch against 9.5.x
Attached patch and reroll diff

Removed needs reroll tag, and fixed Drupal CS issue of patch 174.

Rerolled patch against #174

Rerolled patch against #176 . Fixed custom commands

Ok against 9.4.x and 9.5.x

The patch is still not passing the code checks.

Addressed Drupal CS issue of patch #177.

Tested patch #180 on drupal 10.1.
patch working fine

Adding issue credits for those who changed the shape/course of the patch and/or made issue summary updates

Putting back to needs review because @quietone asked for a usability review in #167

I've pinged the UX team, I think this is ready to go code wise so would be good to get a +1 from the UX team on the wording

Slack thread where I pinged the UX team https://drupal.slack.com/archives/C1AFW2ZPD/p1668983795995769

Usability review

We discussed this issue at #3322499: Drupal Usability Meeting 2022-11-25. That issue has a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @AaronMcHale, @benjifisher, @ravi.shankar, @rkoller, and @simohell.

We considered whether 404 is the correct response code and agreed that it is. See RFC 7231: it does not matter, as we initially thought, whether the batch ID is in the path or the query string.

If we can figure out some common scenarios when this comes up, as in Comment #144, then it would be nice to give a more helpful message. That will require some discussion, so it should get its own issue. We do not want to hold up this one.

The only change we would like to see for this issue is to replace the error message. Instead of "The requested batch (20) could not be found", either of these is slightly shorter and more consistent with other Drupal error messages:

• The batch with ID 20 does not exist.
• No batch with ID 20 exists.

I am setting the status to NW for that change, and to update the screenshots.

I am also removing the "Beta phase evaluation" section from the issue summary and updating the remaining tasks.

@Bhanu951:

Thanks for implementing the changes that the usability team recommended (Comment #187).

I am curious about the "after" screenshot: is the text "Error message" normally visible in the messages area, or did you use browser tools to un-hide it?

I think the usability concerns have been addressed. This issue now needs code review and testing.

I am updating the issue summary:

• Use a numbered list for "remaining tasks".
• "Cross off" the tasks that have been completed.
• Repeat in the "User Interface changes" section that part of this issue is to remove the redirect.
• This does not count as an API change.

@benjifisher :

Thanks for the review.

I am curious about the "after" screenshot: is the text "Error message" normally visible in the messages area, or did you use browser tools to un-hide it?

I haven't used any browser tools. It is being displayed as shown in the image.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Tested on Drupal 10.1.x with a standard install
Confirmed going to /batch?123 I get the "No active batch" error
Applying the fix
Going to /batch?123 I get a 404 page with "No batch with ID 123 exists."

Reviewing the change record and the change being addressed is clearly states.

Looks good

I agree with #193 that this needs a code review.

added patch against #180 in 10.1 version

Hiding patch #197 as it's incomplete and omitting important changes.

Adding to the list #3339883: Employees of Dotsquares are posting mass re-roll patches which are invalid and/or incomplete