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.Since generating a 404 page calls drupal_get_messages(), if an HTML page contains any broken resources / images the hidden 404 will swallow the messages and prevent them from displaying on the page.
This is actually not new behavior, I tested D6 and it works the same way. What is new is that the rendering system seems to invoke template_preprocess_page much earlier in the rendering process, which means that if you are calling e.g. dpm() in a template file the output gets deferred to a subsequent request. If this is a 404 you'll never see the message.
Of course our live sites never have 404's :-) but it's quite common on a dev site.
I'm not sure what the best solution is - it seems ideally drupal_get_messages would get called late like D6, but if this isn't possible we should consider suppressing messages on 404.
| Comment | File | Size | Author |
|---|---|---|---|
| #34 | interdiff-885896-30-34.txt | 1.27 KB | yogeshmpawar |
| #34 | 404_pages_swallow-885896-34.patch | 4.39 KB | yogeshmpawar |
| #30 | 404_pages_swallow-885896-26-reroll.patch | 4.38 KB | joelpittet |
| #26 | 404_pages_swallow-885896-26.patch | 4.39 KB | wturrell |
| #19 | 885896-suppress-404-messages-19.patch | 4.24 KB | kim.pepper |
Comments
Comment #1
grendzy CreditAttribution: grendzy commentedComment #2
Caligan CreditAttribution: Caligan commentedDuplicate patch in updated git format (prior is -p0).
Comment #4
Caligan CreditAttribution: Caligan commentedComment #5
Caligan CreditAttribution: Caligan commentedSame patch rolled for D8.
Comment #6
BrockBoland CreditAttribution: BrockBoland commentedComment #7
BrockBoland CreditAttribution: BrockBoland commentedRe-rolled for new core file structure, but still waiting on steps to reproduce this issue. This will also need a test case.
Comment #8
jemond CreditAttribution: jemond commentedAttached is a simple module to reproduce this issue.
Steps to reproduce:
Expected to see: The server time from the previous request in in the Drupal messages area
What I saw: No message
Comment #9
jemond CreditAttribution: jemond commentedComment #10
BrockBoland CreditAttribution: BrockBoland commentedjemond's demo module will need to be rolled into a test case, but I can't find where
drupal_set_message()actually gets tested. Maybe it's not?Comment #11
jemond CreditAttribution: jemond commentedThe patch in #7 does not appear to fix the issue in Drupal 8. I need to look further into this.
Comment #12
danquah CreditAttribution: danquah commentedThis patch should fix the problem in Drupal 8. The logic has changed somewhat in D8 so the patch in #7 did not apply. The patch includes a testcase that uses the module suggested in #8 to emit test-messages which is then tested for in the testcase.
The testcase is placed under system as messages did not seem to have a testcase. It might be an idea to do a new issue for creating test-cases for messages.
Comment #13
YesCT CreditAttribution: YesCT commentedjust a quick standards review.
standards have changed a little. Now it's
Contains \Drupal\...
no doc block needed on getInfo http://drupal.org/node/325974
or setUp.
Also, setUp should be public.
third person verb like:
Tests that
all comments need to wrap at 80 chars. http://drupal.org/node/1354
Comment #14
kim.pepperRe-rolled against head and applied code style fixes as per #13
Comment #15
sunComment #16
kim.pepper14: suppress-404-messages-885896-14.patch queued for re-testing.
Comment #18
kim.pepperComment #19
kim.pepperRe-roll.
Comment #22
joelpittetRe-rolled. Tests are the same but the 4xx controller moved. Added to each method
Comment #26
wturrell CreditAttribution: wturrell as a volunteer commentedNB: first day using SimpleTest. Please tell me where I've screwed up.
I'm unclear if this issue / the previous tests are still current.
I've updated them for D8 anyway.
Was confused by the first one - I tried it manually and the session message is displayed immediately, despite "message is set in a preprocess_node hook and wont make it in this page render". Then I thought, well that's just because I'm doing it through the web UI and I'm redirected, and it won't happen when the test runs, but it appears to:
(clearly if the message is shown in the first of these three, it's no surprise the last test fails too.)
Patch changes:
- move test to /core/modules/system/src/Tests/System
- add the @group annotation
- randomName(10) becomes randomString(10)
- $node->did becomes $node->id()
- phpcs fixes
- update test module info file to YAML for D8
Comment #28
joelpittet@wturrell sorry for the late response.
We try to avoid doing phpcs fixes because they muddy the patch for reviewers to tell which changes are pertinent to the fix and which ones are just cleanup. Most clean-up gets posted in a separate or follow-up issue.
Test results are showing that maybe it's still getting swollowed?
Comment #29
joelpittetA quick re-roll due to a coding standards change.
#2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase
Comment #30
joelpittetSorry, my phpcbf didn't run and didn't tell me.
Comment #31
joelpittetComment #34
yogeshmpawarStraight reroll to remove coding standard issues related to this patch & also added interdiff.
Comment #36
joelpittetThank you for rerolling:) Yeah the tests seem to show stuff is still not quite right with the patch.
Comment #37
wturrell CreditAttribution: wturrell as a volunteer commented@joelpittet thanks.
See my comments in #26 - I'm aware the test is failing, could do with another pair of eyes on the code / clarity on what the correct sequence should be.
(I realise you only rerolled the previous patch…)
Comment #39
joelpittetWe shouldn't need to be logged in for this test?