Introduce theme wrapper, as discussed on the Sprint

theme_wrapper.patch looks okay.

The ids used in box_to_wrapper_core.patch are a bit vague I think we should avoid using numbers in ids unless they correspond to an actual id in the db- such as node-37 or comment-48. For example "comment-4" in the patch could be renamed to "comment-viewing-options."

I vote against numbers too. Can't we use 'comment' instead of 'comment-1', 'comment-2', etc.

Most of the calls to theme('box') are used to print status messages. Shouldn't we use drupal_set_message() instead? That would be a lot more consistent with other status messages and is more likely to catch the user's eye. Please do.

One more revision and this can be committed.

I wonder why we need a 6-step plan to get such simple changes implemented ...

The changes to the comment.module no longer apply.

The changes to the search module are incorrect; part of the code is missing?

Sometimes the ID and class are identical, sometimes they are not?

Does it make sense to specify an empty title? Sure that should be an optional paramter?

If you change the API, the following files will need to be updated:

article/article.module
banner/banner.module
cvslog/cvs.module
eatlocal/resource/resource.module
ecommerce/cart/cart.module
evaluation/evaluation.patch
event/event.module
filestore2/filestore2.module
lanparty/lanparty.module
location/location.inc
location/location.module
mail_archive/mail_archive.module
massmailer/massmailer.module
paypal_framework/paypal_framework.module
peoplesemailnetwork/peoplesemailnetwork.module
project/comment.inc
project/project.module
project/release.inc
queue/queue.module
recipe/recipe.module
rsvp/rsvp.theme
tagadelic/tagadelic.module
term_statistics/term_statistics.module
trackback/trackback.module
troll/troll.module
volunteer/volunteer.module

Quite a few modules incorrectly use theme('box') as a substitute for drupal_set_message().

I'm not sure, but shouldn't we warp everything which is returned to the screen through theme_box()?

Then it would be very easy to style particular tables/whatever differently from page to page...

Just think about it, and consider this..

Something i forgot to mmention is that I would encourage a namechange from theme_box() to theme_wrapper() or theme_container(), just to make it bore obvious what it does..

Ber, Dries what do you guys think of this?

I am concerned about bandwidth with the potential for many long class names/ids in both the HTML and CSS. Do you know how much effect this change will have?

IMHO it would be better NOT to call theme('wrapper'..) at all, but to make the raw data available to the template engine - you can call it in the template if you want to, but if it is not right for your theme (eg handheld display) you don't need to. Of course this will be much easier once you move away from XTemplate.

djnz said:

IMHO it would be better NOT to call theme('wrapper'..) at all, but to make the raw data available to the template engine - you can call it in the template if you want to, but if it is not right for your theme (eg handheld display) you don't need to. Of course this will be much easier once you move away from XTemplate.

I wholeheartedly agree. Making the raw data available is the best. Whatever presentation details are best handled in the theme templating engine.

I beg to differ on the fact that you need to move away from XTemplate to do this. I use the april 2005 version 1.7 of XTemplate which uses iterations/interpolations etc. over variables, and it's perfectly capable of handling the raw data into variables.

The xtemplate.engine interface is what's not exactly "optimal" with regards to handling flexibilty in themeing. The template-file xtemplate.xtmpl is even worse with regards to flexibilty. The actual XTemplate class is just perfect - and extremely fast.

I am completing my first version of an interface for the newer XTemplate where sub-blocks/subsections can override assigns done in parent-blocks/-sections in a way just like CSS declarations and where subsection logic can affect inclusion of parent-blocks/-sections.
Xtemplate is extremely flexible and simple in my opinion; that it's probably the fastest (?) full-fledged templating system around doesn't hurt as well.

Bèr,
I'm actually for the use of CSS, class-names, unique IDs etc. with XHTML and all-liquid layouts tweaked into the most stunning creations like on www.csszengarden.com, and the ability to support different media platforms, cascading CSS rules and keeping IE, FF and the others in standards-compliant mode - avoiding quirks mode for box-layout in any browser platform and generally avoiding tag-soup...
... it's delegating the creation of XHTML structure outside the template engine that I don't like.

