Replace drupal_not_found() with throw NotFoundHttpException

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

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

Tests failed because the NotFoundHttpException() was not included.

#6: 1587850-6-replace_drupal_not_found.patch queued for re-testing.

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

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.

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.

Thanks a lot. Assuming tests pass.

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.

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.

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

().

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

@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.

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

So .... if this passes: Done.

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