Problem/Motivation

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

Proposed resolution

Move the building of the 'breadcrumb' render array from template_process_page() to template_preprocess_page() in preparation for removing template_process_page(). Because of #1947536: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service we don't need to worry about code in the stack randomly calling drupal_set_breadcrumb(), and when the process layer is removed template_preprocess_page() the preprocess layer will be the last stage before rendering anyway.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Blockers

#1939066: Convert theme_breadcrumb() to Twig

Related Issues

#1843650: Remove the process layer (hook_process and hook_process_HOOK)
#1589968: Move Title from process to preprocess, remove template_process_page()
#2004286: Defer calls to drupal_get_* functions until printed inside a template by adding a RenderWrapper class

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klonos’s picture

I hope to see this make it in D8, but there's been no action for moths :/

bdragon’s picture

Assigned: Unassigned » bdragon
Issue tags: -Twig

grabbing @ code sprint process kill table

bdragon’s picture

Issue tags: +Twig

fix tags

bdragon’s picture

Status: Active » Needs review
FileSize
5.04 KB

Let's see what the tests say.

bdragon’s picture

(test queue is a bit borked atm)

Status: Needs review » Needs work
Issue tags: -Twig, -theme system cleanup

The last submitted patch, 0001-1843668-Make-breadcrumbs-renderable.patch, failed testing.

bdragon’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Twig, +theme system cleanup

The last submitted patch, 0001-1843668-Make-breadcrumbs-renderable.patch, failed testing.

Stefan Freudenberg’s picture

This depends on https://drupal.org/node/2004286 to be committed.

Stefan Freudenberg’s picture

Status: Needs work » Postponed

Setting status to postponed.

Stefan Freudenberg’s picture

Issue summary: View changes

update

jenlampton’s picture

Status: Postponed » Closed (duplicate)

Actually, closing as a dupe of https://drupal.org/node/1939066

jenlampton’s picture

Issue summary: View changes

added defered wrapper for render on print

jenlampton’s picture

Title: Make Breadcrumbs renderable » Make Breadcrumbs renderable (& remove from process)
Status: Closed (duplicate) » Needs work

Okay, not closed/dupe. Just blocked by #1939066: Convert theme_breadcrumb() to Twig
This is going to need a reroll once that gets in (to remove the conversion).
Changing to needs work.

joelpittet’s picture

Assigned: bdragon » Unassigned
Status: Needs work » Needs review
FileSize
2.29 KB

I know this won't work but thought I'd post for someone to work with from. There are a few errors that the testbot will catch.

This is just the remaining pieces from #4 without the twig conversion from #1939066: Convert theme_breadcrumb() to Twig

Status: Needs review » Needs work

The last submitted patch, 1843668-13-breadcrumbs-renderable-process-remove.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
FileSize
1.57 KB
1.44 KB

Trying this simple patch that just moves the breadcrumb variable from process to preprocess. Seems to behave locally and passes the tests that failed above - didn't check all the tests with exceptions, those just look like invalid render array fallout.

Interdiff from #13.

(I think this is got a lot simpler now that #1947536: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service is in.)

star-szr’s picture

Issue summary: View changes

blockers

jenlampton’s picture

Status: Needs review » Reviewed & tested by the community

I thought we wouldn't need to do anything special to move these up to process now, but I kept thinking myself around in circles.

This looks great (testing patch locally) and breadcrumbs are working for me. Yay!

jenlampton’s picture

Issue summary: View changes

Add link to process meta

star-szr’s picture

Issue summary: View changes

Update issue summary to reflect latest patch

star-szr’s picture

Title: Make Breadcrumbs renderable (& remove from process) » Move building of breadcrumb render array from template_process_page() to template_preprocess_page()

Re-titling.

YesCT’s picture

Issue tags: +RTBC July 1

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll...

git ac https://drupal.org/files/1843668-15.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1477  100  1477    0     0   1549      0 --:--:-- --:--:-- --:--:--  1913
error: patch failed: core/includes/theme.inc:2749
error: core/includes/theme.inc: patch does not apply
star-szr’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.41 KB

Rerolled.

penyaskito’s picture

+++ b/core/includes/theme.incundefined
@@ -2779,6 +2779,15 @@ function template_preprocess_page(&$variables) {
+    '#breadcrumb' => \Drupal::service('breadcrumb')->build(\Drupal::service('request')->attributes->all()),
+  );

Use Drupal::request() instead of Drupal::service('request').
Otherwise, looks good to me.

gloob’s picture

FileSize
545 bytes
1.4 KB

Changing Drupal::service('request') by Drupal::request()

Testing contributing workflow in Drupal hope you don't mind Cottser ;-)

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC after the re-roll.
Thanks :)

star-szr’s picture

Not at all, thanks @gloob and @penyaskito :D

jenlampton’s picture

nm

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

effulgentsia’s picture

I think it's fine that this was committed. However, the following from the issue summary is incorrect:

when the process layer is removed template_preprocess_page() will be the last stage before rendering anyway

Every module's and theme's hook_preprocess_page() implementation runs after template_preprocess_page().

Delaying the breadcrumb build until after all module and theme preprocess functions were done was added in #907690-24: Breadcrumbs don't work for dynamic paths & local tasks #2 because of performance issues that were around at that time. I don't know yet whether the concerns from then are still valid or not, so I left a comment on that issue linking to this one.

star-szr’s picture

Thanks @effulgentsia! I realize after reviewing that it wasn't quite right. What I meant to say was that the preprocess layer in general would be the last stage before rendering, not specifically template_preprocess_page().

star-szr’s picture

Issue summary: View changes

Better wording

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

Anonymous’s picture

Issue summary: View changes

Correction regarding template_preprocess_page()