The template engine I have nearly completed supports as much flexibility I think I'd care to use in any near future with support for creating the whole page with iterated theme wrappers for node/comment/box/block still not requiring parsing the page until a final recursive parse with cut-off of subsections/sub-blocks at this time and all logic in assigning/altering variables done in the invocation of section-dependent functions with varying granularity just like CSS - i.e all variables are first setup and then one parse fills in the variable fields and outputs the XHTML.

That means I could also archive or cache presentation-independent copys of the web-pages using serialization - or possibly optimizing page-serving by changing sub-sections of cached page from the un-serialized content variables ... e.g changing just a my-forum-threads for each user-id on the front page - while avoiding other content-pulling function calls or DB queries until cached page is dirtied.

Well, that's some of the benefits I see with raw content data handled in the template engine, although you misunderstood what I said in my previous post.
:-)

@djnz eldarin; Can you please get me some figures?

I was kind of hoping you might come up with some figures to justify the change, unless it is optional/themeable.

IMHO anything that results in a step change in either disk footprint, execution time or bandwidth requirement needs to be carefully considered. One of the themes I am working on is for handheld wireless devices. There are no images and every byte counts - my whole payload is less than 4.3k.

Just as I mentioned the possible serialization of all content variables - including iterated sections like node,comment etc.; I forgot to mention the best part of caching a partially parsed page, with some variable fields intact - e.g a personalized part of the page. That would prune down the amount of parsing repeated for this special case.
There are some improvements done to XTemplate class to achieve this capability of conserving all content - including iterated parts - but nothing more than 3 lines of code in the parse-function.

I guess the fact that it is an overridable theme_ function takes away the objection about payload bloat. Just a couple of comments about the function itself:

$class = NULL - should this be $class = '' or does "wrapper$class" tolerate NULL?

Whilst I understand your reasons for wanting id's (I think) it is going to be really hard to ensure they are unique. How about allowing $id = '' as the default and then do

  $id = $id ? " id=\"$id\"" : '';
  $output  = "<div class=\"wrapper$class\"$id>\n";

The patch makes a lot of sense to me, and my theme engine as it is now will benefit. Having class and id fields along with content+title instead of just the region is much better with regards to CSS styling.

I would of course prefer to be able to receive arrays as a little more complex content, but that all depends on the modules ... :-)

I made my theme engine such that I can have multi-level loops and rules "tagged" on theme-blocks (modified xtemplate v0.3), so that's why I would like arrays as content instead of just 50 calls to theme_wrapper on a list - e.g log listing.

Sometimes it's faster to just choose the right template file, than having 10s of if-thens to decide what/how content should be parsed into blocks ... Initially selecting template file based on url, user, browser-sniffing, taxonomy etc. is then just a question of matching it from a simple array.

Now, I just need to get my homebrew forum module up to speed. It now supports deleting/moving sub-threads or posts (between threads), filtering on users in threads etc. I think this theme_wrapper function in the core would greatly ease designers' efforts as well as developers' .

How about updating the CSS files?

Did you forget to attach this new file???

I'm no longer convinced this is a good thing. There are only 5 theme('box')'s left in core, some of which are not strictly necessary in my view:

1. Unlike any other form, we wrap the comment form in a theme('box'). Why?
2. Unlike any other table, we wrap the menu table in a theme('box'). Why?
3. ...

I'd like to see us investigate if the remaining calls to theme('box') could be removed. I'm reasonably convinced that we can.

I agree with Dries, although the wrapper is a good idea, it does introduce extra markup that IMO would be better if we could avoid entirely. If we can get rid of the other theme('box') I think that would make the most sense.

Remember: KISS (keep it simple stupid) !

Someone should fix this, let's keep it open.

Won't make it into D6, bumping to D7.

With the new completely forms-based rendering in D7, it seems this is now implemented as the new #theme_wrapper element property. I suggest this be set to "fixed".

Huh yeah. (fgm, please show your archeology permit at the exit :p )