Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
But default picture (if exist) appears for all users (including users with custom picture selected).
Comment | File | Size | Author |
---|---|---|---|
#23 | 1058564-interdiff-23.patch | 688 bytes | Niklas Fiekas |
#23 | 1058564-comment-picture-preview-23-test.patch | 1.7 KB | Niklas Fiekas |
#23 | 1058564-comment-picture-preview-23.patch | 2.27 KB | Niklas Fiekas |
#23 | comment-preview-before.png | 13.44 KB | Niklas Fiekas |
#23 | comment-preview-after.png | 21.41 KB | Niklas Fiekas |
Comments
Comment #1
montesq CreditAttribution: montesq commentedOn my side, the default icon is always displayed in the preview mode. But when the comment is finally submitted, the actual user's picture is displayed.
(set the issue status "active" as long as nobody submits a patch for this issue)
Comment #2
droplet CreditAttribution: droplet commentedI'm also removed check_plain for account name. it's load from DB and should be safe ?? if not, we need to add check_plain for UID ?
Comment #3
montesq CreditAttribution: montesq commentedThanks Droplet for this patch, it fixes the picture issue in my case.
For the check_plain, I let someone else (more skilled than me) confirm that point before marking as RTBC...
Comment #5
grendzy CreditAttribution: grendzy commentedcheck_plain is always applied to the account name because it's user-supplied data, unlike the uid.
Here's updated patches for D6 and D7.
Comment #6
grendzy CreditAttribution: grendzy commentedComment #7
droplet CreditAttribution: droplet commented#5,
Yes, it's user-supplied data. But does it come from COMMENT submit directly or DB ??
when insert into DB, it already filtered unsafe data, should we recheck again??
Comment #8
grendzy CreditAttribution: grendzy commentedEven though usernames are somewhat constrained during account registration, my understanding is our convention is to always check_plain them on output. See template_preprocess_username for an example.
Comment #9
Dave ReidAck! Can we fix the name output to be using format_username($account) please?
Comment #10
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedSubscribe.
Edit: What happened with the tags? I didn't do anything.
Comment #11
Shyamala CreditAttribution: Shyamala commentedAll looks good and the patch is working. But we must use theme ('user_picutre',... option so we can provide for an override of the user picture image theme generically.
Should be use theme_get_setting('toggle_comment_user_picture') before displaying the image? the code in preview must match that in comment render at the field level.
Also like to highlight that there is no default image for a user who has not uploaded his profile image or for anonymous user.
Comment #12
droplet CreditAttribution: droplet commentedhere isn't generate the real code? you should do that later (theme)
user_name, I create a new issue:
#1243882: Use format_username() in comment_preview()
Comment #13
droplet CreditAttribution: droplet commentedComment #14
dixon_The patch in #5 was made from the wrong directory. It also had an offset. Here is a re-roll.
This patch fixes the user picture in comment preview.
Re #11: I can't reproduce the problem you are experiencing with default images. Everything works as expected for me.
Comment #15
dixon_The patch applies to D7 without offset. So it should be possible for a straight backport. I haven't been able to test on D6 yet.
Comment #16
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedSimilar problem: What about the signature? I wonder if there is a more generic solution.
Comment #17
dixon_@Niklas You are right. I tested, we have the same problem with signatures -- they aren't visible in comment preview. Can you open a new issue for that?
Comment #18
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedHere it is: #1267918: User signature does not appear in comment preview.
But why aren't signatures and user pictures attached in user_comment_view (implementation of hook_comment_view) in the first place?
Comment #19
naxoc CreditAttribution: naxoc commentedI threw in tests for the user picture. There is a patch for both D7 and D8 - it is an issue in both versions.
Comment #20
Niklas Fiekas CreditAttribution: Niklas Fiekas commented#19: comment_preview_pic_D8-44930-8.patch queued for re-testing. (To quickly see if it still applies after #1267918: User signature does not appear in comment preview got in.)
Comment #22
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOk, here's a rebase.
Comment #23
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedRerolled the patch removing t() from the assert. Also made some screenshots.
Before the patch:
(That the placeholder user picture is used before the patch is another +1 for backporting.)
After the patch:
For completeness: It looks like this, before and after the patch, when user pictures are disabled:
(Ugg ... that interdiff should have been a .txt)
Comment #24
xjmAwesome, thanks @Niklas Fiekas! The screenshot showing the placeholder image gives me a lot more confidence in us backporting this.
Pending testbot, I think this is probably RTBC.
Comment #25
xjmYay!
Comment #26
webchickWe talked about this on IRC a fair amount, in terms of its applicability to D7. There's a risk here that we break themes in the wild who were not accounting for the image on comment preview. Technically, this bug fix represents a data structure change. However, in the end, bringing consistency to the comment display in both preview and "real" view seems worth taking the risk, since without it preview is just "sort of half assed preview" and doesn't actually show users what's about to be posted. :P
Therefore, with fingers crossed, committed and pushed to 8.x and 7.x. It'll be interesting to see reactions of themers from this change in 7.13, and might be a good method of helping to resolve some unanswered concerns at http://groups.drupal.org/node/210973.
Comment #27
webchickTagging as something to mention in the 7.13 release notes.
Comment #28
Dave ReidLooks like this accidentally committed parts of #1064954: _node_revision_access() static usage - see http://drupalcode.org/project/drupal.git/commit/186437d.
Comment #29
webchickOops! Thanks a lot for catching that. Reverted and re-patched. Sorry about that.
Comment #30
Tor Arne Thune CreditAttribution: Tor Arne Thune commented