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

Comments

mikeytown2 created an issue. See original summary.

mikeytown2’s picture

Status: Active » Needs review
StatusFileSize
new408 bytes
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
StatusFileSize
new185.38 KB
new222.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

Status: Needs work » Needs review
StatusFileSize
new1.49 KB
new1.89 KB

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

Issue tags: -Needs tests
StatusFileSize
new1013 bytes
new1.39 KB

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.

poker10’s picture

StatusFileSize
new1013 bytes
new1.39 KB
new2.11 KB

Thanks @Kristen Pol for pointing this out. It seems like I made an unintentional mistake here - I did not mean to RTBC my own patch. Reuploading the patch from #6 again with an interdiff, which was missing here.

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.