#493030: RDF #1: core RDF module is stuck due to annoyingly hard-coded implementation of theme_username(). That patch has a refactoring of it included with it, but I've thought more about improving that, and decided splitting that off into this issue rather messing with that patch.

I'll attach my proposed patch to this issue as soon as my extremely slow cvs connection finishes making it.

Note: there's a different issue, #192056: User's raw login name should not be output directly, that can probably be marked duplicate if this one is accepted.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Note: I marked this critical, because I've been told that getting RDF into core is critical.

effulgentsia’s picture

FileSize
84.47 KB
183.23 KB

I have no idea when my cvs connection will wake up and finish making the patch, so in the meantime, here's the 2 affected files. The relevant functions are:

theme.inc
----------------
theme_username()
template_preprocess_username()

common.inc
---------------
change to the 'username' signature in drupal_common_theme()

If someone with a functioning CVS connection feels inspired to roll this, go for it (note the base revision numbers at the top of each file).

effulgentsia’s picture

Status: Active » Needs review
FileSize
7.27 KB

Here's the patch.

pwolanin’s picture

Quick looks - seems like the right direction. The @param blocks inthe doxygen seem really long.

scor’s picture

thank you so much effulgentsia for starting the work on this. good idea to split it. I'll keep it in the series of RDF patches for now until this is committed.

dahacouk’s picture

Display Name Code Sprint Saturday September 5th...

Dave Cohen and I just had a really good face to face discussion with Angie (webchick). In summary, we are going to use the Saturday code sprint to make sure that Drupal 7 will facilitate us building contributed modules to implement display names.

This may mean altering some parts of core in a very small way. If you are passionate about cleaning up "theme_username" throughout core and making it easier for contributed modules to implement display names in a clean and consistent way, then join us at DrupalCon Paris or over the wires.

Saturday September 5th 2009 10:00 to 17:00 CEST
Location: DrupalCon Paris and online elsewhere

If you use IRC then #kendra is ready and waiting for you during those times.

I'll make sure developers are kept well feed and watered. So, if you are at DrupalCon and passionate about making it easy to implement display names then get in touch.

This is pretty much our last chance for making Drupal 7 display name friendly! Let's make it happen!

Cheers Daniel

Dave Cohen’s picture

Status: Needs review » Needs work

These are not duplicates, but affect the same function, so they will need to be reconciled when either is committed.

#192056: User's raw login name should not be output directly

Anonymous’s picture

subscribing so i can review when the next patch is added.

scor’s picture

Just did a quick review with Sun during the code sprint.

1. The order of the theme_username function should be reordered:
$object, $display_name, $link_path, $options

2. why is the 'access' option necessary? you could use the presence of $link_path to know if you have to output a link or not.

3. in template_preprocess_username(), rename $object to $account, which makes more sense.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
8.61 KB

This patch includes feedback from #4, #9, and discussions related to #192056: User's raw login name should not be output directly.

@sun: please review this to see if it's clear why order of params are the way they are.

Additional review from prior reviewers is welcome and would be much appreciated! Thanks.

effulgentsia’s picture

Apologies. I posted #10 too soon. Here's an update that also includes the ability to wrap another span (or other element) around the output. This is needed by RDF. With this patch, the RDF module will be able to put attributes on a span tag inside the link (by adjusting $processed['name']), on the link (by adjusting $processed['link_options']['attributes']), and on a span tag outside the link (by adjusting $processed['attributes']), all 3 of which are needed. Note, however, that themes still retain full control to change whatever the rdf module does, since their preprocess and process functions will run later, something demanded by themers.

catch’s picture

+ *   - 'no_link':
+ *     If set to TRUE, forces the output to not be a link. This can be used
+ *     when displaying a username as a block title or in other situations where
+ *     it's not desired for it to be a link.
+ *   - 'no_markup':
+ *     If set to TRUE, forces the output to not have markup of any kind. This
+ *     can be used when displaying a username within an attribute (for example,
+ *     the value of an "alt" attribute), within a plain text email, or in other
+ *     situations where no markup is desired.

