Follow-up to #1986330: When Batch ID doesn't exist, Drupal should emit a 404

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

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Usability Bug, because the message "No active batch" is not clear, since no batch is ran and it does not exists. Response must help figure out what is going on actually.
Issue priority Normal, because this is a self-contained bug and does not impact the overall functionality of the software.
Unfrozen changes Not unfrozen because it changes automated tests and needs a bug fix.
Prioritized changes Prioritized, the main goal of this issue is a bug fix, usability and user experience improvement.
Disruption Impact on improving usability is fair. Also it doesn't disrupt core API because it is internal to batch API only.

Proposed resolution

Since the batch doesn't exist Drupal should just emit a 404 instead

User interface changes

N/A

API changes

If you try to go to /batch?id=123 Drupal emit a 404.

CommentFileSizeAuthor
#19 d7_when_batch_id-2826866-19.patch520 bytescebasqueira
#14 Screen Shot 2017-01-12 at 12.23.55 PM.png112.91 KBnikunjkotecha
#10 2826866-10.patch519 bytesnikunjkotecha
#6 d7_when_batch_id-2826866-6.patch585 byteslomasr
#3 interdiff-d7_when_batch_id-2826866-3.txt532 bytesyago elias
#3 d7_when_batch_id-2826866-3.patch677 bytesyago elias
#2 2826866-2.patch401 bytesnikunjkotecha

Comments

nikunjkotecha created an issue. See original summary.

nikunjkotecha’s picture

nikunjkotecha
he/him
Primary language Gujarati
Location India, Gujarat, Rajkot
commented
Status: Needs work » Needs review
StatusFileSize
new 2826866-2.patch401 bytes

Adding patch for D7.

yago elias’s picture

yago elias commented
Status: Needs review » Needs work
StatusFileSize
new d7_when_batch_id-2826866-3.patch677 bytes
new interdiff-d7_when_batch_id-2826866-3.txt532 bytes

We should add a message log to the dblog system.

yago elias’s picture

yago elias commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: d7_when_batch_id-2826866-3.patch, failed testing.

lomasr’s picture

lomasr commented
StatusFileSize
new d7_when_batch_id-2826866-6.patch585 bytes

Adding a patch for Text change.Please review.

lomasr’s picture

lomasr commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: d7_when_batch_id-2826866-6.patch, failed testing.

nikunjkotecha’s picture

nikunjkotecha
he/him
Primary language Gujarati
Location India, Gujarat, Rajkot
commented
Assigned: Unassigned » nikunjkotecha
nikunjkotecha’s picture

nikunjkotecha
he/him
Primary language Gujarati
Location India, Gujarat, Rajkot
commented
StatusFileSize
new 2826866-10.patch519 bytes

Updated patch to include the message before drupal_exit().

nikunjkotecha’s picture

nikunjkotecha
he/him
Primary language Gujarati
Location India, Gujarat, Rajkot
commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: 2826866-10.patch, failed testing.

nikunjkotecha’s picture

nikunjkotecha
he/him
Primary language Gujarati
Location India, Gujarat, Rajkot
commented
Status: Needs work » Needs review

I'm not sure what's the error for fail as it shows nothing in error and it passed in simplytest.me for me.

nikunjkotecha’s picture

nikunjkotecha
he/him
Primary language Gujarati
Location India, Gujarat, Rajkot
commented
StatusFileSize
new Screen Shot 2017-01-12 at 12.23.55 PM.png112.91 KB

screenshot attached from simplytest.me

Status: Needs review » Needs work

The last submitted patch, 10: 2826866-10.patch, failed testing.

ajits’s picture

ajits
he/him
Primary language Marathi
Location India
commented 
+++ b/includes/batch.inc
@@ -51,8 +51,9 @@ function _batch_page() {
+      watchdog('Batch', '@batch_id is not a valid batch id.', array('@batch_id' => $_REQUEST['id']), WATCHDOG_ERROR);

Can we get a better message? This sounds very technical.
How about

Trying to execute an invalid batch. ID : @batch_id

navneet0693’s picture

navneet0693 commented

Agreed with comment in #16

navneet0693’s picture

navneet0693 commented 
+++ b/includes/batch.inc
@@ -51,8 +51,9 @@ function _batch_page() {
+      watchdog('Batch', '@batch_id is not a valid batch id.', array('@batch_id' => $_REQUEST['id']), WATCHDOG_ERROR);

It's just a suggest if we use % instead of @

cebasqueira’s picture

cebasqueira commented
Status: Needs work » Needs review
StatusFileSize
new d7_when_batch_id-2826866-19.patch520 bytes
dinesh18’s picture

dinesh18 commented

Verified and working as expected.

cebasqueira’s picture

cebasqueira commented

Dinesh18 please change the status to RTBC

navneet0693’s picture

navneet0693 commented
Status: Needs review » Reviewed & tested by the community
stefan.r’s picture

stefan.r commented

How does this work in Drupal 8? If it has the same issue, this should be fixed there first.

David_Rothstein’s picture

David_Rothstein commented
Status: Reviewed & tested by the community » Needs work

The Drupal 8 issue is the parent of this one (#1986330: When Batch ID doesn't exist, Drupal should emit a 404). Looks like it was just marked RTBC, but the two patches are not in sync. They use different wording, and (especially) this one is missing the automated test.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.