Moodle

Mediaplugin filter returns blank $text when bad UTF-8 is found

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.7.2
  • Fix Version/s: 1.7.3, 1.8.3, 1.9
  • Component/s: Filters, Resource, Unicode
  • Labels:
    None
  • Environment:
    PHP5-only issue.
  • Affected Branches:
    MOODLE_17_STABLE
  • Fixed Branches:
    MOODLE_17_STABLE, MOODLE_18_STABLE, MOODLE_19_STABLE

Description

I've been hours trying to find out why, under certain circumstances, a link like "/file.php/263/onefile.htm" would return a blank page while "/file.php/263/otherfile.htm" does not. Tracing took me through file.php->filelib.php (send_file function) -> filter_text function and finally to mediaplugin_filter function, where I found that preg_replace() was the one returning a blank $text.

The problem arose when upgraded the server to PHP5, so I suspect preg_replace() is returning an empty $text when the original $text is not well-formed UTF-8. This is very probable, since the files in moodledata not necessarily are in UTF-8 (this particular installation was a 1.4, then 1.5, and now 1.7.2).

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Pablo Etcheverry added a comment -

I've made an ugly hack to solve this problem. I just check the result of preg_replace and if it is null, I simply ignore it and replace it with the original text.

filter/mediaplugin/filter.php:

-$text = preg_replace($search, $replace, $text);

+$newtext = preg_replace($search, $replace, $text);
+$text = $newtext ? $newtext : $text;

Show
Pablo Etcheverry added a comment - I've made an ugly hack to solve this problem. I just check the result of preg_replace and if it is null, I simply ignore it and replace it with the original text. filter/mediaplugin/filter.php: -$text = preg_replace($search, $replace, $text); +$newtext = preg_replace($search, $replace, $text); +$text = $newtext ? $newtext : $text;
Hide
Permalink
Martin Dougiamas added a comment -

Nick, can you please check this in to 1.7, 1.8 and head?

Show
Martin Dougiamas added a comment - Nick, can you please check this in to 1.7, 1.8 and head?
Hide
Permalink
Joseph Rézeau added a comment - - edited

Nicolas,
You shouldn't have applied Pablo's lovely/ugly hack... Now, the mutlimedia filter is broken: it won't filter HTML uploaded files or Composed Web pages or other resources etc.for MP3 links to display the MP3 player. GRRRR!

The problem appears when more than one filter is enabled (which is the usual case). Then, the $text passed as parameter is lost because of the $newtext variable you introduced...

Joseph

Show
Joseph Rézeau added a comment - - edited Nicolas, You shouldn't have applied Pablo's lovely/ugly hack... Now, the mutlimedia filter is broken: it won't filter HTML uploaded files or Composed Web pages or other resources etc.for MP3 links to display the MP3 player. GRRRR! The problem appears when more than one filter is enabled (which is the usual case). Then, the $text passed as parameter is lost because of the $newtext variable you introduced... Joseph
Hide
Permalink
Petr Škoda added a comment -

Oh - I did not notice this bug, seems to be similar to MDL-10155 - I guess there should be an is_null() check instead, see:
http://moodle.cvs.sourceforge.net/moodle/moodle/filter/multilang/filter.php?r1=1.19&r2=1.20

Show
Petr Škoda added a comment - Oh - I did not notice this bug, seems to be similar to MDL-10155 - I guess there should be an is_null() check instead, see: http://moodle.cvs.sourceforge.net/moodle/moodle/filter/multilang/filter.php?r1=1.19&r2=1.20
Hide
Permalink
Nicolas Connault added a comment -

Sorry Joseph, this is fixed now. I must not have been thinking straight

Show
Nicolas Connault added a comment - Sorry Joseph, this is fixed now. I must not have been thinking straight
Hide
Permalink
Nicolas Connault added a comment -

I haven't tested this, but I haven't heard any complaints since I fixed it and committed it over a week ago, so I'm now closing the bug.

Show
Nicolas Connault added a comment - I haven't tested this, but I haven't heard any complaints since I fixed it and committed it over a week ago, so I'm now closing the bug.

People

Vote (0)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: