Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#29 | 1587850-29-replace_drupal_not_found.patch | 11.52 KB | pfrenssen |
#25 | 1587850-24-replace_drupal_not_found.patch | 11.52 KB | pfrenssen |
#23 | 1587850-23-replace_drupal_not_found.patch | 11.61 KB | pfrenssen |
#19 | core_replace_drupal_not_found-1587850-19.patch | 11.4 KB | Anonymous (not verified) |
#17 | core_replace_drupal_not_found-1587850-17.patch | 11.4 KB | Anonymous (not verified) |
Comments
Comment #1
pfrenssenI 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();
Comment #2
pfrenssenComment #4
pfrenssenHere is a version of the patch diffed against 8.x.
Comment #6
pfrenssenTests failed because the NotFoundHttpException() was not included.
Comment #7
pfrenssenComment #9
Niklas Fiekas CreditAttribution: Niklas Fiekas commented#6: 1587850-6-replace_drupal_not_found.patch queued for re-testing.
Comment #11
Crell CreditAttribution: Crell commentedEr. 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. :-)
Comment #12
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedHe got the full kernel patch + the patch uploaded for the testbot ;) This is going to be an easy merge when the time comes.
Comment #13
catchComment #14
Crell CreditAttribution: Crell commentedThis should be fairly routine.
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedHere's a re-roll after the big merge.
Comment #16
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks chrisdolby, the patch applies.
Looks like some changes got lost, especially fixing references in comments and removing the function itself:
Minor style problem:
There should be a newline between the use statement and the following doxygen.
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks 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.
Comment #18
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedExcellent, thanks.
Now were on the nit-pick level, really:
Class names usually have no () behind them.
While we're rerolling it once more anyway, we could add a trailing
.
to that sentence.No () here, as well.
().
().
After that, I'd say it's good to go.
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedI'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!
Comment #20
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks a lot. Assuming tests pass.
Comment #21
pfrenssenThere are some things missing in the latest patch that were included in my patch from #6:
This does not respect the 80-character word wrap boundary.
drupal_exit() will never be called in this situation. This was addressed in the patch in #6.
Same here, drupal_exit() should be removed.
There is one additional fix that was not present in #6 though:
But this needs to respect the 80-character word wrap boundary.
Comment #22
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks, pfrenssen. Sorry, I didn't catch that.
Comment #23
pfrenssenHere 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.
Comment #24
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks, pfrenssen.
Now the only missing thing should be the superflous brackets, as above.
The class names in the comments shouldn't have a () behind them.
().
().
().
Comment #25
pfrenssenOh I had not seen that Chris had improved the documentation in my patch. Have included his improvements. Sorry, Chris!
Comment #27
pfrenssen@Niklas, yes thanks! I noticed too, but saw your comment only after I posted the new patch :-/
Comment #28
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedI 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.Comment #29
pfrenssenI should not try to outsmart the patch command. This is better!
Comment #30
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedSo .... if this passes: Done.
Comment #31
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #32
David_Rothstein CreditAttribution: David_Rothstein commentedFor anyone who's looking for it, a change notification for this issue was started at http://drupal.org/node/1616360