Problem/Motivation

go to /batch and you'll get a 403 and a php warning:

Cannot modify header information - headers already sent by (output started at /var/www/html/includes/common.inc)

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#10 interdiff_5-6.txt2.11 KBpoker10
#10 3138348-10.patch1.39 KBpoker10
#10 3138348-10_test-only.patch1013 bytespoker10
#6 3138348-6.patch1.39 KBpoker10
#6 3138348-6_test-only.patch1013 bytespoker10
#5 drupal-batch-403-warning-3138348-5.patch1.89 KBawset
#5 interdiff_2_5.txt1.49 KBawset
#4 Screen Shot 2022-01-10 at 3.29.00 PM.png222.46 KBKristen Pol
#4 Screen Shot 2022-01-10 at 3.26.25 PM.png185.38 KBKristen Pol
#2 drupal-batch-403-warning-3138348-2.patch408 bytesmikeytown2
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeytown2 created an issue. See original summary.

mikeytown2’s picture

jungle’s picture

Issue summary: View changes
Issue tags: +Needs tests, +Bug Smash Initiative

@mikeytown2, thanks for filing this.

Applying IS template, tagging "Needs tests" and "Bug Smash Initiative".

Kristen Pol’s picture

Status: Needs review » Needs work
FileSize
185.38 KB
222.46 KB

Thanks for the issue and patch. I've tested as follows:

  1. Installed Drupal 7 dev
  2. Went to /batch
  3. Noted the error in the logs (same as in issue summary)
  4. Applied the patch
  5. Went to /batch
  6. Noted the error in the logs (just shows 403s as it should)

Moving back to "needs work" for tests.

awset’s picture

Hi All,

thanks for the good work.

Adding a test for the patch provided by mikeytown2.
I also added the interdiff for the same.

this need a review.

thanks

poker10’s picture

Thanks @awangsetyawan for the test. I have tweaked it a little bit to remove the need of the new method (assertNoLogMessage()), so we can keep the code cleaner.

Adding test-only patch and regular patch to see if testbot will be happy. If yes, I think this can be set as RTBC, as the patch is trivial and it is confirmed also by manual testing.

The last submitted patch, 6: 3138348-6_test-only.patch, failed testing. View results

poker10’s picture

Status: Needs review » Reviewed & tested by the community

As I have mentioned in #6, I am setting this as RTBC, because patch was unchanged and confirmed that is is working before. Tests are passing / failing as expected too.

Kristen Pol’s picture

Status: Reviewed & tested by the community » Needs review

@poker10 Thanks for the updated patch. Please create an interdiff when making changes:

https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...

Since you updated the patch, you can't mark this RTBC so moving back to Needs review.

Kristen Pol’s picture

Thanks @poker10 :)

The last submitted patch, 10: 3138348-10_test-only.patch, failed testing. View results

mcdruid’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +RTBM

There are several other places in core where we call drupal_exit() after drupal_access_denied() for example:

modules/user/user.pages.inc:271:      drupal_access_denied();
modules/user/user.pages.inc-272-      drupal_exit();
--
modules/contact/contact.pages.inc:23:    drupal_access_denied();
modules/contact/contact.pages.inc-24-    drupal_exit();
--
modules/contact/contact.pages.inc:191:    drupal_access_denied();
modules/contact/contact.pages.inc-192-    drupal_exit();

Patch looks good, thanks!

Kristen Pol’s picture

What’s RTBM? Curious:)

mcdruid’s picture

RTBM is Ready To Be Merged :) so approved by one maintainer for another to commit.

It's mentioned in #3093258: [policy, no patch] Proposed Drupal 7 contributor / maintainer workflow which never really got beyond a draft, but we've been following it pretty much for a while in D7.

Kristen Pol’s picture

Oh! Very cool. I wasn’t familiar. Thanks:)

  • poker10 committed 40d1dbb on 7.x
    Issue #3138348 by poker10, awset, mikeytown2: system_batch_page() should...
poker10’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -RTBM

Thanks everyone who contributed!

Status: Fixed » Closed (fixed)

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