User story:

A themer should be able to take two fields that are about to be rendered and put them in an HTML wrapper using "prefix" & "suffix"

How this currently works in ds for D7:

  • ds adds a preprocess function to all entities (theme_reg_alter)
  • In that function a "region" is created AND rendered (using ds_render_region)

This means that a theme has no opportunity to alter the render array of fields in the "region"

My proposal:

  • ds adds a preprocess function to all entities (theme_reg_alter)
  • ds adds a process function to all entities (theme_reg_alter)
  • In the preprocess function all the fields are moved into the new "region" created by ds
  • In the process function those regions are rendered

This give themes the opportunity to create a process function that allows them to alter the fields.

Comments

bleen18’s picture

Issue summary:View changes

Revamped issue summary.

swentel’s picture

Status:Active» Closed (won't fix)

I'm a bit afraid to introduce new hooks since they might be called a lot during rendering.
Also after some chatting on IRC following should be enough:

- either use field_group module which not only allows you to use fieldgroup tabs, but regular div containers as well
- themes can also implement any hook now in D7

So there should be enough power already to make this easily work.

bleen18’s picture

Title:Themes cannot alter content before ds layouts are rendered» Themes/modules cannot alter entity render arrays before ds layouts are rendered
Status:Closed (won't fix)» Active

I spoke with swentel on IRC this morning about this and he gave me a couple of good suggestions:

  1. the field_group module
  2. hook_entity_view_alter

The first suggestion works well but only for the limited case of grouping fields together - super powerful, but limited. The second suggestion gets envoked too early to alter the render array after ds has made its changes. I stil content that ds should alter that render array in such a way that other themes/modules can alter it further if desired.

I made one proposal in the original post that I still feel is valid, but here is an alternative (better?) approach: instead of using hook_theme_registry_alter to add a preprocess function to all entities, why doesn't ds use hook_entitiy_view_alter to rejigger the render array according to the appropriate layout. This would allow other modules/themes to use hook_entity_view_alter() to modify that array even further and I think this would be cleaner to maintain in general.

This would not require any new hooks and I think it would be more inline with render_api in d7...

I'd be willing to put in some time on a patch as long as I knew there was some support from the maintainers (and no gotchas that I havent thought of)

Thoughts?

swentel’s picture

The gotcha I think is that we choose to render the content already in ds_render_region() so the template files only need todo print $region_name and that's it.

The problem with changing that is that we'll have to update the template files in ds, but that might (rather, most likely) possibly break existing sites out there and template overrides they might have done. So basically, that's an API change which is not really possible here. I'm also doing this late in the process because I want to possibly pick up any preprocess variables that are made available and to make sure other modules like field_group (which I co-maintain) have already moved all stuff in approriate groups.

So the place I can think of right now is an alter just before the rendering, either in ds_entity_variables (before line 596) or in ds_render_region(). I prefer the first function as we can just pass on the content. The second function is just for rendering the regions and the alter hook would be called to much. (I don't think it's necessary to know the exact region as well which we're going to render right at that point).

It's another hook that you could implement that which I'd fully support implementing. something like hook_ds_prerender_alter($content, $context); where $content is the array and $context contains the name of the entity type, bundle and view mode. Maybe we should pass on the layout settings here as to make sure.

Thoughts ?

bleen18’s picture

A few points:

I dont think there would have to be an API change given my proposal in #2, as ds could still render the render_arrays in a preprocess function. They would simply be built in hook_entity_view_alter(). You're concern about waiting until preprocess for the sake of other modules (like field_group) may make this suggestion moot however.

As for your proposal in #3... if we added a drupal_alter around line 596, it would stil be (just barely) too early. To illustrate this suggestion, here is some pseudo code:

<?php
   drupal_alter
('ds_prerender', array($content, $layout, ...));

  
// Create region variables based on the layout settings.
   
foreach ($layout['regions'] as $region_name => $region) {

     
// Create the region content.
     
$vars[$region_name] = ds_render_region($vars[$render_container], $region_name, $layout);
       ...
    }
?>

With the drupal_alter in that position I still could not alter $vars['left'] for example. This means that I could not modify a render array of left to add new item of #type->markup in the correct position within the left region.

Now, lets say that ds_render_region returned a render array instead of markup, the drupal_alter was called and then drupal_render was called. Something like this (more pseudo-code):

<?php
  
// Create region variables based on the layout settings.
   
foreach ($layout['regions'] as $region_name => $region) {
     
$layout_render_array = array();

     
// Create the region content.
     
$layout_render_array[$region_name] = ds_render_region($vars[$render_container], $region_name, $layout);
       ...
    }
   
drupal_alter('ds_prerender', array($content, $layout, ...));
    foreach(
$layout_render_array as $reg=>$content) {
       
$vars[$reg] = drupal_render($content);
    }
?>

Thoughts?

(PS ... I'm enjoying this, thanks for taking the time to talk it out with me)

swentel’s picture

Ok, I get the problem with the alter I proposed. I've got a patch ready on my local machine to see the impact performance wise and it's little slower right now. I'm going to experiment to see whether I should introduce the api change or not so the extra loop may be avoided.

bleen18’s picture

swentel: any chance you can post the patch you're working with?

swentel’s picture

StatusFileSize
new3.05 KB

here's the patch - note it has some dsm() calls in it so you'll need to install devel() (testing microtime - didn't have time to clean this up)

bleen18’s picture

swentel ... writing this from your DC London session just before it starts :)

Are you still considering the patch in #7?

swentel’s picture

Status:Active» Closed (duplicate)

I still do - this is related now as well with #1235902: Create conditionals in fields. - Hope I can get the first version of that working next week after a good weekend of sleep :)

swentel’s picture

Status:Closed (duplicate)» Active

No changing status yet, will use this one as the main issue to track.
edit: others related:
- #1250144: Show block on specific pages settings not working
- #1256702: Iterate through fields in region from within template

muschpusch’s picture

subscribe...

swentel’s picture

Status:Active» Postponed

Ok, I need to postpone this one for a second branch - the refactoring with a preprocess and process function makes me freak out and a lot of tests start failing now - also in combination with renderable elements and panels.

As a temporary thing, you can still implement hook_process_node for instance yourself and do some fiddling. There's much done then of course, but it's still possible. I don't have a timeframe for a second branch though.

swentel’s picture

Version:7.x-1.x-dev» 7.x-2.x-dev
Category:bug» task
Status:Postponed» Active

Tagging for upcoming sprint

swentel’s picture

Assigned:Unassigned» swentel
swentel’s picture

Status:Active» Needs review
StatusFileSize
new2.98 KB

This will probably go in.

swentel’s picture

Status:Needs review» Needs work
StatusFileSize
new3.36 KB

updated patch. still couple of failures, almost there.

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new3.8 KB

This one actually is green!

bleen18’s picture

Havent had a chance to test this properly, but in perusing the code the patch in #17 looks great :)

aspilicious’s picture

Status:Fixed» Closed (fixed)

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

Anonymous’s picture

Issue summary:View changes

Revamped issue summary.