Problem/Motivation

Currently if you try to go to /batch?id=123 you get redirected to the homepage with a message stating "No active batch".

Proposed resolution

Since the batch doesn't exist Drupal should just emit a 404 instead.
Display an error message to the user.

Remaining tasks

  1. Update the error message: see Comment #187.
  2. Update the "after" screenshot in the issue summary.
  3. #193, code review and testing

User interface changes

Error message displayed, "No batch with ID 20 exists."

Before

There is a redirect to the home page:

before-mr-13250

After

There is a 404 response instead of a redirect, and the error message is updated.

after-mr-13250

API changes

None

CommentFileSizeAuthor
#197 1986330-197.patch1.03 KBAkhil Yadav
#190 1986330-after-mr-13250.png147.93 KBBhanu951
#190 1986330-before-mr-13250.png250.07 KBBhanu951
#182 After patch #180.png60.06 KBbebalachandra
#182 Before patch #180.png68.09 KBbebalachandra
#180 interdiff_177-180.txt480 bytesravi.shankar
#180 1986330-180.patch2.21 KBravi.shankar
#178 #1986330.png15.43 KBDrDam
#177 1986330-177.patch2.23 KBimmaculatexavier
#176 interdiff_174-176.txt1.54 KBimmaculatexavier
#176 1986330-176.patch2.23 KBimmaculatexavier
#175 interdiff_174-175.txt553 bytesravi.shankar
#175 1986330-175.patch1.03 KBravi.shankar
#174 reroll_diff_166-174.txt1.96 KBimmaculatexavier
#174 1986330-174.patch1.03 KBimmaculatexavier
#174 1986330-175.patch1.03 KBimmaculatexavier
#166 interdiff_163-166.txt567 bytesravi.shankar
#166 1986330-166.patch2.19 KBravi.shankar
#165 interdiff_163-165.txt2.05 KBSuresh Prabhu Parkala
#165 1986330-165.patch2.23 KBSuresh Prabhu Parkala
#163 interdiff_159-163.txt520 bytesrajeevk
#163 1986330-163.patch2.22 KBrajeevk
#159 1986330-159.patch2.14 KBravi.shankar
#156 Screenshot from 2019-09-11 20-12-45.png9.65 KBquietone
#156 Screenshot from 2019-09-11 20-10-39.png3.42 KBquietone
#151 1986330-147-151-interdiff.txt1.25 KBandriansyah
#151 1986330-151.patch2.14 KBandriansyah
no_valid_batch-404.patch379 bytestimaholt
#6 drupal8.batch-system.1986330-6.patch1.38 KBkasperg
#7 1986330-fix-when-batch-id-doesnt-exist.patch856 bytesmarcelodornelas
#10 1986330-fix-when-batch-id-doesnt-exist-v2.patch866 bytesmarcelodornelas
#11 1986330-fix-when-batch-id-doesnt-exist-v3.patch838 bytesmarcelodornelas
#12 1986330-fix-when-batch-id-doesnt-exist-v3.patch838 bytesmarcelodornelas
#13 1986330-fix-when-batch-id-doesnt-exist-v3.patch838 bytesmarcelodornelas
#14 1986330-fix-when-batch-id-doesnt-exist-v3.patch838 bytesmarcelodornelas
#20 drupal8.batch-system.1986330-20.patch1005 bytesstefank
#24 drupal8.batch-system.1986330-24.patch1.2 KBkasperg
#31 when_batch_id_doesn_t-1986330-31.patch2.38 KBsubhojit777
#37 when_batch_id_doesn_t-1986330-36.patch2.41 KBwheatpenny
#37 interdiff.txt912 byteswheatpenny
#37 tests-only-1986330-36.patch1.21 KBwheatpenny
#39 when_batch_id_doesn_t-1986330-39.patch2.37 KBwheatpenny
#39 interdiff.txt902 byteswheatpenny
#43 when_batch_id_doesn_t-1986330-43.patch2.34 KBsubhojit777
#43 interdiff-1986330-39-43.txt584 bytessubhojit777
#44 Selection_009.png22.77 KBsubhojit777
#46 Selection_010.png129.62 KBsubhojit777
#46 when_batch_id_doesn_t-1986330-46.patch2.93 KBsubhojit777
#46 interdiff-1986330-43-46.txt1.45 KBsubhojit777
#49 when_batch_id_doesn_t-1986330-49.patch2.93 KBsubhojit777
#49 interdiff-1986330-46-49.txt539 bytessubhojit777
#50 failing_patch-1986330-50.patch1.77 KBsubhojit777
#50 interdiff-1986330-49-50.txt1.04 KBsubhojit777
#54 Screenshot.png70.23 KBAkshayKalose
#66 when_batch_id_doesn_t-1986330-66.patch2.92 KBsubhojit777
#66 interdiff-1986330-49-66.txt1.25 KBsubhojit777
#81 when_batch_id_doesn_t-1986330-81.patch2.96 KBrichardcanoe
#83 interdiff.1986330.66.81.txt707 bytesYesCT
#86 when_batch_id_doesn_t_only_test-1986330-86.patch1.11 KBsubhojit777
#86 TL-DR-interdiff-1986330-81-86.txt1.53 KBsubhojit777
#86 when_batch_id_doesn_t-1986330-86.patch2.28 KBsubhojit777
#95 1986330-95.patch2.21 KBshashikant_chauhan
#98 1986330-98.patch2.21 KBshashikant_chauhan
#102 1986330-102.patch2.22 KBnikunjkotecha
#102 interdiff-98-102.txt823 bytesnikunjkotecha
#104 Selection_007.png4.74 KBquietone
#104 Selection_006.png15.86 KBquietone
#111 when_batch_id_doesn_t-1986330-111.patch2.24 KBcebasqueira
#118 when_batch_id_doesn_t-1986330-111_tests_only.patch1.1 KBaron.beal
#118 when_batch_id_doesn_t-1986330-111.patch2.22 KBaron.beal
#125 when_batch_id_doesn_t-1986330-125.patch2.04 KBnikunjkotecha
#126 when_batch_id_doesn_t-1986330-125.patch2.04 KBnikunjkotecha
#132 batch_id_doesnt_exist-1986330-132.patch1.99 KBshashikant_chauhan
#133 Selection_001.png17.64 KBquietone
#141 batch_id_doesnt_exist-1986330-141.patch0 bytesvacho
#141 interdiff_132-141.txt1.69 KBvacho
#142 batch_id_doesnt_exist-1986330-142.patch1.1 KBvacho
#142 interdiff_132-142.txt789 bytesvacho
#143 batch_id_doesnt_exist-1986330-143.patch2.14 KBmangy.fox
#146 batch_id_doesnt_exist-1986330-146.patch6.19 KBsmagdits
#146 1986330-143-146-interdiff.txt1.11 KBsmagdits
#149 batch_id_doesnt_exist-1986330-147.patch2.15 KBsmagdits
#149 1986330-143-147-interdiff.txt714 bytessmagdits

