Title says it all, this patch adds support for the style_name argument to override the default. This currently requires http://drupal.org/project/imagecache_profiles.

Not sure if this should be set to 8.x already, feel free to change that...

Why this patch needs to be commited!

+ Adds an important feature that currently requires a contrib module. Image style support for user pictures in core is quite useless when you can't use it...
+ Tiny patch, only 3 changed lines
+ Backwards compatible

Please review...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Stalski’s picture

Tested manually, If it gets through, i will use this in heartbeat.
great job.

Berdir’s picture

Note: Docs need to be updated, don't have time right now, anyone up for it?

Dave Reid’s picture

Added documentation.

bryancasler’s picture

Subscribe

Berdir’s picture

Version: 7.x-dev » 8.x-dev

Moving to 8.x, needs to get in there first. Who can RTBC this... ? :)

BenK’s picture

Subscribing

Stalski’s picture

Status: Needs review » Reviewed & tested by the community

bumping this.
Totally forgot it was not in core yet. Heartbeat does not work without it actually. Can anyone give feedback on whether this will be put in or not. If not, I'll create a custom imagestyle.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
+++ modules/user/user.module	10 Jan 2011 16:23:24 -0000
@@ -1450,9 +1452,10 @@ function template_preprocess_user_pictur
+      $style = !empty($variables['style_name']) ? $variables['style_name'] : variable_get('user_picture_style', '');

Not sure I like these kind of constructs. Doesn't really make for very transparent code in my opinion. Can we do something cleaner for Drupal 8?

Dave Reid’s picture

You don't like using the tertiary operators or is it something else? I guess we could do:

if (empty($variables['style_name'])) {
  $variables['style_name'] = variable_get('user_picture_style', '');
}
Stalski’s picture

I think we should make sure the style_name is always set when the preprocessor comes in. When user properties are set on the node, user or comment object, the style name might come together with the picture.

Berdir’s picture

At some point, we do need to check if one was passed and use the default if not, I'm not sure what the problem is with that code.

The only alternative would be to already set the default in hook_theme() but that would mean that changing the default would a require theme registry rebuild.

Eric_A’s picture

The only alternative would be to already set the default in hook_theme() but that would mean that changing the default would a require theme registry rebuild.

There is another alternative: if you're going to inject the style name, have it always injected and let the theme() callers do the variable_get('user_picture_style', '') call.

Berdir’s picture

That is not backward compatible and would make it impossible to backport this. So this is IMHO not an option.

Eric_A’s picture

The drop is always moving... let's worry about possible D7-implementations when the new feature is in D8, *if* we're going to spend time back porting this new feature to D7. (meaning: if there is a chance of if getting in...)
Any D7 contrib module that wishes to use a new feature will have to choose between declaring a dependency on a certain minor version or deal with the fact that that it might or might not work. This information must be in the doc block.
EDIT: removed my comment about D7 API breakage. It might not be valid and I think that discussion should be better left alone for now.

Eric_A’s picture

The drop is always moving... let's worry about possible D7-implementations when the new feature is in D8

My point being that we should not hold back clear improvements for the sake of keeping backport patches as simple as possible.

Berdir’s picture

Yes sure, but I don't see the improvement in your suggestion. We do have a default for that value, why should every module using this have to provide that default value explicitly?

Core alone calls this theme functions at least 6 times. How is it simpler to update all these instead of that simple if condition?

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.11 KB
There is another alternative: if you're going to inject the style name, have it always injected and let the theme() callers do the variable_get('user_picture_style', '') call.

Additional it's somehow bad coder experience. If such a function is hard to use people will use the picture manually again.

Here is a patch which implements exactly what davereid suggested with a version which works on git.

andypost’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
+++ b/modules/user/user.moduleundefined
@@ -1459,10 +1461,13 @@ function template_preprocess_user_picture(&$variables) {
+      if (module_exists('image') && file_valid_uri($filepath) && $variables['style_name']) {

using file_valid_uri() is not good. This limits ability to poing to file - only public:// private:// and ..proto://
but this does not work if path like ./themes/bartik/logo.png

This bring a lot of questions in imagecache_profiles module - #1107464: user default image doesn't get formatted

So pay special attension about documentation and tests

swentel’s picture

Version: 8.x-dev » 7.x-dev

User picture is a field now, moving to D7 if anyone still wants this.

Elin Yordanov’s picture

Issue summary: View changes
Status: Needs work » Needs review

Bumping this patch to be reviewed.

nithinkolekar’s picture

another patch with "needs review" for core.

~40 issues for user module with an average of ~3 years old :(.

DA should initiate issue clearance day every month with either acceptance of patch to core or rejection(if they really don't want contributors to contribute to core enhancement/issues)