At the inner div.

Files: 
CommentFileSizeAuthor
#7 non_ascii_character-421294-7.patch416 bytessuperspring
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch non_ascii_character-421294-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 non_ascii_character-421294-3.patch436 bytessuperspring
PASSED: [[SimpleTest]]: [MySQL] 59,017 pass(es).
[ View ]

Comments

futuresoon’s picture

It just looks that way in WordPad. Try notepad++

lyricnz’s picture

Version:6.10» 8.x-dev

The hexdump of that line is

grep -A10 theme_user_signature core/modules/user/user.module | grep '<div>' | hexdump -C
00000000  20 20 20 20 24 6f 75 74  70 75 74 20 2e 3d 20 27  |    $output .= '|
00000010  3c 64 69 76 3e e2 80 94  3c 2f 64 69 76 3e 27 3b  |<div>...</div>';|
00000020  0a                                                |.|
00000021

So the characters between the div and close-div are: e2 80 94 which is unicode for &mdash; Would it be better to use the the &mdash; or are we OK with unicode in the output markup?

superspring’s picture

Status:Active» Needs review
StatusFileSize
new436 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,017 pass(es).
[ View ]

So we can protect against editors which don't support unicode editing that symbol and file transfer protocol which modify the non-ASCII contents of files I think it should be named to the HTML character reference.

Here is a patch which updates this.

lyricnz’s picture

Status:Needs review» Reviewed & tested by the community

Works for me.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks!

David_Rothstein’s picture

Version:8.x-dev» 7.x-dev
Status:Fixed» Patch (to be ported)
Issue tags:+needs backport to D7
superspring’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new416 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch non_ascii_character-421294-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Same patch as above but for D7.

TR’s picture

Status:Needs review» Reviewed & tested by the community

Trivial, and already committed to D8.

TR’s picture

David_Rothstein’s picture

Title:non-ascii character in function theme_user_signature($signature)» (rollback?) non-ascii character in function theme_user_signature($signature)
Version:7.x-dev» 8.x-dev
Issue summary:View changes
Status:Reviewed & tested by the community» Needs review

On further thought, should we really do this?

There is unicode all over the Drupal codebase, including in HTML output, and including several instances of this exact same character.

At the very least, we should be consistent.

And I am also under the impression that Drupal generally prefers unicode characters to HTML entities, because they're more portable.

If you text editor can't handle unicode, I think you need a better text editor :)

lyricnz’s picture

IIRC, at the time the original patch was submitted, this was the only instance (of this character at least).

FWIW, there doesn't seem to be a clear consensus online about Unicode vs Character Entities (both have their advantages/disadvantages) - http://en.wikipedia.org/wiki/Unicode_and_HTML

As far as I understand (and I'm not terribly current on this), the theme_* functions are tied explicitly to the HTML output mechanism (as compared to web-services etc), so the difference between entities and unicode is probably mostly about maintainability rather than anything semantic.

TR’s picture

I personally don't really care one way or the other, but I think we should be consistent. This issue was reported against D6 and took 4+ years to get into D8, the least we could do is backport this trivial change to D7. Or revert the D8 change and close the issue. I'm not sure why tiny changes always take more effort than major rewrites.

#657166: use × instead of x is similar, but in that it's proposed to replace the ASCII with Unicode, sort of the opposite of what we're doing here ...

webchick’s picture

Um. It took one week to get this in 8.x, once a patch was supplied. We can't commit changes to D8 if we have no patch. So let's please be accurate. :)

I personally lean towards reverting the D8 patch, but it's unclear what the actual problem was that was being solved, since the issue summary is a single sentence.

Damien Tournoud’s picture

Title:(rollback?) non-ascii character in function theme_user_signature($signature)» Rollback: non-ascii character in function theme_user_signature($signature)

Yes, please rollback. There are unicode characters all over the code base, and that's how it should be.

superspring’s picture

In my opinion we shouldn't force editors on developers. Having less unicode in the code base is better in my opinion. Non-unicode editors are rare, but I'd dislike breaking a file by using one if I needed to.

sun’s picture

Title:Rollback: non-ascii character in function theme_user_signature($signature)» REVERT: non-ascii character in function theme_user_signature($signature)
Status:Needs review» Reviewed & tested by the community

HTML entities are a thing of the past, the web is UTF-8/Unicode today.

Drupal core uses » and « and — and → and even ' all over the place, for good reasons.

I wholeheartedly agree with @Damien Tournoud, this patch should be reverted...

...but in the end, this commit does not matter, because theme_user_signature() is dead code in D8. :P

#1548204: Remove user signature and move it to contrib

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 7: non_ascii_character-421294-7.patch, failed testing.

sun’s picture

Status:Needs work» Reviewed & tested by the community
webchick’s picture

Status:Reviewed & tested by the community» Closed (won't fix)

Ok, seems we're in agreement, so reverted.

Also marking this "won't fix" since it goes against what we do elsewhere.

  • Commit 068dd71 on 8.x by webchick:
    Revert Issue #421294: We use unicode, not html entities.