Issue fork drupal-1986330

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

skwashd’s picture

Status: Needs review » Reviewed & tested by the community

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

alexandrezia’s picture

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

caiovlp’s picture

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!

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D7

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...

kasperg’s picture

Assigned: timaholt » kasperg

I'll take a look at this for D8.

kasperg’s picture

Status: Needs work » Needs review
FileSize
1.38 KB

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.
marcelodornelas’s picture

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

valthebald’s picture

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()

valthebald’s picture

Status: Needs review » Needs work

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

marcelodornelas’s picture

Status: Needs work » Needs review
FileSize
866 bytes

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

marcelodornelas’s picture

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

marcelodornelas’s picture

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

marcelodornelas’s picture

This is fixed.

marcelodornelas’s picture

Status: Needs review » Needs work
FileSize
838 bytes

This is fixed.

kasperg’s picture

Status: Needs work » Needs review

#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.

valthebald’s picture

Status: Needs review » Needs work
Issue tags: +Needs followup

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.

mgifford’s picture

Assigned: kasperg » Unassigned
Issue summary: View changes
dawehner’s picture

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.

valthebald’s picture

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

stefank’s picture

Status: Needs work » Needs review
FileSize
1005 bytes

Ok, here is a reroll from #6.

valthebald’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs followup
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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()