We generally try to avoid negative variables. Any reason these can't be 'display_link' and 'display_markup' (or similar, not too keen on those names either).

pwolanin’s picture

It seems to me there is way too much logic inside the theme function still - still more of that should be handled by a preprocess function. The current code to me looks like it make it easy for themers to make mistakes.

Also, in the process function there are use of reference variables that seem out of line with typical core code style.

emmajane’s picture

#1: I've read the patch and the comments that talk about discussion, but I don't know what the actual discussion was. Can someone please summarize what this patch actually does and what happened in the conversations at Drupalcon? The themers will appreciate having these additional details instead of having to rely on mind melds. ;)

#2: As catch pointed out in #12 the TRUE for "negative" in the $options is downright confusing. This needs to be altered so that TRUE provides a positive and FALSE provides a negative of the action. (i.e. change from no_CONDITION to just CONDITION and update as appropriate).

pwolanin’s picture

One major goal is to be able to apply extra wrapper tags with attributes during the preprocess phase - this would support RDFa.

We have avoided making it a template due to performance concerns, but that's more-or-less what we want in the end.

I think trying to also make this function support a no-markup version is outside that scope of a single theme function - especially if we are never using it in core.

Here's a version that's closer to what core does now, but moves all the logic to pre+process, and give the themer an easy pre-constructed "safe_output". In addition, this should be mostly backward-compatible (though sub-optimal since extra attributes will be missing and sites will see double encoding).

the need for RDFa properties can be satisfied by the rdf module adding to $object->attributes during the preprocess phase.

pwolanin’s picture

catch suggests just using $variables['object'] instead of a separate $object as a reference to it.

effulgentsia’s picture

@pwolanin: as i read this, there's only a single object attributes variable (not counting the link-specific attributes rel and title) supported by your patch. Is that sufficient for RDF? For some reason, I thought RDF required one set of attributes on a span within the link and another set of attributes on a span outside the link. Did I misunderstand the needs for RDF?

pwolanin’s picture

@effulgentsia - my understanding is that the properties need to be applied to the element that contains the data. Of course, I'm not a deep expert by any means.

However, look at the example scor posted for title here: http://groups.drupal.org/node/22716

The a tge for the title has the RDFa property jsut as a link would get with this patch:

  <h2 ><a property="dc:title" href="/d701/node/5">title of the article</a></h2>

A single attributes variable is fine and is similar to what we have discussed for the main content - we can set any number of properties like:

  $object->attributes['properties'][] = 'foaf:name';
effulgentsia’s picture

@pwolanin: While I agree that it's out of scope for RDF, letting code that calls theme('username', ...) specify a desire for markup-free output would be used by core if the support was there for it. That's what #192056: User's raw login name should not be output directly is all about. It's insane that throughout core, usernames are sometimes not themed by theme('username') because they're used in contexts where markup (or at least a link) is not desired. If we're refactoring theme_username here anyway, I think it makes sense to add support for that, so #192056: User's raw login name should not be output directly can be updated to just clean up client code within core. If you'd be willing, please update your patch accordingly. Otherwise, we can defer on this here, and deal with it in #192056: User's raw login name should not be output directly once this much lands.

pwolanin’s picture

@effulgentsia - as above - it seems to me like that should be a different theme function all together.

effulgentsia’s picture

@pwolanin: Regarding #18, sounds good, but beyond my knowledge to confirm. If scor approves, so do I.

By the way, for themers evaluating this, there's a separate issue dealing with similar needs here: #569362: Document $attributes, $title_attributes, and $content_attributes template variables. However, that only applies to theme hooks implemented as templates. I agree with pwolanin that for several reasons, including performance, we do not want username as a template. I think that makes username unique as all other instances (I'm aware of) where RDF attributes are needed are templates. Sucks to have that exception, but as long as we provide a good default for the markup we're generating, and at least some mechanism by which a themer can override that markup, then at least we meet the minimum criteria for acceptability. Patch in comment #16 accomplishes that, but for a themer to override, they have to implement a process function and have at least mid-level knowledge of how to code PHP and what's going on in the previous preprocess and process functions.

