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...
Comment | File | Size | Author |
---|---|---|---|
#17 | 1021564-user-picture-style.patch | 2.11 KB | dawehner |
#3 | 1021564-user-picture-style.patch | 1.93 KB | Dave Reid |
user_picture_style_name.patch | 1.26 KB | Berdir | |
Comments
Comment #1
Stalski CreditAttribution: Stalski commentedTested manually, If it gets through, i will use this in heartbeat.
great job.
Comment #2
BerdirNote: Docs need to be updated, don't have time right now, anyone up for it?
Comment #3
Dave ReidAdded documentation.
Comment #4
bryancasler CreditAttribution: bryancasler commentedSubscribe
Comment #5
BerdirMoving to 8.x, needs to get in there first. Who can RTBC this... ? :)
Comment #6
BenK CreditAttribution: BenK commentedSubscribing
Comment #7
Stalski CreditAttribution: Stalski commentedbumping 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.
Comment #8
Dries CreditAttribution: Dries commentedNot 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?
Comment #9
Dave ReidYou don't like using the tertiary operators or is it something else? I guess we could do:
Comment #10
Stalski CreditAttribution: Stalski commentedI 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.
Comment #11
BerdirAt 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.
Comment #12
Eric_A CreditAttribution: Eric_A commentedThe 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.Comment #13
BerdirThat is not backward compatible and would make it impossible to backport this. So this is IMHO not an option.
Comment #14
Eric_A CreditAttribution: Eric_A commentedThe 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.
Comment #15
Eric_A CreditAttribution: Eric_A commentedThe 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.
Comment #16
BerdirYes 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?
Comment #17
dawehnerAdditional 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.
Comment #18
andypostusing 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
Comment #19
swentel CreditAttribution: swentel commentedUser picture is a field now, moving to D7 if anyone still wants this.
Comment #20
Elin Yordanov CreditAttribution: Elin Yordanov commentedBumping this patch to be reviewed.
Comment #21
nithinkolekar CreditAttribution: nithinkolekar commentedanother 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)