cant we make drupal_get_page do the is_string - if not - wrap - in array(#markup) dance? just $page['content'] = $content; $page['content'] = is_array($content) ? $content : array('#markup' => $content); seems rather easy to me and saves code. chx_: perhaps. 6 in one. half dozen in the other (an american saying?) depends, really. i see inconsitent behaviour already + // Since we disable blocks, we must normalize to an array. + if (is_string($return)) { + $page = drupal_get_page(array('#markup' => $return)); and $page = drupal_get_page(array('#markup' => t('The website encountered an unexpected error. Please try again later.'))); chx_: for backwards compat is has to be in drupal_render_page() but drupal_render_page runs + if (is_string($page)) { + $page = drupal_get_page(array('main' => array('#markup' => $page))); + } i would rather see this call to be $page = drupal_get_page($page) and make d_g_p do the wrapping instead of having the wrapping in the code thrice not to mention that as i already pointed out the wrapping is inconsistent chx: it is common for us to require you to use the new syntax if you want special features like disabling blocks. like query alter only happens for think dynamic select er, remove the 'think' there well, we have to have it in drupal_render_page(). rly? we could add another place, if you want why you cant have if (is_string($page)) $page = drupal_get_page($page) in drupal_render_page? yes, unless we convert all existing menu callbacks oh and have drupal_render_page do array('main' => array('#markup' => $page)) and then you can drop the other two wrappings. that sounds very nice other two? drupal_not_found that has to stay and _drupal_log_error and why maybe not good idea maybe not :) $variables['show_blocks'] = &$variables['page']['#show_blocks']; <= is that & really necessary? are we going to change #show_blocks later and want the variable to reflect that? it is not needed, but i vaguely thought it was better. if you think not, i will remove. i always raise an eyebrow at references i do not readily see the purpose removing ... are we going to change #show_blocks later and want the variable to reflect that? <= this is the big question.if you think yes, keep, if not, drop. nitpick: for the sake of reviewers, move the whitespace fixes in template_preprocess_page (main and secondary menu) into a separate patch i was wondering what are you changing and apparently: nothing this patch is hard enough already without wasting braintime on comprehending a nonchange as a nonchange next up, template_preprocess_node also has references. ok, i will look atwhitespace. agreed i already removed those references too on my pc block_page_alter yay my heart sings merrily at the - $output .= theme('block', $block); line honestly. havin' the theme layer calling a major hook was always very dirty thanks for cleaning chx: so, about drupal_get_page(). should that have an optional $content? If so, does that default to NULL? in your suggestion it can be string or array yeah string or array why would that be optional?? '#weight' => $weight + $weight++; yuck. '#weight' => $weight++, thanks :) ewww you missed the trailing comma at least if you insist on keeping the ++ on a separate line, i am fine but then move it before ++$weight; where is that? as that's faster block_get_blocks_by_region i am mostly just nitpicking my way through the patch the generic direction looks ok to me why is $page[$region]['blocks']['#sorted'] = TRUE; not in block_get_blocks_by_region ? chx: the door is open now to make block module optional that is fine but what has to do with that line not being in b_g_b_b_r ? you converted blog module to DBTNG/multiload in this patch????? meow! meow is http://webchick.net/please-stop-eating-baby-kittens there is a srted line in bpa sorted yupp i thought thta was sufficient. maybe not that's what i do not understand why is that line separate? you have if ($blocks = block_get_blocks_by_region($region)) { + $page[$region]['blocks'] = $blocks; + $page[$region]['blocks']['#sorted'] = TRUE; + } and I am staring at it wondering why block_get_blocks_by_region does not set #sorted TRUE is equivalent, no? well i also wonder whether this code block should be simple $page[$region]['blocks'] = block_get_blocks_by_region($region); i thought the code looks better if all the properties of $blocks is set in that function instead of picking one arbitrarily to be set in the caller. moved #sorted = TRUE moshe_work: and what about scratching the whole if block? i know it changes the $page structure but it's a small change really -- as said just havge $page[$region]['blocks'] = block_get_blocks_by_region($region); regardless whether the right side is empty or not there is a general inconsitency of what you call this array sometime you have it as $output, otherplace it's $build i think frando did not want to populate empty regions here it is not for block module to do that fine agian i am staring at $output as i thought that's a string i changed that to $blocks just now oh, where? $build is good you mean block_get_blocks_by_region()? i mean the whole patch uses $output as an array oftentimes and yet blog module uses $build instead yeah. i like $build in general lemme change block_get_blocks_by_region() now ok and then generally switch $output to $build if possible thanks. '#weight' => count($nodes), in blog_page_last, wtf? why do you want it to sink if there are more nodes?? this would wreak havoc on page_alter / theming chx: that might not be needed anymore we did not used to group all nodes its compeltely new in the patch Join flabberkenny has joined this channel (n=flabberk@217-19-28-232.dsl.cambrium.nl). OK to just say '#weight' => 5, for the pager? aye. hummm how come node_view even works?? + 'node' => $node, + 'teaser' => $teaser, these two are not form children moshe_work: ? hmmm moshe_work: also why do you need to set #weight and #sorted explicitly, if you just leave ............... oh wait this is not form_builder, fine, fine, fine. chx: do i need to change: 'node' => $node, + 'teaser' => $teaser, to #teaser? or something? right. seems to work now you mean w/o the change? maybe the preprocessor is fiddling then those lines of code are superflous try removin them i can't remove them then how does this work currently? * chx_ is puzzled i thought it works because the += does not even fire. if you throw a non-array at drupal_render that ought to break chx: see template_preprocess_node + if (strstr(drupal_render(node_view(clone $node)), $keyword) || strstr($node->title, $keyword)) { <= have mercy on me and separate this into more than one line. we STILL do not code in C :P chx: seems like those elements never get rendered moshe_work: YUCK chx: thats JVD code I think i just added drupal_render() i will split it up moshe_work: if you are fixing up queries and multiple loads, breaking this into edible bits cant be that big a problem moshe_work: i hoped eventually we get an array which as together cna be thrown at drupal_render. having this not-really-drupal-render array is really ugly. agreed. moshe_work: and you have drupal_render(node_view($node)); by the boatloads lemme look moshe_work: so how the f does this currently work...? I know. maybe drupal_render() ignores non arrays it works because it has a #theme on it so the iterate-the-children part does not fire now, give me a break but seriously. yes, node and teaser must be #node and #teaser this #theme business is odd this is the first major broken-ness of the patch. frando and eaton opened an issue yes but just how many issues you want to melt into this one ? right ok so we agreed you change these to proper properties well, still use #theme or introduce a #type=node? #theme is fine just make the array a real renderable array in the preprocessor, i will need to do $variables['teaser'] = $variables['#teaser'] = ok? node_page_default again has this curious business of tying the #weight to count($nodes) -- which I presume came from the fact that the nodes have a weight but then those are under a different root, so just give that a fixed weight. moshe_work: yes it's ok. er, variables['teaser'] = $variables['elements']['#teaser'] yes yes chx: yup - fixed all pager weights $build = array('#markup' => '
' . $default_message . '
'); sniff, i can haz #prefix and #postfix, please? still node_page_default chx: i guess this goes unchanged: $variables['content'] = drupal_render($node->content); chx: why bother just more lines and gets in the way if other modules want to prepend or append well, i guess it can useful system_batch_page , $page = drupal_get_page(array('#markup' => $output)); more code to be removed, yay in system_elements + '#value' => NULL, change that to empty string thanks. wow another + '#markup' => '
' . filter_xss_admin($term->description) . '
', which would beneift from #prefix and #suffix. the less string concats we do , the better we are off should '#value' => '' even be there? i would risk a yes. well. we do not babysit broken code do we. then no. moshe_work: I think I am finished. moshe_work: in overall very nice minus that node_view array hiccup. those string concats just happen later yeah but before they happen we can alter in pre_render for example. true debating microseconds now