This is a kernel-patch follow-up.

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

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pfrenssen’s picture

I have tried my hand at this. I've worked from the 'kernel' branch, and have merged in the latest 8.x from core and rebased the patch against that.

I suppose the same can be done for drupal_access_denied(), replace it with throw new AccessDeniedHttpException();

pfrenssen’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1587850-1-replace_drupal_not_found.patch, failed testing.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
112.09 KB

Here is a version of the patch diffed against 8.x.

Status: Needs review » Needs work

The last submitted patch, 1587850-4-replace_drupal_not_found.patch, failed testing.

pfrenssen’s picture

Tests failed because the NotFoundHttpException() was not included.

pfrenssen’s picture

Status: Needs work » Needs review

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

The last submitted patch, 1587850-6-replace_drupal_not_found.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review

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

The last submitted patch, 1587850-6-replace_drupal_not_found.patch, failed testing.

Crell’s picture

Er. This issue isn't going to work until the kernel patch goes in. I apprecate your enthusiasm but it's not something that we can realistically work on yet. :-)

Niklas Fiekas’s picture

He got the full kernel patch + the patch uploaded for the testbot ;) This is going to be an easy merge when the time comes.

catch’s picture

Priority: Normal » Major
Crell’s picture

Issue tags: +Novice

This should be fairly routine.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
7.44 KB

Here's a re-roll after the big merge.

Niklas Fiekas’s picture

Status: Needs review » Needs work

Thanks chrisdolby, the patch applies.

Looks like some changes got lost, especially fixing references in comments and removing the function itself:

$grep -r "drupal_not_found" core/
./includes/file.inc: * If the file does not exist drupal_not_found() will be returned.
./includes/common.inc: * return MENU_NOT_FOUND instead of calling drupal_not_found(). However,
./includes/common.inc: * bubble up to menu_execute_active_handler() should call drupal_not_found().
./includes/common.inc:function drupal_not_found() {
./includes/common.inc: * can also be called by error conditions such as drupal_not_found(),
./modules/search/search.module:  // for example by drupal_access_denied() or drupal_not_found()
./modules/statistics/statistics.pages.inc: *   drupal_not_found().
./modules/statistics/statistics.pages.inc:    drupal_not_found();
./modules/statistics/statistics.pages.inc: *   drupal_not_found().
./modules/statistics/statistics.admin.inc: *   page was not found, drupal_not_found() is called.

Minor style problem:

+++ b/core/modules/book/book.pages.incundefined
@@ -6,7 +6,7 @@
 use Drupal\node\Node;
-
+use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
/**

There should be a newline between the use statement and the following doxygen.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
11.4 KB

Thanks Niklas, sorry I missed those the first time round!

Here's another patch to remove the function, the last few calls and adjust the comments. I've also fixed the newline issue in book.pages.inc.

Niklas Fiekas’s picture

Excellent, thanks.

Now were on the nit-pick level, really:

+++ b/core/includes/file.incundefined
@@ -1997,7 +1997,7 @@ function file_transfer($uri, $headers) {
+ * If the file does not exist a NotFoundHttpException() will be thrown.

Class names usually have no () behind them.

+++ b/core/modules/search/search.moduleundefined
@@ -1035,7 +1035,7 @@ function search_box($form, &$form_state, $form_id) {
+  // for example by drupal_access_denied()

While we're rerolling it once more anyway, we could add a trailing . to that sentence.

+++ b/core/modules/statistics/statistics.admin.incundefined
@@ -225,7 +227,7 @@ function statistics_top_referrers() {
+ *   page was not found, a NotFoundHttpException() is thrown.

No () here, as well.

+++ b/core/modules/statistics/statistics.pages.incundefined
@@ -5,13 +5,14 @@
+ *   not found, this will throw a NotFoundHttpException().

().

+++ b/core/modules/statistics/statistics.pages.incundefined
@@ -67,8 +68,7 @@ function statistics_node_tracker() {
+ *   not found, this will throw a NotFoundHttpException().

().

After that, I'd say it's good to go.

Anonymous’s picture

I've fixed the comments to remove the () and add a . to the end of the comment for the search_box() function.

Hopefully that should now cover everything!

Niklas Fiekas’s picture

Status: Needs review » Reviewed & tested by the community

Thanks a lot. Assuming tests pass.

pfrenssen’s picture

Status: Reviewed & tested by the community » Needs work

There are some things missing in the latest patch that were included in my patch from #6:

+ * can also be called by error conditions such as drupal_access_denied()
+ * and drupal_site_offline().

This does not respect the 80-character word wrap boundary.

@@ -41,7 +43,7 @@ function contact_site_form($form, &$form_state) {
       drupal_set_message(t('The contact form has not been configured. <a href="@add">Add one or more categories</a> to the form.', array('@add' => url('admin/structure/contact/add'))), 'error');
     }
     else {
-      drupal_not_found();
+      throw new NotFoundHttpException();
       drupal_exit();
     }
   }

drupal_exit() will never be called in this situation. This was addressed in the patch in #6.

@@ -375,7 +377,7 @@ function language_admin_delete_form($form, &$form_state, $language) {
   $languages = language_list();
 
   if (!isset($languages[$langcode])) {
-    drupal_not_found();
+    throw new NotFoundHttpException();
     drupal_exit();
   }

Same here, drupal_exit() should be removed.

There is one additional fix that was not present in #6 though:

--- a/core/modules/search/search.module
+++ b/core/modules/search/search.module
@@ -1035,7 +1035,7 @@ function search_box($form, &$form_state, $form_id) {
 function search_box_form_submit($form, &$form_state) {
   // The search form relies on control of the redirect destination for its
   // functionality, so we override any static destination set in the request,
-  // for example by drupal_access_denied() or drupal_not_found()
+  // for example by drupal_access_denied().
   // (see http://drupal.org/node/292565).
   if (isset($_GET['destination'])) {
     unset($_GET['destination']);

But this needs to respect the 80-character word wrap boundary.

Niklas Fiekas’s picture

Thanks, pfrenssen. Sorry, I didn't catch that.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
11.61 KB

Here is a straight reroll of #6. Additionally I updated the documentation of search_box() as this was missing from my patch as Chris found out. I went one step further and removed the whole line since drupal_access_denied() is gone too now. This is consistent with some of the other documentation changes.

I'm now going over to #1591604: Replace drupal_access_denied() with throw AccessDeniedHttpException and will do the same as there will be conflicts between these two patches as they work on the same lines of documentation.

Niklas Fiekas’s picture

Status: Needs review » Needs work

Thanks, pfrenssen.

Now the only missing thing should be the superflous brackets, as above.

+++ b/core/includes/file.incundefined
@@ -1997,7 +1997,7 @@ function file_transfer($uri, $headers) {
+ * If the file does not exist a NotFoundHttpException() will be thrown.

The class names in the comments shouldn't have a () behind them.

+++ b/core/modules/statistics/statistics.admin.incundefined
@@ -225,7 +227,7 @@ function statistics_top_referrers() {
+ *   page was not found, NotFoundHttpException() is thrown.

().

+++ b/core/modules/statistics/statistics.pages.incundefined
@@ -5,13 +5,15 @@
+ *   NotFoundHttpException().

().

+++ b/core/modules/statistics/statistics.pages.incundefined
@@ -67,8 +69,8 @@ function statistics_node_tracker() {
+ *   NotFoundHttpException();

().

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
11.52 KB

Oh I had not seen that Chris had improved the documentation in my patch. Have included his improvements. Sorry, Chris!

Status: Needs review » Needs work

The last submitted patch, 1587850-24-replace_drupal_not_found.patch, failed testing.

pfrenssen’s picture

Status: Needs work » Needs review

@Niklas, yes thanks! I noticed too, but saw your comment only after I posted the new patch :-/

Niklas Fiekas’s picture

I am getting fatal: corrupt patch at line 295 when I try to apply the latest patch. Let's see what the testbot says. Other than that this should really be everything, now.

pfrenssen’s picture

I should not try to outsmart the patch command. This is better!

Niklas Fiekas’s picture

Status: Needs review » Reviewed & tested by the community

So .... if this passes: Done.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

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.