floretan’s picture

Status: Needs review » Reviewed & tested by the community

The approach of the patch looks good. It passes the automated tests and I tested it manually too.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work

I did a more critical review and it does look good. I'd like 1 thing however.

Within template_process_username():

Either: 1)
Don't sanitize 'name' and 'extra'.

Or: 2)
Sanitize them earlier in the function, and before doing so, leave a copy of the unsanitized versions (e.g., "name_raw" and "extra_raw"). And then modify the lines that generate "safe_output" to not do their own sanitization.

My reasoning for wanting one of the above is that:
a) contrib modules running process functions should have access to the unsanitized versions.
b) if a themer wants to change how "safe_output" is generated, he or she should be able to copy the line that generates it from template_process_username() to use as a starting point and not have different rules apply for whether to sanitize or not.

pwolanin’s picture

@effulgentsia - yeah, I was thinking about that too and agree that both should be accessible raw. let me re-roll.

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
5.05 KB

this is along the line of option #1 - leaves the raw values in place and includes safe versions in addition for the themer.

note also this is really BC since name is not treated with check_plain.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work

Still not crazy about creating safe versions, but not giving themers an example of using them. How about this approach?

function template_process_username(&$variables) {
  // Create sanitized versions of name and extra.
  $variables['object']->safe_name = check_plain($variables['object']->name);
  $variables['object']->safe_extra = check_plain($variables['object']->extra);
  
  // Create a link_options property suitable for passing to l() function.
  if (isset($variables['object']->link_path)) {
    $variables['object']->link_options = array(
      'attributes' => $variables['object']->link_attributes + $variables['object']->attributes,
      // Don't want l() function to sanitize a second time.
      'html' => TRUE,
    );
  }
}

function theme_username($object) {
  if (isset($object->link_path)) {
    $output = l($object->safe_name . $object->safe_extra, $object->link_path, $object->link_options);
  }
  else {
    $output = '<span' . drupal_attributes($object->attributes) . '>' . $object->safe_name . $object->safe_extra . '</span>';
  }
  return $output;
}

This would allow themers to override the theme function without having to deal with preprocess or process functions.

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
5.16 KB

Here's a patch as Alex suggests - I don't feel strongly about this vs #25, but then I'm not a themer...

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work

Sorry to be so nitpicky here, but I looked through other theme functions (node, comment, etc.), that have preprocess functions, and in all cases, the convention is for the preprocess function to be responsible for security, not the process function. Also, the only place the "safe_" prefix is used is for field items, not for things like node title or comment signature, and I think we should treat 'name' and 'extra' more like node title and comment signature than like field items. While I'd be open to us refactoring all code to change to a convention of handling security in the process phase rather than the preprocess phase, until we do so, I think consistency is critical. We need to be able to give module authors a consistent message: either to handle security in the preprocess phase or not, but not have it be theme hook dependent.

So, please move the sanitization from process to preprocess. With that in mind, I actually think it's better to just have "name" and "extra" be the sanitized versions when template_preprocess_username() is done, and to not have the "safe_" variables. Modules implementing preprocess functions will know that these are sanitized, since that should always be the case after template_preprocess_* is done, and if they need access to the unsanitized versions, they can grab it from $object->account->name and variable_get().

We still need the process function for creating link_options, since the merging of link_attributes and attributes should happen after all preprocess functions have run.

Sorry to not have caught this earlier. I wasn't aware of the convention until I looked through other code in more detail.

pwolanin’s picture

@effulgentsia - I did it at the process phase thinking that we would thus relieve module authors of the need to do it themselves in the preprocess phase - however, this does reduce flexibility.

I'm really not sure about the naming - leaving the plain versions as non-safe makes the changes proposed BC to D6 - so old themes would work as expected even if missing the new attributes. however, if the worse case is double-encoding, I think that a minor breakage.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
5.19 KB

Every example I can find so far in core of a preprocess function is for a tpl file, not a theme function, so I'm not sure we really have a standard yet. But in any case, here's a re-roll. This makes the code a little simpler.

pwolanin’s picture

quoting (with permission) IRC exchange with merlin on the role of preprocess functions for theme functions:

[10:03am] pwolanin: merlinofchaos: i.e. can/should it be substantially transforming the input variable into something that's all safe to output?
[10:04am] pwolanin: merlinofchaos: I've figured out how they work, it's more a question of what is sensible to do there - questioning wrt this issue: http://drupal.org/node/565792
[10:05am] merlinofchaos: pwolanin: My guess is that if you're using them, then yes, it should treat the theme function as though it were a template and go ahead and transform the data. Of course, it's limited because it can't add new variables.
[10:05am] merlinofchaos: There's a lot of reasons why I worry that preprocess funcs for funcs is ultimately a bad idea, that they will be more frustrating than useful, but the demand was strong.

pwolanin’s picture

oops - made too much of a change - took the attribute merging out of the process phase. Adding it back here and making sure that link_options and link_attributes are always set during the preprocess phase, and also that we don't destroy any added link options.

pwolanin’s picture

through discussion of this patch w/ merlin and Heine, we've concluded that the current preprocess for functions system is rather broken - here's the proposed solution: #572618: All theme functions should take a single argument to make preprocess sane and meaningful

pwolanin’s picture

just restoring one code comment to the more verbose existing core comment text.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

discussed with Heine in IRC, but he couldn't post a bit ago when d.o was unreachable. Anyhow, he was satisfied enough with this patch in the absence of any other change to the theme system to improve preprocess function behavior.

fago’s picture

Status: Reviewed & tested by the community » Needs review

I agree that #572618: All theme functions should take a single argument to make preprocess sane and meaningful makes a lot of sense, however looking at this patch I wonder why do we built up a fake "object/account" if it's none? It's a collection of sanitized variables for the theme function, so why don't we put them directly in $variables like it's done for the templates too? That way we prevent people being confused about what's different between the "close-to-account-object" and a real $account.

fago’s picture

Status: Needs review » Reviewed & tested by the community

grml, cross-posted, re-setting status, although I still think those $object should go.

pwolanin’s picture

@fago - we cannot construct a new argument for the function - that's really what the other issue is about.

effulgentsia’s picture

I'm en-route from Paris back to the US today, but will review tomorrow, whether it's committed by then or not. Thanks for all the work on this, Peter!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

This looks like the right thing to do. Although I'm a bit worried about the performance implications, I committed this to CVS HEAD.

fago’s picture

@pwolanin: ah, I see!

catch’s picture

Status: Fixed » Active

Back in the profiler again.

$object->extra is only set to:

$variables['object']->extra = ' (' . t('not verified') . ')';

But at the end of template_preprocess_username() we do check_plain() - which means two check_plain() for every theme('username') on a page. Since t() is trusted content, do we really need to check_plain() that?

Please mark back to fixed if I've missed something obvious.

salvis’s picture

EDIT: false alarm, please ignore this!

The patch in #34 broke theme_username():

function theme_username($object) {
  if (isset($object->link_path)) {
    // We have a link path, so we should generate a link using l().
    // Additional classes may be added as array elements like
    // $object->link_options['attributes']['class'][] = 'myclass';
    $output = l($object->name . $object->extra, $object->link_path, $object->link_options);
  }
  else {
    // Modules may have added important attributes so they must be included
    // in the output. Additional classes may be added as array elements like
    // $object->attributes['class'][] = 'myclass';
    $output = '<span' . drupal_attributes($object->attributes) . '>' . $object->name . $object->extra . '</span>';
  }
  return $output;
}

If we pass the anonymous $user, then $object has neither attributes, nor name, nor extra. The result is

* Notice: Undefined property: stdClass::$attributes in theme_username() (line 1961 of drupal-head\includes\theme.inc).
* Recoverable fatal error: Argument 1 passed to drupal_attributes() must be an array, null given, called in drupal-head\includes\theme.inc on line 1961 and defined in drupal_attributes() (line 2306 of drupal-head\includes\common.inc).

salvis’s picture

Status: Active » Needs review
FileSize
1.97 KB

EDIT: false alarm, please ignore this!

Here's what I had to do to recover from the fatal error (WSOD with error message).

pwolanin’s picture

Status: Active » Needs work

I thought we handled this in the preprocess function? If not there is a logic flaw there

@salvis - are you calling theme_username() directly? If so, you need to reproduce the preprocessing.

pwolanin’s picture

Status: Needs work » Postponed (maintainer needs more info)

I just enabled PHP filter and this works fine as node content:

print theme('username', drupal_anonymous_user());

as does:

$account = user_load(0);
print theme('username', $account);

please explain the steps to reproduce your bug.

salvis’s picture

Status: Postponed (maintainer needs more info) » Active

Yes, this was just what I was doing:

    $account = user_load(0);
    print theme('username', $account);

In the meantime I've reinstalled my test installation and now it's working again. I'm sorry about the false alarm...

(Setting status back to active, as it was after #42.)

pwolanin’s picture

FileSize
634 bytes

@catch - good point - I think those calls were originally in the _process function where possibly other modules would have added to extra. However, Alex suggested essentially that any module that adds more to extra or name has to be sure it's safe, so we should remove this call since the requirment to make sure added data is safe is already documented in the doxygen.

Dries’s picture

Status: Needs review » Reviewed & tested by the community

This looks good. Marking RTBC. Will commit later.

coderintherye’s picture

Status: Reviewed & tested by the community » Needs work

Edit: Ok just read through this thread and see I am getting the same error as was happening in #43 http://drupal.org/node/565792#comment-2033958

However, I don't see any actual fix for this, why is it a false alarm? This is a fresh install. Or am I missing something?

Re-opening this based on pwolanin's advice that I post here. If one applies the patch from http://drupal.org/node/572618#comment-2037444 then the following error comes up on a fresh drupal 7 install.

Notice: Undefined property: stdClass::$attributes in theme_username() (line 1975 of C:\drive\xampplite\htdocs\drupal7\includes\theme.inc).
Recoverable fatal error: Argument 1 passed to drupal_attributes() must be an array, null given, called in C:\drive\xampplite\htdocs\drupal7\includes\theme.inc on line 1975 and defined in drupal_attributes() (line 2306 of C:\drive\xampplite\htdocs\drupal7\includes\common.inc).

I'm not sure how this coincides with the issue/patch at hand in this thread, but this is where pwolanin directed me. I'd really like to see the realname functionality make it into core so whatever I can do to help that please suggest.

catch’s picture

Status: Needs work » Reviewed & tested by the community

The patch in #48 is still RTBC so moving back just for that.

I can't reproduce the bug that anarchman's getting, but would be good to narrow it down.

@salvis - how did you resolve that?

coderintherye’s picture

@catch - are you installing any other patches or just the one that I linked to on a fresh install?

pwolanin’s picture

It looks possibly like a theme registry rebuild is needed to pick up the preprocess function?

salvis’s picture

I got the error in #43 after updating via CVS from a state before the commit in #40. To recover, I flushed the database and reinstalled from scratch. In hindsight, rebuilding the theme registry would probably have been sufficient.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Dave Cohen’s picture

Oops. replying here was an accident. (Nice work on this.)

Status: Fixed » Closed (fixed)
Issue tags: -RDF

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