tstoeckler’s picture

Status: Needs work » Reviewed & tested by the community
+++ 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.

kasperg’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.2 KB

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

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

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 :-)

kasperg’s picture

No worries. Happy to help.

alexpott’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs issue summary update +Needs tests

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.

subhojit777’s picture

Assigned: Unassigned » subhojit777

Status: Needs work » Needs review
subhojit777’s picture

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.

subhojit777’s picture

Assigned: subhojit777 » Unassigned
FileSize
2.38 KB

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.

moymilo’s picture

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

moymilo’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests
moymilo’s picture

Issue summary: View changes
moymilo’s picture

Issue summary: View changes

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

YesCT’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

wheatpenny’s picture

Status: Needs work » Needs review
FileSize
2.41 KB
912 bytes
1.21 KB

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.

dawehner’s picture

  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.

wheatpenny’s picture

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

YesCT’s picture

Status: Needs review » Needs work

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.

The last submitted patch, 37: tests-only-1986330-36.patch, failed testing.

subhojit777’s picture

Assigned: Unassigned » subhojit777
subhojit777’s picture

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.

subhojit777’s picture

Assigned: subhojit777 » Unassigned
Status: Needs work » Needs review
FileSize
22.77 KB

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

subhojit777’s picture

Assigned: Unassigned » subhojit777

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.

subhojit777’s picture

Assigned: subhojit777 » Unassigned
FileSize
129.62 KB
2.93 KB
1.45 KB

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.

Status: Needs review » Needs work

The last submitted patch, 46: when_batch_id_doesn_t-1986330-46.patch, failed testing.

subhojit777’s picture

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

subhojit777’s picture

Status: Needs work » Needs review
FileSize
2.93 KB
539 bytes

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

subhojit777’s picture

Patch without fix. This patch should fail testing.

Status: Needs review » Needs work

The last submitted patch, 50: failing_patch-1986330-50.patch, failed testing.

subhojit777’s picture

Status: Needs work » Needs review

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

subhojit777’s picture

AkshayKalose’s picture

FileSize
70.23 KB

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

subhojit777’s picture

Status: Needs review » Needs work
subhojit777’s picture

Assigned: Unassigned » subhojit777
subhojit777’s picture

Assigned: subhojit777 » Unassigned
Status: Needs work » Needs review
+++ 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

subhojit777’s picture

*bump* anybody wants to do review?

valthebald’s picture

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

DevElCuy’s picture

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.

DevElCuy’s picture

Issue summary: View changes
DevElCuy’s picture

Issue summary: View changes
DevElCuy’s picture

Status: Needs review » Needs work

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.

DevElCuy’s picture

Issue summary: View changes
subhojit777’s picture

Assigned: Unassigned » subhojit777
subhojit777’s picture

Status: Needs work » Needs review
FileSize
2.92 KB
1.25 KB

I will add the beta changes now.

subhojit777’s picture

Assigned: subhojit777 » Unassigned

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

Status: Needs review » Needs work

The last submitted patch, 66: when_batch_id_doesn_t-1986330-66.patch, failed testing.

subhojit777’s picture

Status: Needs work » Needs review
vitorsdcs’s picture

Status: Needs review » Needs work

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.

YesCT’s picture

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?

DevElCuy’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: +LatinAmerica2015

@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!

DevElCuy’s picture

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.

  • webchick committed 30432a0 on 8.0.x
    Issue #1986330 by subhojit777, marcelodornelas, wheatpenny, kasperg,...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

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 committed 08f0207 on 8.0.x
    Revert "Issue #1986330 by subhojit777, marcelodornelas, wheatpenny,...
xjm’s picture

Status: Fixed » Needs work

@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();
xjm’s picture

+++ 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.

xjm’s picture

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'
richardcanoe’s picture

Status: Needs work » Needs review
FileSize
2.96 KB

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.

YesCT’s picture

Issue tags: -Novice

not novice

YesCT’s picture

