use drupal_render() for profile display

This, friends, is where it is at. We have a badass rendering infrastructure and places like this are perfect for it. Will have more opportunity to test tomorrow, but a HUGE +1 to the principle.

Not tested yet but +1 on the idea.

big fat juicy +1 for the idea. I just quickly tested it, and it looks like something might be up : I added 2 textfield profile fields, but only one is getting displayed on user view. I'll try and have a more in depth look later.

nothing jumps out looking at the patch itself.

James promised to give this a decent review.

i fixed a bug in user_profile that was causing it to only display a single field.

err, profile_view_profile() rather.

in comparison to the old formatting, i think the fieldsets look pretty tacky. i'd much rather use headings than fieldsets.

chx alerted me to the fact that i didn't include a file.

user/1 is just giving me a blank page with this patch. Trying to track it down...

Additional: When I submit the user edit form I get:

    * warning: _phptemplate_render(./themes/engines/phptemplate/user_profile.tpl.php): failed to open stream: No such file or directory in /Applications/MAMP/htdocs/head/themes/engines/phptemplate/phptemplate.engine on line 398.
    * warning: _phptemplate_render(): Failed opening './themes/engines/phptemplate/user_profile.tpl.php' for inclusion (include_path='.:/Applications/MAMP/bin/php4/lib/php') in /Applications/MAMP/htdocs/head/themes/engines/phptemplate/phptemplate.engine on line 398.

Which Ted tells me is because a file is missing from the patch, so I will wait for his re-roll and then review. :)

Updated patch to fix those problems webchick was encountering.

However, there is a larger problem. The output *should* not be using fieldsets whatsoever, those are for form elements. Heck, the HTML looks like:

<fieldset><legend>Personal</legend><div class="form-item">
 <label>Last name: </label>
 Serbinski
</div>
<div class="form-item">
 <label>Name: </label>
 Ted
</div>
</fieldset>

That's not a form!!!

I looked into this and I don't think it's a problem with this patch, but a larger one in general. The output should ideally be: each category as an DL, with an H3 for a title and DT / DD for each of the values.

However, I don't see any possible way to do this because it's passing things to drupal_render() ... perhaps that needs to be patched for when content is being viewed?

If that is case, then don't hold up this patch, it's ready to go. We'll march on over to drupal_render() to fix that so those in the know speak up :-)

drupal_render() is utterly and completely agnostic about the content it renders. If you set #type on each element to 'profile_field' it will call theme_profile_field when it comes time to render them. If you set the #type on the enclosing element to profile_category, it will call theme_profile_category...

Eaton you're right, here's a patch that introduces a new theme_dl() ... after talking with Moshe and Karoly, this is our best bet.

Updated patch after talking with Moshe. New default CSS for better profiles :-)

Screenshot of CSS.

• Please remove 'user_build_content()'. There is only one call to this function.
• Why are definition lists the best solution? Personally, I don't see profile elements as definition lists. I think of these are regular key-value pairs, rather than term-definidation pairs.
• Crazy idea: generate microformat enabled profile (http://microformats.org/wiki/hcard-examples) -- probably not for this patch. ;-) Note that the hcard examples in the link above are not using definitition lists either.

DL is easily answered, that's how it's done now. See http://api.drupal.org/api/HEAD/function/theme_user_profile . If you do not like the DLs, that's another issue, this patch just moves things towards a better modell.

And Moshe answered the other two. So, putting back to RTBC.

As for DL, have a read.

Some people believe that definition lists should only be used for terms and definitions. Others believe that definition lists can be used to tie together any items that have a direct relationship with each other (name/value sets). This second point of view is supported by an example within the W3C specifications

Because the W3C says so ;-)

OK. Next issue: I find it very difficult to figure out whether all the data has been properly escaped.

For example, in theme_dl():

+  $output = ($element['#title'] != '') ? '<h3>'. $element['#title'] .'</h3>' : '';

Is $element['#title'] secure? It all looks insecure ...

Dries, compare please theme_dl and theme_user_profile. Zero escape and it should be so. Also observe the following untouched code in profile module:

if ($value = profile_view_field($user, $field)) {
       $description = ($field->visibility == PROFILE_PRIVATE) ? t('The content of this field is private and only visible to yourself.') : '';
       $title = ($field->type != 'checkbox') ? check_plain($field->title) : NULL;

The title is escaped for good in there. And profile_view_field gives a treat to various types -- textareas are check_markup'd and so on. I can not see any new secholes blown by this patch.

First of all, dl is good.

- The visual theming looks really heavy, grey, and arbitrary. It should be as little as possible and anything fancy needs to be done by bluemarine and friends.
- As node.module exemplifies, #type => markup is unneeded.
- What is a #type => item? theme_item() calls theme_form_element(), which is certainly wrong for user viewing.
- Doesn't #type => dl need an entry in system_elements()?

This should be two separate patches. One to introduce theme_dl() and replace the hard coding we have scattered around now and a second for the whole content thing.

- Doesn't #type => dl need an entry in system_elements()?

Nope. system_elements() is necessary to provide default values for array properties that aren't specified, and other conveniences, but by itself, #type maps to theme functions.

Drumm, this simply goes a bit far . Many, many patches in the past introduced new functions as they needed them without being split up. I do not see the reason to do such fine grain patches. These do belong together. It would simply delay development to say "to test this patch please first apply 79221 and then this one".

Also, #type => item does call form_element but as we theme the whole stuff it does not matter, drupal_render does not reach this. Consistency reasons are behind the decision to be them items.

What consistency reasons?

Here's an updated patch. Also includes the user_profile.tpl.php that Moshe forgot ;-)

I changed the CSS back to something extremely simple. Neil that was a good point.

For those that are interested, with this setup, it makes it *really* easy to do some awesome stuff, like show profiles in a column view, with labels on left and values on right, very cool. But nothing fancy for the super simple Drupal themes :-(

Here's a screen of that simplicity.

Drumm, as said, it could be any #type because drupal_render won't even reach that element, we render the children by hand in theme_dl. Consistency: something which is plain text but has a #title is an #item. That's it.

m3avrck: are you saying that using form_alter() is easier than overwriting a theme function? IMO, this patch makes it harder to theme stuff. The current theme function also makes it easy to create two columns.

There are a few places we use dl in core. node/add and admin/help come to mind. I'd like to see these use a themable definition list function at some point and could be used to demonstrate a theme_dl() patch which stands on its own.

Lets compare theme_dl() to its nearest relative, theme_item_list():
- theme_item_list() has a name which describes what it does- it is a themable list of items. theme_dl() is named after an HTML tag.
- theme_item_list() takes a list of items, which follows the 'foo' or array('data' => 'foo', ...) pattern. theme_dl() follows the #-means-metadata pattern.
- both attempt to explain complex array structures without an example.

If the # pattern is really what we want to use, instead of the 'data' pattern, I think throwing an example in the function documentation will help make that clear.

I'm not sure if I like the first new name that comes to mind, theme_definition_list(). The name 'definition list' is misleading, as shown above, and 'item list' doesn't directly correspond to ul or ol. The concept is a list of labeled items.

Why even set '#type' => 'item' if theme_dl() handles it and doesn't even look at #type?

The visual design looks good.

Moshe: I know how to theme. The point is that with the proposed system, it takes one extra step (i.e. a form_alter), and one that is not necessarly designer friendly. I was just objecting to Ted's comment which suggested that this would make theming easier. Quite the opposite, I'd say.

I agree with Neil that the theme_dl is quite different from theme_item_list -- someone a designer would not expect, IMO.

@Dries, I say it's easier to theme because it follows CSS Zen conventions--output the correct HTML and let CSS take care of you.

In this case, the output is a DL. So what you might think, right?

Well, actually, that is completely wrong. Make my DL into a table, how about a calendar, image gallery-esque, and boxtastic

That's pretty darn easy to theme, have a look for more examples: http://www.maxdesign.com.au/presentation/definition/

If Drupal outputted *consistent* and clean HTML *everywhere*, we would have no real need for theming functions, IMO :-)

But, to play devil's advocate, yes if you don't want this to be a DL or need something else, you'll need to create a theme_() and work a bit of form_alter(). But then again, you have a user_profile.tpl.php to play with as well. And that has the $account array which has *everything* a themer would need to use:

  stdClass Object
(
    [uid] => 1
    [name] => ted
    [pass] => 49e02178d15a1784c2e98a5335824b83
    [mail] => ted@lullabot.com
    [mode] => 0
    [sort] => 0
    [threshold] => 0
    [theme] => 
    [signature] => 
    [created] => 1156297548
    [access] => 1156521406
    [login] => 1156521368
    [status] => 1
    [timezone] => 0
    [language] => 
    [picture] => 
    [init] => ted@lullabot.com
    [data] => a:2:{s:6:"submit";s:18:"Create new account";s:7:"form_id";s:13:"user_register";}
    [submit] => Create new account
    [form_id] => user_register
    [roles] => Array
        (
            [2] => authenticated user
        )

    [profile_name] => Ted
    [profile_last] => Serbinski
    [content] => <dl class="user-member"><dt>Member for</dt><dd>2 days 14 hours</dd></dl><h3>Personal</h3><dl><dt class="profile-profile_last">Last name</dt><dd class="profile-profile_last">Serbinski</dd><dt class="profile-profile_name">Name</dt><dd class="profile-profile_name">Ted</dd></dl>

    [profile] => Array
        (
            [summary] => Array
                (
                    [member_for] => Array
                        (
                            [#value] => 2 days 14 hours
                            [#title] => Member for
                            [#type] => item
                            [#printed] => 1
                        )

                    [user_picture] => Array
                        (
                            [#value] => 
                            [#printed] => 1
                        )

                    [#weight] => -5
                    [#attributes] => Array
                        (
                            [class] => user-member
                        )

                    [#type] => dl
                    [#children] => <div class="form-item">
 <label>Member for: </label>
 2 days 14 hours
</div>

                    [#printed] => 1
                )

            [Personal] => Array
                (
                    [profile_last] => Array
                        (
                            [#attributes] => Array
                                (
                                    [class] => profile-profile_last
                                )

                            [#weight] => 0
                            [#value] => Serbinski
                            [#title] => Last name
                            [#type] => item
                            [#printed] => 1
                        )

                    [profile_name] => Array
                        (
                            [#attributes] => Array
                                (
                                    [class] => profile-profile_name
                                )

                            [#weight] => 0
                            [#value] => Ted
                            [#title] => Name
                            [#type] => item
                            [#printed] => 1
                        )

                    [#title] => Personal
                    [#type] => dl
                    [#children] => <div class="form-item">
 <label>Last name: </label>
 Serbinski

</div>
<div class="form-item">
 <label>Name: </label>
 Ted
</div>

                    [#printed] => 1
                )

            [#children] => <dl class="user-member"><dt>Member for</dt><dd>2 days 14 hours</dd></dl><h3>Personal</h3><dl><dt class="profile-profile_last">Last name</dt><dd class="profile-profile_last">Serbinski</dd><dt class="profile-profile_name">Name</dt><dd class="profile-profile_name">Ted</dd></dl>

            [#printed] => 1
        )

)

I can use all of the #title/#value pairs to do whatever I want if need be.

So yes, easier to theme IMO.

theme_dl() is still misnamed and underdocumented. See my last review for details.

If theme_dl holds up this important patch then fuck it. Now it's theme_profile_category.

Moved theme_profile_category to user.module. That this function should be theme_dl and reusable as such? Noone cares it seems. Then me neither.

Dries, compare please theme_dl and theme_user_profile. Zero escape and it should be so.

That doesn't help me. Where is $category escaped, for example? (Categories can be created through the administration page.) Maybe the old code was buggy too?

http://drupal.org/node/54898 there is a more general theme_definition_list.

So, now we do not trust user administrators to not enter XSS? What's the point? But again, I am happy to jump over whatever.

We're discussing this on the mailing list.

I'm still not sure why we have the useless #type=>item in some places. It is confusing because it is never actually used for anything.

I'd love to see a good theme_dl() in core, but it does not have to come with this patch (I'd prefer a patch before this so this can use it, but I can't be picky about the order of patches).

This is a good improvement since the elements of content are always at the same array positions (they are assigned sequentially now). But the double use of it, building ->content and then at the last minute switching to ->profile and overwriting ->content is confusing.

This does seem consistent with how nodes do it.

From my previous review:

"I'm still not sure why we have the useless #type=>item in some places. It is confusing because it is never actually used for anything."

Why not just omit is since theme_profile_category doesn't seem to look at it. I'm not asking for things to be reengineered. The #type is never used so why confuse people by saying there is a form item there?

"But the double use of it, building ->content and then at the last minute switching to ->profile and overwriting ->content is confusing."

Nothing was said about that.

#type item is now #type profile_item and thus the theme profile_category got simplified seriously. there is no ->profile any more. otherwise patch unchanged from the previous RTBC stage.

Credit goes to Ber for the generic idea of how theme_profile_item should look like.

Apparently I have no uploaded the file I intented to.

*sigh* Steven will kill me for posting in rapid succession but returning and rereading the whole issue dawned on me we lost a tpl.php along the way.

Excellent work guys! Code is cleaner and we still have our semantic DL and such. I'm happy with the new patch even if my theme_dl is gone ;-)

Good to go indeed!

Looking good.

We have this useful structured array of $account->content which is overwritten by a string right before we theme it. Wouldn't it be good to be able to access that array from the themable function?

The css probably needs to be in user.css. It isn't included if you have profile module turned off, which is the default.

.profile h3 is in profile.css while I'm 90% sure that h3 comes from user module.

Resurrecting and putting to "code needs review" since it's been a while ...

subscribing

No longer applies.