Moodle

Extra utf decoding in external DB (patch included)

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Fixed
  • Affects Version/s: 1.5.3
  • Fix Version/s: 1.8
  • Component/s: Authentication
  • Labels:
    None
  • Environment:
    All
  • Affected Branches:
    MOODLE_15_STABLE
  • Fixed Branches:
    MOODLE_18_STABLE

Description

Extra utf decoding in external DB. It damage firstname etc.

Maybe should we check encoding here?

Index: auth/db/lib.php

===================================================================

RCS file: /cvsroot/moodle/moodle/auth/db/lib.php,v

retrieving revision 1.15.2.2

diff -u -r1.15.2.2 lib.php

— auth/db/lib.php 31 Aug 2005 05:59:31 -0000 1.15.2.2

+++ auth/db/lib.php 21 Apr 2006 10:04:24 -0000

@@ -76,7 +76,7 @@

if ($rs = $authdb->Execute(SELECT .$pcfg[field_map_$field]. FROM $CFG->auth_dbtable

WHERE $CFG->auth_dbfielduser = '$username')) {

if ( $rs->RecordCount() == 1 ) { - $result[$field] = addslashes(stripslashes(utf8_decode($rs->fields[0]))); + $result[$field] = addslashes(stripslashes($rs->fields[0])); //dlnsk (extra utf8_decode removed) }

}

}

Issue Links

Activity

Hide
Petr Škoda (skodak) added a comment -

to be fixed in 1.8

Show
Petr Škoda (skodak) added a comment - to be fixed in 1.8
Hide
Martín Langhoff added a comment -

Hola Petr! If you are taking this issue over, a few quick notes from me:

  • It only makes sense if moodle db is running in unicode (I was thinking of making it conditional in 1.6)
  • I don't quite agree with the patch as proposed :-/ though it'll make it work if both DBs are in utf-8 and the connection set as such
  • Most databases should support being told SET NAMES('utf-8') – and that's actually the best solution: the db assumes we'll pass it utf-8 and it takes care of transcoding of all data we pass. Important for $vars we pass in WHERE clauses and potential UPDATEs. I think the only thing we update right now is the password, but this could change.
  • If SET NAMES is not supported, then it's a mess, as we'll have to ask the user about the ext db encoding, and then mess around with encode/decode. Don't think it's worth it, because any modern DB that can deal with interesting charsets should support SET NAMES. And users with interesting charsets tend to have modern databases for that exact reason.
Show
Martín Langhoff added a comment - Hola Petr! If you are taking this issue over, a few quick notes from me:
  • It only makes sense if moodle db is running in unicode (I was thinking of making it conditional in 1.6)
  • I don't quite agree with the patch as proposed :-/ though it'll make it work if both DBs are in utf-8 and the connection set as such
  • Most databases should support being told SET NAMES('utf-8') – and that's actually the best solution: the db assumes we'll pass it utf-8 and it takes care of transcoding of all data we pass. Important for $vars we pass in WHERE clauses and potential UPDATEs. I think the only thing we update right now is the password, but this could change.
  • If SET NAMES is not supported, then it's a mess, as we'll have to ask the user about the ext db encoding, and then mess around with encode/decode. Don't think it's worth it, because any modern DB that can deal with interesting charsets should support SET NAMES. And users with interesting charsets tend to have modern databases for that exact reason.
Hide
Martín Langhoff added a comment -

In short, my plan was to

  • drop utf8_decode()
  • copy whatever Eloy has implemented as a portable way to perform SET NAMES() in configure_dbconnection(). Hmmm. Reading the comments around that function is quite interesting - perhaps we should copy those to documentation (on page or wikipage)
  • DBs that don't support USE NAMES() or conf/env vars (see configure_dbconnection() ) are probably too hard to support.
Show
Martín Langhoff added a comment - In short, my plan was to
  • drop utf8_decode()
  • copy whatever Eloy has implemented as a portable way to perform SET NAMES() in configure_dbconnection(). Hmmm. Reading the comments around that function is quite interesting - perhaps we should copy those to documentation (on page or wikipage)
  • DBs that don't support USE NAMES() or conf/env vars (see configure_dbconnection() ) are probably too hard to support.
Hide
Petr Škoda (skodak) added a comment -

Hola!

Thanks for the info, my plans were similar.

Judging from MDL-6332 we will have only unicode in 1.8 which makes coding easier. I think we could make the encoding conversion and database setup configurable or at least properly documented so that people can tweak it.

Show
Petr Škoda (skodak) added a comment - Hola! Thanks for the info, my plans were similar. Judging from MDL-6332 we will have only unicode in 1.8 which makes coding easier. I think we could make the encoding conversion and database setup configurable or at least properly documented so that people can tweak it.
Hide
Martin Dougiamas added a comment -

Petr can you nail this one?

Show
Martin Dougiamas added a comment - Petr can you nail this one?
Hide
Eloy Lafuente (stronk7) added a comment -

Petr,

please take a look to MDL-8479. it's related and solution is provided here...I think.

Ciao

Show
Eloy Lafuente (stronk7) added a comment - Petr, please take a look to MDL-8479. it's related and solution is provided here...I think. Ciao
Hide
Petr Škoda (skodak) added a comment -

working on it...

Show
Petr Škoda (skodak) added a comment - working on it...
Hide
Petr Škoda (skodak) added a comment -

fixed in cvs, thanks for the report!

Show
Petr Škoda (skodak) added a comment - fixed in cvs, thanks for the report!

Dates

  • Created:
    Updated:
    Resolved: