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.

Files: 
CommentFileSizeAuthor
#29 1587850-29-replace_drupal_not_found.patch11.52 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 36,686 pass(es).
[ View ]
#25 1587850-24-replace_drupal_not_found.patch11.52 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1587850-24-replace_drupal_not_found.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 1587850-23-replace_drupal_not_found.patch11.61 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 36,679 pass(es).
[ View ]
#19 core_replace_drupal_not_found-1587850-19.patch11.4 KBchrisdolby
PASSED: [[SimpleTest]]: [MySQL] 36,680 pass(es).
[ View ]
#17 core_replace_drupal_not_found-1587850-17.patch11.4 KBchrisdolby
PASSED: [[SimpleTest]]: [MySQL] 36,682 pass(es).
[ View ]
#6 1587850-6-replace_drupal_not_found_interdiff.patch12.19 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1587850-6-replace_drupal_not_found_interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#6 1587850-6-replace_drupal_not_found.patch113.95 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] 36,254 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#4 1587850-4-replace_drupal_not_found.patch112.09 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] 36,595 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#1 1587850-1-replace_drupal_not_found.patch10.16 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1587850-1-replace_drupal_not_found.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

StatusFileSize
new10.16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1587850-1-replace_drupal_not_found.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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();

Status:Active» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new112.09 KB
FAILED: [[SimpleTest]]: [MySQL] 36,595 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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.

StatusFileSize
new113.95 KB
FAILED: [[SimpleTest]]: [MySQL] 36,254 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
new12.19 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1587850-6-replace_drupal_not_found_interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Tests failed because the NotFoundHttpException() was not included.

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.

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.

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. :-)

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

Priority:Normal» Major

Issue tags:+Novice

This should be fairly routine.

Status:Needs work» Needs review
StatusFileSize
new7.44 KB
PASSED: [[SimpleTest]]: [MySQL] 36,671 pass(es).
[ View ]

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

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.

Status:Needs work» Needs review
StatusFileSize
new11.4 KB
PASSED: [[SimpleTest]]: [MySQL] 36,682 pass(es).
[ View ]

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.

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.

StatusFileSize
new11.4 KB
PASSED: [[SimpleTest]]: [MySQL] 36,680 pass(es).
[ View ]

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!

Status:Needs review» Reviewed & tested by the community

Thanks a lot. Assuming tests pass.

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.

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

Status:Needs work» Needs review
StatusFileSize
new11.61 KB
PASSED: [[SimpleTest]]: [MySQL] 36,679 pass(es).
[ View ]

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.

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();

().

Status:Needs work» Needs review
StatusFileSize
new11.52 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1587850-24-replace_drupal_not_found.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs work» Needs review

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

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.

StatusFileSize
new11.52 KB
PASSED: [[SimpleTest]]: [MySQL] 36,686 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

So .... if this passes: Done.

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks!

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.