Status: Needs review » Needs work
FileSize
707 bytes

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.

subhojit777’s picture

Assigned: Unassigned » subhojit777

Lets see what I can do.

subhojit777’s picture

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.

subhojit777’s picture

Assigned: subhojit777 » Unassigned
Status: Needs work » Needs review
FileSize
1.11 KB
1.53 KB
2.28 KB

I have re-wrote the test using batch_test module .

The last submitted patch, 86: when_batch_id_doesn_t_only_test-1986330-86.patch, failed testing.

  • webchick committed 08f0207 on 8.1.x
    Revert "Issue #1986330 by subhojit777, marcelodornelas, wheatpenny,...
  • webchick committed 30432a0 on 8.1.x
    Issue #1986330 by subhojit777, marcelodornelas, wheatpenny, kasperg,...

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed 08f0207 on 8.3.x
    Revert "Issue #1986330 by subhojit777, marcelodornelas, wheatpenny,...
  • webchick committed 30432a0 on 8.3.x
    Issue #1986330 by subhojit777, marcelodornelas, wheatpenny, kasperg,...

  • webchick committed 08f0207 on 8.3.x
    Revert "Issue #1986330 by subhojit777, marcelodornelas, wheatpenny,...
  • webchick committed 30432a0 on 8.3.x
    Issue #1986330 by subhojit777, marcelodornelas, wheatpenny, kasperg,...

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll, +Novice

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

shashikant_chauhan’s picture

Status: Needs work » Needs review
FileSize
2.21 KB

Adding patch.

shashikant_chauhan’s picture

Issue tags: -Needs reroll

Status: Needs review » Needs work

The last submitted patch, 95: 1986330-95.patch, failed testing.

shashikant_chauhan’s picture

Status: Needs work » Needs review
FileSize
2.21 KB

Status: Needs review » Needs work

The last submitted patch, 98: 1986330-98.patch, failed testing.

quietone’s picture

@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).

nikunjkotecha’s picture

Assigned: Unassigned » nikunjkotecha
Issue tags: +#dcdelhi
nikunjkotecha’s picture

Assigned: nikunjkotecha » Unassigned
Status: Needs work » Needs review
FileSize
2.22 KB
823 bytes

Updated patches to use SafeMarkup instead of String.

nikunjkotecha’s picture

Issue tags: -Needs backport to D7

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

quietone’s picture

Issue summary: View changes
FileSize
4.74 KB
15.86 KB

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?

valthebald’s picture

Version: 8.2.x-dev » 8.3.x-dev

Bumping version

mangy.fox’s picture

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

mangy.fox’s picture

Status: Needs review » Reviewed & tested by the community

Also re-ran tests against 8.3.x.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

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

mangy.fox’s picture

Issue tags: -Needs change record

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

navneet0693’s picture

+++ 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

cebasqueira’s picture

Status: Needs work » Needs review
Issue tags: +ciandt-contrib
FileSize
2.24 KB

Status: Needs review » Needs work

The last submitted patch, 111: when_batch_id_doesn_t-1986330-111.patch, failed testing.

cebasqueira’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Needs work
+++ 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

  • webchick committed 08f0207 on 8.4.x
    Revert "Issue #1986330 by subhojit777, marcelodornelas, wheatpenny,...
  • webchick committed 30432a0 on 8.4.x
    Issue #1986330 by subhojit777, marcelodornelas, wheatpenny, kasperg,...

  • webchick committed 08f0207 on 8.4.x
    Revert "Issue #1986330 by subhojit777, marcelodornelas, wheatpenny,...
  • webchick committed 30432a0 on 8.4.x
    Issue #1986330 by subhojit777, marcelodornelas, wheatpenny, kasperg,...

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

aron.beal’s picture

Rerolled test into BrowserTestBase, passes.

navneet0693’s picture

Status: Patch (to be ported) » Needs review

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.

The last submitted patch, 118: when_batch_id_doesn_t-1986330-111_tests_only.patch, failed testing.

navneet0693’s picture

Status: Needs review » Reviewed & tested by the community

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

aron.beal’s picture

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.

navneet0693’s picture

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

