Problem/Motivation

We want to remove the process layer to simplify the theme system.

Proposed resolution

Move building of render array for 'messages' from template_process_page() to template_preprocess_page(). The call to drupal_get_messages() still happens just before the messages are output so the behaviour is identical and the same messages are caught and displayed.

Remaining tasks

n/a

User interface changes

None

API changes

None

#1843650: Remove the process layer (hook_process and hook_process_HOOK)
#1843798: [meta] Refactor Render API to be OO
#1589968: Move Title from process to preprocess, remove template_process_page()
#1939082: Convert theme_status_messages() to Twig

CommentFileSizeAuthor
#15 1843678-14.patch1020 bytesstar-szr
#15 interdiff.txt659 bytesstar-szr
#13 1843678-13.patch906 bytesstar-szr
#13 interdiff.txt545 bytesstar-szr
#8 core-make_messages_renderable-1843678-6.patch962 bytesjenlampton
#6 core-make_messages_renderable-1843678-6.patch962 bytesjenlampton
#1 0001-1843678-Make-messages-renderable.patch4.11 KBbdragon

Comments

bdragon’s picture

bdragon commented
Status: Active » Needs review
StatusFileSize
new 0001-1843678-Make-messages-renderable.patch4.11 KB

OK, so I don't quite like the way I am doing this, we might want to make deeper changes to the structure of messages.

Also I don't know how much this affects which messages land on which page load. It seems to me that *any* change in how this runs causes behavioral changes.

sbudker1’s picture

sbudker1 commented
Status: Needs review » Reviewed & tested by the community

Applying the patch was successful and the messages remained the same! During the testing I did not see any behavioral changes.

sbudker1’s picture

sbudker1 commented
Issue summary: View changes

renderable meta

star-szr’s picture

star-szr
he/him
Primary language English
commented

This patch is very close to #1939082: Convert theme_status_messages() to Twig, should these two issues be merged? The templates look a bit different but the overall changes are very similar.

jenlampton’s picture

jenlampton
she / her
commented

Hm. Well, this issue was marked as a blocker for #1843650: Remove the process layer (hook_process and hook_process_HOOK) but it looks like all that's in this patch is a theme function to template conversion. Is there more that needs to be done to messages to turn them into a proper rendarable?

If so, we should remove the theme function -> template file conversion from this issue (since that's what #1939082: Convert theme_status_messages() to Twig one is for) and leave this issue for the refactoring.

If not, we should close this issue and mark it as a duplicate of #1939082: Convert theme_status_messages() to Twig

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Bumping back to needs review so we can discuss this.

jenlampton’s picture

jenlampton
she / her
commented
StatusFileSize
new core-make_messages_renderable-1843678-6.patch962 bytes

After a quick discussion with @joelpittet we decided to remove the conversion from this issue.
re-rolling without the conversion.

jenlampton’s picture

jenlampton
she / her
commented
Title: Make Messages renderable » Make Messages renderable (&remove from process)
Status: Needs review » Needs work

Maybe we can do the same thing here as we did in #1843668: Move building of breadcrumb render array from template_process_page() to template_preprocess_page()
And also use this issue to get them out of process :)

jenlampton’s picture

jenlampton
she / her
commented
Title: Make Messages renderable (&remove from process) » Make Messages renderable (& remove from process)
Status: Needs work » Needs review
StatusFileSize
new core-make_messages_renderable-1843678-6.patch962 bytes

maybe something like this?

jenlampton’s picture

jenlampton
she / her
commented
Status: Needs review » Needs work

wrong patch. :/ also, rethinking this...

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs work » Needs review

#8: core-make_messages_renderable-1843678-6.patch queued for re-testing.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Issue summary: View changes

Add related Twig conversion

star-szr’s picture

star-szr
he/him
Primary language English
commented

Maybe I'm missing something but because 'messages' is already a render array it seems to me that we can just move it to preprocess without any side effects. It's already renderable. The actual drupal_get_messages() call will remain in the theme function, or with #1939082: Convert theme_status_messages() to Twig, in the preprocess for the template file.

jenlampton’s picture

jenlampton
she / her
commented

okay, so the current patch removes from process and adds to preprocess. so we should pass tests here (YAY!) and then just need a quick review. :)

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new interdiff.txt545 bytes
new 1843678-13.patch906 bytes

Just updating the comment to reflect what is actually happening. We aren't populating messages, just setting up a render array for later rendering. I think this is ready to go!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1843678-13.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new interdiff.txt659 bytes
new 1843678-14.patch1020 bytes

Comment can be a bit better :)

Interdiff is from #8.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Title: Make Messages renderable (& remove from process) » Move building of messages render array from template_process_page to template_preprocess_page

Re-titling.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Title: Move building of messages render array from template_process_page to template_preprocess_page » Move building of messages render array from template_process_page() to template_preprocess_page()

Parens :)

star-szr’s picture

star-szr
he/him
Primary language English
commented
Issue summary: View changes

Add link to process meta

star-szr’s picture

star-szr
he/him
Primary language English
commented
Issue summary: View changes

Update summary to reflect latest patch

yesct’s picture

yesct commented
Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll 
git ac https://drupal.org/files/1843678-14.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1020  100  1020    0     0   1160      0 --:--:-- --:--:-- --:--:--  1463
error: patch failed: core/includes/theme.inc:2749
error: core/includes/theme.inc: patch does not apply
star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs work » Fixed
Issue tags: -Needs reroll

Hmm, I was doing an unrelated reroll and it looks like this got included as part of a commit on #2026347: Adding methods to UserInterface/AccountInterface.

Ref. commit 45a9e85 and the theme.inc diff.

So… fixed? :)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Reasoning for problem/motivation