This is a kernel-patch follow-up.

drupal_access_denied() is now a trivial wrapper around throwing Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException. Let's eliminate the middle man.

All instances of drupal_access_denied() should be replaced with throwing the appropriate exception, or else refactored so that the "denied" logic happens further up in the routing layer where it belongs.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Title: Replace drupal_access_denied() with throw » Replace drupal_access_denied() with throw AccessDeniedHttpException
catch’s picture

Priority: Normal » Major
Crell’s picture

Issue tags: +Novice

This should be fairly routine.

underq’s picture

Status: Active » Needs review
FileSize
5.92 KB

Patch done, need review ;-)

Status: Needs review » Needs work

The last submitted patch, core-replace_drupal_access_denied-1591604-4.patch, failed testing.

underq’s picture

Status: Needs work » Needs review
FileSize
7.66 KB

Add use SymfonyComponentHttpKernelExceptionAccessDeniedHttpException; /code>

Status: Needs review » Needs work

The last submitted patch, core-replace_drupal_access_denied-1591604-6.patch, failed testing.

Albert Volkman’s picture

aspilicious’s picture

Status: Needs work » Needs review
pfrenssen’s picture

Status: Needs review » Needs work

The are mentions of drupal_access_denied() scattered across the documentation, please update these too.

For example, in the documentation for drupal_deliver_page():

/**
 * Delivers a page callback result to the browser in the appropriate format.
 *
 * This function is most commonly called by menu_execute_active_handler(), but
 * can also be called by error conditions such as drupal_not_found(),
 * drupal_access_denied(), and drupal_site_offline().
 */
pfrenssen’s picture

Assigned: Unassigned » pfrenssen

I'll do this, have done the same in #1587850: Replace drupal_not_found() with throw NotFoundHttpException. The patches in these two issues will give conflicts anyway since they are affecting the same lines of documentation.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
12.31 KB

Did the following updates to the patch in #8:

  • Removed the drupal_access_denied() function itself.
  • Updated documentation of drupal_deliver_page(), file_download() and search_box_form_submit().
  • Replaced calls to drupal_acccess_denied() in aggregator_admin_refresh_feed(), comment_approve() and overlay_user_dismiss_message().
  • Removed code that was never going to be executed after throwing the AccessDeniedHttpException in contact_site_form(), contact_personal_form(), menu_delete_menu_page(), menu_item_delete_page()
Niklas Fiekas’s picture

Status: Needs review » Needs work
Issue tags: +Novice, +WSCCI, +symfony, +kernel-followup

The last submitted patch, 1591604-12-core-replace_drupal_access_denied.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
8.63 KB

I've rebased pfrenssen's patch on the latest head, should hopefully test OK now.

aspilicious’s picture

Status: Needs review » Needs work

You forgot to remove the drupal_access_denied() function

Anonymous’s picture

Status: Needs work » Needs review
FileSize
11.17 KB

Sorry, that was the point after all!

There were also some other changes missing. This should cover includes/common.inc, includes/file.inc and modules/aggregator/aggregator.admin.inc, and in the process removing the function.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Did a grep, is rtbc if this come back green!

underq’s picture

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

Rebased chrisdolby patch with remove drupal_access_denied(), edit file_download() comment and add AccessDeniedHttpException() in aggregator_admin_refresh_feed() function.

I hope that's good.

underq’s picture

Oups you are so fast :)

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

They both are correct, but the patch command they use was different, thats why there is a size change. Btw you can't access the first patch anymore because they used the same name while crossposting, so the second one overwrites the first one.

tim.plunkett’s picture

No, one was core_replace and the other core-replace.

The first patch changes the file permissions which is wrong. The second one (#19) is RTBC.

aspilicious’s picture

Woow timplunkett, I didn't notice the subtle permission change in the git headers.
Thank you!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Great. Love the clean-ups. Committed to 8.x. Thanks!

David_Rothstein’s picture

Status: Fixed » Reviewed & tested by the community

This and several other issues look like they weren't actually committed/pushed. So, #19 is still RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed
David_Rothstein’s picture

For anyone who's looking for it, a change notification for this issue was started at http://drupal.org/node/1616360

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