xjm’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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!

nikunjkotecha’s picture

Updated patch to use sprintf and removed unwanted includes.

nikunjkotecha’s picture

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.

nikunjkotecha’s picture

navneet0693’s picture

Status: Needs work » Needs review

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

nikunjkotecha’s picture

Thanks @Navneet.

quietone’s picture

Issue summary: View changes

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.

shashikant_chauhan’s picture

Assigned: Unassigned » shashikant_chauhan
Issue tags: -LatinAmerica2015, -#dcdelhi +DrupalMumbaiCodeSprint16

Cleaning the tags. We are working on this issue.

shashikant_chauhan’s picture

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.

quietone’s picture

Status: Needs review » Needs work
FileSize
17.64 KB

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.

shashikant_chauhan’s picture

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.

nikunjkotecha’s picture

@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.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

vacho’s picture

Issue tags: +Needs reroll
vacho’s picture

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.

vacho’s picture

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.

mangy.fox’s picture

Assigned: shashikant_chauhan » Unassigned
Status: Needs work » Needs review
FileSize
2.14 KB

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

Also updated the test to meet current standards.

selva.swamy@gmail.com’s picture

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.

John Cook’s picture

Status: Needs review » Needs work

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])

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.

smagdits’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.19 KB
1.11 KB

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

John Cook’s picture

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.

John Cook’s picture

Status: Needs review » Needs work
smagdits’s picture

Status: Needs work » Needs review
FileSize
714 bytes
2.15 KB

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.

John Cook’s picture

Status: Needs review » Needs work

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.

andriansyah’s picture

Hi @john-cook,

updated with the point no 1 and 2.

andriansyah’s picture

Status: Needs work » Needs review
John Cook’s picture

Status: Needs review » Needs work
Issue tags: +Documentation

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.

andriansyah’s picture

Status: Needs work » Needs review

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

John Cook’s picture

Status: Needs review » Reviewed & tested by the community

All the points from #150 have now been done.

Marking as RTBC.

quietone’s picture

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.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 151: 1986330-151.patch, failed testing. View results

ravi.shankar’s picture

ravi.shankar’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 159: 1986330-159.patch, failed testing. View results

AkashKumar07’s picture

Add default theme in BatchNotFoundTest.php class

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

Hope this will help you.

rajeevk’s picture

Status: Needs work » Needs review
FileSize
2.22 KB
520 bytes

Re-rolling patch for 8.9.x-dev

quietone’s picture

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

This should be {@inheritdoc}

Suresh Prabhu Parkala’s picture

Status: Needs work » Needs review
FileSize
2.23 KB
2.05 KB

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

ravi.shankar’s picture

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

quietone’s picture

Issue summary: View changes
Issue tags: -Novice +Needs usability review

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.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

larowlan’s picture

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

immaculatexavier’s picture

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

ravi.shankar’s picture

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

immaculatexavier’s picture

Rerolled patch against #174

immaculatexavier’s picture

Rerolled patch against #176 . Fixed custom commands

DrDam’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
15.43 KB

Ok against 9.4.x and 9.5.x

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

The patch is still not passing the code checks.

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
2.21 KB
480 bytes

Addressed Drupal CS issue of patch #177.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

bebalachandra’s picture

FileSize
68.09 KB
60.06 KB

Tested patch #180 on drupal 10.1.
patch working fine

bebalachandra’s picture

Status: Needs review » Reviewed & tested by the community
larowlan’s picture

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

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

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

larowlan’s picture

benjifisher’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs usability review +Needs issue summary update

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 made their first commit to this issue’s fork.

Bhanu951’s picture

FileSize
250.07 KB
147.93 KB
Bhanu951’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
Bhanu951’s picture

Issue summary: View changes
benjifisher’s picture

Issue summary: View changes

@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.
Bhanu951’s picture

@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.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

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

quietone’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

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

Akhil Yadav’s picture

added patch against #180 in 10.1 version

BramDriesen’s picture

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

quietone’s picture

@Akhil Yadav, It is expected that contributors read the issue before making comments etc. The MR is against 10.1.x. I am removing credit.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.