Moodle
  1. Moodle
  2. MDL-20821

Evaluate best way to handle PHP deprecation of ereg_replace and eregi_replace function in PHP 5.3.0

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.9.6, 2.0
    • Fix Version/s: 2.0
    • Component/s: General
    • Labels:
      None
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      32850

      Description

      With the MRBS block I was recently alerted to the PHP deprecation (as of 5.3.0) of ereg_replace. It looks like through most of Moodle 2.0 the ereg_replace and eregi_replace calls have been replaced with preg_replace; however, a search of the code base shows that it is still present in some places in Moodle 2.0 and widely through 1.9. Since preg_replace is PHP 4 compatible it seems safe to just replace it but with PHP 5.3.0 being the current stable release it seems important to make sure we clean up the code so as to avoid folks getting the deprecation warnings. Peace - Anthony

      1. 091118_MDL20821_19patch.txt
        135 kB
        Andrew Davis
      2. MDL20821_19patch.txt
        45 kB
        Andrew Davis
      3. MDL20821trunkpatch.txt
        15 kB
        Andrew Davis
      4. MDL20821trunkpatch2.txt
        49 kB
        Andrew Davis
      5. regextest.php
        0.4 kB
        Andrew Davis

        Issue Links

          Activity

          Hide
          Anthony Borrow added a comment -

          Just for easy reference:

          http://www.php.net/manual/en/function.ereg-replace.php
          http://www.php.net/manual/en/function.eregi-replace.php

          http://www.php.net/manual/en/function.preg-replace.php

          Peace - Anthony

          p.s. - We should probably look at all of the ereg related functions (ereg, eregi, split, etc.) in fact here is a list of all the deprecated 5.3.0 functions - http://php.net/manual/en/migration53.deprecated.php

          Show
          Anthony Borrow added a comment - Just for easy reference: http://www.php.net/manual/en/function.ereg-replace.php http://www.php.net/manual/en/function.eregi-replace.php http://www.php.net/manual/en/function.preg-replace.php Peace - Anthony p.s. - We should probably look at all of the ereg related functions (ereg, eregi, split, etc.) in fact here is a list of all the deprecated 5.3.0 functions - http://php.net/manual/en/migration53.deprecated.php
          Hide
          Tim Hunt added a comment -

          I think we should use preg everywhere. (And, wherever possible, we should try to write unit tests for code that mangles strings using regexs.) Thus, this might be a good introductory job for one of the new developers Martin has recruited recently.

          Show
          Tim Hunt added a comment - I think we should use preg everywhere. (And, wherever possible, we should try to write unit tests for code that mangles strings using regexs.) Thus, this might be a good introductory job for one of the new developers Martin has recruited recently.
          Hide
          Anthony Borrow added a comment -

          p.s. - My thanks to Roy Overthrow for alerting me to the deprecation issue for which I created CONTRIB-1619

          Show
          Anthony Borrow added a comment - p.s. - My thanks to Roy Overthrow for alerting me to the deprecation issue for which I created CONTRIB-1619
          Hide
          Martin Dougiamas added a comment -

          All yours Andrew!

          Show
          Martin Dougiamas added a comment - All yours Andrew!
          Hide
          Andrew Davis added a comment -

          Patch for trunk. Patch for 1.9 to follow. About 120 instances of ereg_replace and eregi_replaced in total.

          Closely related to this is the 270 instances of ereg() and eregi() that need to be replaced with preg_match.

          Show
          Andrew Davis added a comment - Patch for trunk. Patch for 1.9 to follow. About 120 instances of ereg_replace and eregi_replaced in total. Closely related to this is the 270 instances of ereg() and eregi() that need to be replaced with preg_match.
          Hide
          Andrew Davis added a comment -

          1.9 patch attached. Putting this issue on hold. All instances of ereg_replace and eregi_replace are gone in both branches. I'll return later today to remove ereg and eregi.

          Show
          Andrew Davis added a comment - 1.9 patch attached. Putting this issue on hold. All instances of ereg_replace and eregi_replace are gone in both branches. I'll return later today to remove ereg and eregi.
          Hide
          Anthony Borrow added a comment -

          Thanks for your work on this. I'm copying this from the above referenced PHP page about deprecated functions. Another one that I would search for is split but just to be thorough you might want to review the following list:

          — from http://php.net/manual/en/migration53.deprecated.php
          Deprecated functions:

          • call_user_method() (use call_user_func() instead)
          • call_user_method_array() (use call_user_func_array() instead)
          • define_syslog_variables()
          • dl()
          • ereg() (use preg_match() instead)
          • ereg_replace() (use preg_replace() instead)
          • eregi() (use preg_match() with the 'i' modifier instead)
          • eregi_replace() (use preg_replace() with the 'i' modifier instead)
          • set_magic_quotes_runtime() and its alias, magic_quotes_runtime()
          • session_register() (use the $_SESSION superglobal instead)
          • session_unregister() (use the $_SESSION superglobal instead)
          • session_is_registered() (use the $_SESSION superglobal instead)
          • set_socket_blocking() (use stream_set_blocking() instead)
          • split() (use preg_split() instead)
          • spliti() (use preg_split() with the 'i' modifier instead)
          • sql_regcase()
          • mysql_db_query() (use mysql_select_db() and mysql_query() instead)
          • mysql_escape_string() (use mysql_real_escape_string() instead)
          • Passing locale category names as strings is now deprecated. Use the LC_* family of constants instead.
          • The is_dst parameter to mktime(). Use the new timezone handling functions instead.

          Deprecated features:

          • Assigning the return value of new by reference is now deprecated.
          • Call-time pass-by-reference is now deprecated.
          • The use of {} to access string offsets is deprecated. Use [] instead.
          Show
          Anthony Borrow added a comment - Thanks for your work on this. I'm copying this from the above referenced PHP page about deprecated functions. Another one that I would search for is split but just to be thorough you might want to review the following list: — from http://php.net/manual/en/migration53.deprecated.php — Deprecated functions: call_user_method() (use call_user_func() instead) call_user_method_array() (use call_user_func_array() instead) define_syslog_variables() dl() ereg() (use preg_match() instead) ereg_replace() (use preg_replace() instead) eregi() (use preg_match() with the 'i' modifier instead) eregi_replace() (use preg_replace() with the 'i' modifier instead) set_magic_quotes_runtime() and its alias, magic_quotes_runtime() session_register() (use the $_SESSION superglobal instead) session_unregister() (use the $_SESSION superglobal instead) session_is_registered() (use the $_SESSION superglobal instead) set_socket_blocking() (use stream_set_blocking() instead) split() (use preg_split() instead) spliti() (use preg_split() with the 'i' modifier instead) sql_regcase() mysql_db_query() (use mysql_select_db() and mysql_query() instead) mysql_escape_string() (use mysql_real_escape_string() instead) Passing locale category names as strings is now deprecated. Use the LC_* family of constants instead. The is_dst parameter to mktime(). Use the new timezone handling functions instead. Deprecated features: Assigning the return value of new by reference is now deprecated. Call-time pass-by-reference is now deprecated. The use of {} to access string offsets is deprecated. Use [] instead.
          Hide
          Andrew Davis added a comment -

          Its not committed yet (its Tuesday/review day) but I've finished replacing all the ereg's and eregi's. I've also made a pass over that list of deprecated functions. Below is a list of those I haven't yet replaced:

          http://php.net/manual/en/function.split.php a quick search shows ~1100 instances although there's a lot of false positives in there from things like preg_split and Javascript split. Replace with explode or preg_split. I had a quick scan through and I think I should be able to replace more or less all of these with explode using a few find and replaces. I want to get my existing changes committed before I do that.

          http://php.net/manual/en/function.set-magic-quotes-runtime.php It's already being done like this in a few places: if (PHP_VERSION < 6)

          { set_magic_quotes_runtime($magic_quotes); }

          http://php.net/manual/en/function.mysql-escape-string.php Replace mysql_escape_string with mysql_real_escape_string

          I haven't yet figured out what is involved in removing these from the code.
          http://php.net/manual/en/function.define-syslog-variables.php
          http://au2.php.net/manual/en/function.dl.php
          http://php.net/manual/en/function.session-unregister.php

          Show
          Andrew Davis added a comment - Its not committed yet (its Tuesday/review day) but I've finished replacing all the ereg's and eregi's. I've also made a pass over that list of deprecated functions. Below is a list of those I haven't yet replaced: http://php.net/manual/en/function.split.php a quick search shows ~1100 instances although there's a lot of false positives in there from things like preg_split and Javascript split. Replace with explode or preg_split. I had a quick scan through and I think I should be able to replace more or less all of these with explode using a few find and replaces. I want to get my existing changes committed before I do that. http://php.net/manual/en/function.set-magic-quotes-runtime.php It's already being done like this in a few places: if (PHP_VERSION < 6) { set_magic_quotes_runtime($magic_quotes); } http://php.net/manual/en/function.mysql-escape-string.php Replace mysql_escape_string with mysql_real_escape_string I haven't yet figured out what is involved in removing these from the code. http://php.net/manual/en/function.define-syslog-variables.php http://au2.php.net/manual/en/function.dl.php http://php.net/manual/en/function.session-unregister.php
          Hide
          Tim Hunt added a comment -

          I'm not sure we should be replacing these in third-party libraries like typo3 or htmlpurifier.

          Show
          Tim Hunt added a comment - I'm not sure we should be replacing these in third-party libraries like typo3 or htmlpurifier.
          Hide
          Anthony Borrow added a comment -

          Tim - I can appreciate the concern of not wanting to make too many changes to third party libraries but I think I would still advocate for the changes in Moodle since our code should be as issue free as possible. Perhaps we can with the developers of those libraries to ensure that they update the libraries to reflect the changes. I think it is just a matter of following best practices (i.e. not using deprecated functions). On the other hand, since they are just deprecated you could argue that functionality is still there and the site admin should not be running a production site in debug mode but it would drive me crazy if we had to start getting used to seeing deprecation notices every time we working on code because a deprecated function was used in a popular third-party library. What we may want to do is generate a separate patch for each library so that if we need to update the library and that library has not been brought up to 5.3 standards then we can patch it without too much effort (and for good measure checking to see that no new usages of a deprecated function were introduced). Peace - Anthony

          Show
          Anthony Borrow added a comment - Tim - I can appreciate the concern of not wanting to make too many changes to third party libraries but I think I would still advocate for the changes in Moodle since our code should be as issue free as possible. Perhaps we can with the developers of those libraries to ensure that they update the libraries to reflect the changes. I think it is just a matter of following best practices (i.e. not using deprecated functions). On the other hand, since they are just deprecated you could argue that functionality is still there and the site admin should not be running a production site in debug mode but it would drive me crazy if we had to start getting used to seeing deprecation notices every time we working on code because a deprecated function was used in a popular third-party library. What we may want to do is generate a separate patch for each library so that if we need to update the library and that library has not been brought up to 5.3 standards then we can patch it without too much effort (and for good measure checking to see that no new usages of a deprecated function were introduced). Peace - Anthony
          Hide
          Tim Hunt added a comment -

          Yes, we should work with upstream.

          And, when we must, unavoidably make local changes to third-party files, these have to carefully documented according to a procedure Petr worked out, but which I can't remember the details of.

          But, locally hacking third party libraries is basically a disaster waiting to happen when we update those libraries.

          Show
          Tim Hunt added a comment - Yes, we should work with upstream. And, when we must, unavoidably make local changes to third-party files, these have to carefully documented according to a procedure Petr worked out, but which I can't remember the details of. But, locally hacking third party libraries is basically a disaster waiting to happen when we update those libraries.
          Hide
          Andrew Davis added a comment -

          I think I'm going to add new issues for these other deprecated functions to break this into more manageable chunks.

          I'm not sure how to approach deprecated functions in third party libraries. I've just been blindly ploughing through all references which, as you say Tim, will be a problem if we update those libraries and the library authors haven't gone through a similar process of removing deprecated functions.

          Show
          Andrew Davis added a comment - I think I'm going to add new issues for these other deprecated functions to break this into more manageable chunks. I'm not sure how to approach deprecated functions in third party libraries. I've just been blindly ploughing through all references which, as you say Tim, will be a problem if we update those libraries and the library authors haven't gone through a similar process of removing deprecated functions.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi just was looking to the patch and found some things that I don't know if will work, mainly all those [:cntrl:] and friend classes. Haven't tested if but does PCRE support them? I always have thought the answer was no, but perhaps I'm wrong (in fact I've been using POSIX here and there exactly because I needed to identify easily those control chars).

          Just a comment, as said, not verified... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi just was looking to the patch and found some things that I don't know if will work, mainly all those [:cntrl:] and friend classes. Haven't tested if but does PCRE support them? I always have thought the answer was no, but perhaps I'm wrong (in fact I've been using POSIX here and there exactly because I needed to identify easily those control chars). Just a comment, as said, not verified... ciao
          Hide
          Andrew Davis added a comment -

          I also initially thought they weren't supported but they are actually are. There is one restriction. Here's a php warning I got.

          preg_match() [function.preg-match]: Compilation failed: POSIX named classes are supported only within a class

          This works
          $present = preg_match('/[[:cntrl:]]/',$orig);
          This doesn't
          $present = preg_match('/[:cntrl:]/',$orig);

          As long as the named class is in a set ie [ ] they work fine.

          Show
          Andrew Davis added a comment - I also initially thought they weren't supported but they are actually are. There is one restriction. Here's a php warning I got. preg_match() [function.preg-match] : Compilation failed: POSIX named classes are supported only within a class This works $present = preg_match('/[ [:cntrl:] ]/',$orig); This doesn't $present = preg_match('/ [:cntrl:] /',$orig); As long as the named class is in a set ie [ ] they work fine.
          Hide
          Anthony Borrow added a comment -

          Thanks Andy for being thorough, or to quote the Big Lebowski "very thorough". Breaking the task into separate issues was a good idea. Keep up the good work. Peace - Anthony

          Show
          Anthony Borrow added a comment - Thanks Andy for being thorough, or to quote the Big Lebowski "very thorough". Breaking the task into separate issues was a good idea. Keep up the good work. Peace - Anthony
          Hide
          Andrew Davis added a comment -

          Ok, all references to ereg, eregi, ereg_replace and eregi_replace should be gone from both trunk and 19.

          Regarding 3rd party libs, where one was modified its directory will contain a file called moodle_readme.txt or similar. This file contains comments about modifications we have made. Some had already had modifications so I've added more stuff to the bottom of existing files or created new ones as required.

          In the moodle read me file I've added a comment and a patch for that particular library. If/when we integrate a new version of that lib the patch should make it easy to reapply any changes we need. That said, hopefully when we come to integrate new versions of these libs they will have already removed these deprecated functions in which case we can just delete these additions to the read me file.

          I've attached 2 new patches to this issue:
          The trunk patch (MDL20821trunkpatch2.txt) is the second half of my modifications.
          The 1.9 patch (091118_MDL20821_19patch.txt) is all of my modifications to the 1.9 branch. Disregard the first 1.9 patch.

          Apologies for the inconsistent patch naming.

          Show
          Andrew Davis added a comment - Ok, all references to ereg, eregi, ereg_replace and eregi_replace should be gone from both trunk and 19. Regarding 3rd party libs, where one was modified its directory will contain a file called moodle_readme.txt or similar. This file contains comments about modifications we have made. Some had already had modifications so I've added more stuff to the bottom of existing files or created new ones as required. In the moodle read me file I've added a comment and a patch for that particular library. If/when we integrate a new version of that lib the patch should make it easy to reapply any changes we need. That said, hopefully when we come to integrate new versions of these libs they will have already removed these deprecated functions in which case we can just delete these additions to the read me file. I've attached 2 new patches to this issue: The trunk patch (MDL20821trunkpatch2.txt) is the second half of my modifications. The 1.9 patch (091118_MDL20821_19patch.txt) is all of my modifications to the 1.9 branch. Disregard the first 1.9 patch. Apologies for the inconsistent patch naming.
          Hide
          Tim Hunt added a comment -

          Thanks Andrew. Nice work.

          Show
          Tim Hunt added a comment - Thanks Andrew. Nice work.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Agree with Tim, Andrew.. 100%

          Anyway I must confess I'm a bit worried about all this stuff landing into 19_STABLE release right now (days before security release). These exact points need clarification:

          • Has been tested under PHP 4.x ?
          • Knowing regular expressions a bit... have all them been tested after change?
          • I cannot agree with the approach followed to update 3rd part libraries. We should look their upstreams before anything else.
          • Has anybody reviewed the patch before committing it?

          So, without knowing the response for these points... my recommendation is to revert it completely. We cannot go that route at this moment IMO.

          Please note this is nothing against we needing to take rid of the "old" expressions nor against the quality of Andrew's work, not at all!

          But this cannot be done this way (at this moment), hence, my +1 to revert.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Agree with Tim, Andrew.. 100% Anyway I must confess I'm a bit worried about all this stuff landing into 19_STABLE release right now (days before security release). These exact points need clarification: Has been tested under PHP 4.x ? Knowing regular expressions a bit... have all them been tested after change? I cannot agree with the approach followed to update 3rd part libraries. We should look their upstreams before anything else. Has anybody reviewed the patch before committing it? So, without knowing the response for these points... my recommendation is to revert it completely. We cannot go that route at this moment IMO. Please note this is nothing against we needing to take rid of the "old" expressions nor against the quality of Andrew's work, not at all! But this cannot be done this way (at this moment), hence, my +1 to revert. Ciao
          Hide
          Petr Škoda added a comment -

          +1 from me too

          Show
          Petr Škoda added a comment - +1 from me too
          Hide
          Andrew Davis added a comment - - edited

          At this time I don't have enough knowledge of Moodle to have a strong opinion on this either way. I'll bring this to Martin's attention when he gets in.

          One possibility is reverting 1.9 and leaving 2 as is. The changes can be reintroduced into 1.9 at a better time (if such a thing exists). As I understand the schedule we have more time for 2 to look upstream, bring in updated libs (thus overwriting any changes to libs I made) and give them proper testing. The idea of updating 3rd party libs in 1.9 makes me far more nervous than deploying these changes.

          Show
          Andrew Davis added a comment - - edited At this time I don't have enough knowledge of Moodle to have a strong opinion on this either way. I'll bring this to Martin's attention when he gets in. One possibility is reverting 1.9 and leaving 2 as is. The changes can be reintroduced into 1.9 at a better time (if such a thing exists). As I understand the schedule we have more time for 2 to look upstream, bring in updated libs (thus overwriting any changes to libs I made) and give them proper testing. The idea of updating 3rd party libs in 1.9 makes me far more nervous than deploying these changes.
          Hide
          Petr Škoda added a comment -

          found regression in 1.9.x - reverting everything committed there!

          Show
          Petr Škoda added a comment - found regression in 1.9.x - reverting everything committed there!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I really think it's better to revert 19_STABLE completely... and then reintroduce changes progressively by testing each one and so on, paying special interest to PHP 4.3 ...?

          np with HEAD, but stable... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I really think it's better to revert 19_STABLE completely... and then reintroduce changes progressively by testing each one and so on, paying special interest to PHP 4.3 ...? np with HEAD, but stable... ciao
          Hide
          David Mudrak added a comment -

          +1 for revert from STABLE. It is too much mature now to risk possible regressions. I can't see any reason why we ever should replace these deprecated functions on STABLE. Yes, they can produce deprecation warnings on PHP 5.3 servers. But once we have it fixed in HEAD, I would not backport this at all.

          Show
          David Mudrak added a comment - +1 for revert from STABLE. It is too much mature now to risk possible regressions. I can't see any reason why we ever should replace these deprecated functions on STABLE. Yes, they can produce deprecation warnings on PHP 5.3 servers. But once we have it fixed in HEAD, I would not backport this at all.
          Hide
          Dan Marsden added a comment -

          hmmm - while I understand the reasons for not putting this in stable now, I don't think we should ignore this in the 1.9 branch - 1.9 is going to be around for a long time. As these are individually tested in HEAD, we should backport the fix.

          Show
          Dan Marsden added a comment - hmmm - while I understand the reasons for not putting this in stable now, I don't think we should ignore this in the 1.9 branch - 1.9 is going to be around for a long time. As these are individually tested in HEAD, we should backport the fix.
          Hide
          Andrew Davis added a comment -

          "found regression in 1.9.x - reverting everything committed there!"

          So be it. Once the changes have been bedded down in 2 we can backport them to 1.9 if necessary.

          Show
          Andrew Davis added a comment - "found regression in 1.9.x - reverting everything committed there!" So be it. Once the changes have been bedded down in 2 we can backport them to 1.9 if necessary.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yeah, Dan, Andrew. I think 1.9.x as target for this continues 100%.

          It's just the moment was really risky (too many untested changes for an imminent release).

          I think we can do it progressively (looking we don't break anything and checking it works under PHP 4.x - as far as Moodle 1.9 must work there).

          Ciao

          PS: Andrew, don't worry about reverts, it's Moodle's nature and Petr's and Eloy's headbang! No worries at all! LOL!

          Show
          Eloy Lafuente (stronk7) added a comment - Yeah, Dan, Andrew. I think 1.9.x as target for this continues 100%. It's just the moment was really risky (too many untested changes for an imminent release). I think we can do it progressively (looking we don't break anything and checking it works under PHP 4.x - as far as Moodle 1.9 must work there). Ciao PS: Andrew, don't worry about reverts, it's Moodle's nature and Petr's and Eloy's headbang! No worries at all! LOL!
          Hide
          Petr Škoda added a comment -

          reoepning, we have discovered major difference in ereg/preg - the [:anum:] in ereg was accepting only a-z, but in preg it accepts also utf-8 letters such as ?šýž???žýáš

          we need to review all code and test all similar things, I suppose we need fully battery of unit tests for this, because it is not officially documented anywhere and implementation may differ – this could cause some security issues too.

          Show
          Petr Škoda added a comment - reoepning, we have discovered major difference in ereg/preg - the [:anum:] in ereg was accepting only a-z, but in preg it accepts also utf-8 letters such as ?šýž???žýᚠwe need to review all code and test all similar things, I suppose we need fully battery of unit tests for this, because it is not officially documented anywhere and implementation may differ – this could cause some security issues too.
          Hide
          Andrew Davis added a comment -

          Petr, I presume you mean [:alnum:] ie alphanumeric. I'm not able to demonstrate a difference between how preg and ereg that that symbol. I've attached a php file (regextest.php) that demonstrates that both ereg and preg appear to treat it the same. Am I missing something?

          I count 21 instances of [:alnum:] in Moodle 2.0 not counting those within third party libraries.

          /mod/wiki/diff/diff_nwiki.php 1 instance in _split($lines). Doesnt appear to be covered by any unit tests.

          /question/format/webct/format.php 4 instances all in qformat_webct_convert_formula($formula). Doesnt seem to be covered by any unit tests.

          /repository/url/lib.php (1 instance) in analyse_page($baseurl, $content, &$list) and the regex pattern doesn't even seem to be being used at all.

          /lib/wiki_to_markdown.php (16 instances)

          Probably the absolute best thing to do would be for the people responsible for each of those files/areas to write unit tests exercising the regexs to define what the correct behaviour is in that context. I can write the tests if need be but it would be handy for someone who knows what "correct" is to define it. Then the regexs can then be modified if necessary to conform to the defined correct behaviour.

          Show
          Andrew Davis added a comment - Petr, I presume you mean [:alnum:] ie alphanumeric. I'm not able to demonstrate a difference between how preg and ereg that that symbol. I've attached a php file (regextest.php) that demonstrates that both ereg and preg appear to treat it the same. Am I missing something? I count 21 instances of [:alnum:] in Moodle 2.0 not counting those within third party libraries. /mod/wiki/diff/diff_nwiki.php 1 instance in _split($lines). Doesnt appear to be covered by any unit tests. /question/format/webct/format.php 4 instances all in qformat_webct_convert_formula($formula). Doesnt seem to be covered by any unit tests. /repository/url/lib.php (1 instance) in analyse_page($baseurl, $content, &$list) and the regex pattern doesn't even seem to be being used at all. /lib/wiki_to_markdown.php (16 instances) Probably the absolute best thing to do would be for the people responsible for each of those files/areas to write unit tests exercising the regexs to define what the correct behaviour is in that context. I can write the tests if need be but it would be handy for someone who knows what "correct" is to define it. Then the regexs can then be modified if necessary to conform to the defined correct behaviour.
          Hide
          Andrew Davis added a comment -

          Note that all the ereg and eregi function in Moodle 2.0 were removed in November 2009. Just need to decide if:

          1) any more work needs to be done in 2.0 (unit tests to test regexs)

          2) whether we're happy to leave this a 2.0 only fix. the ereg and eregi functions will not be in php 6. Is that a problem we care about for the 1.9 branch?

          Show
          Andrew Davis added a comment - Note that all the ereg and eregi function in Moodle 2.0 were removed in November 2009. Just need to decide if: 1) any more work needs to be done in 2.0 (unit tests to test regexs) 2) whether we're happy to leave this a 2.0 only fix. the ereg and eregi functions will not be in php 6. Is that a problem we care about for the 1.9 branch?
          Hide
          Petr Škoda added a comment -

          please trust me, remove all anums because the behaviour is not guaranteed to be constant in all installations

          Show
          Petr Škoda added a comment - please trust me, remove all anums because the behaviour is not guaranteed to be constant in all installations
          Hide
          Petr Škoda added a comment -

          aaaah, the alnum is used in noncritical places only now, it was removed from parameter cleaning

          Show
          Petr Škoda added a comment - aaaah, the alnum is used in noncritical places only now, it was removed from parameter cleaning
          Hide
          Andrew Davis added a comment -

          Ok, marking this resolved. After discussion in the weekly developer meeting it was decided that php 6 support in the 1.9 branch isn't a priority.

          As Petr says the remaining instances of :alnum: are not in security related areas.

          Show
          Andrew Davis added a comment - Ok, marking this resolved. After discussion in the weekly developer meeting it was decided that php 6 support in the 1.9 branch isn't a priority. As Petr says the remaining instances of :alnum: are not in security related areas.
          Hide
          Petr Škoda added a comment - - edited

          tested on Windows with latest PHP 5.3, Czech locale selected:
          the [:alnum:] and [:alpha:] do match šš???š? as alphanumeric characters, so all uses of :alnum: where developer expects only a-z as absolutely incorrect

          Please note that the /u is mandatory on some installations if you want to match UTF8 chars, on others it is on by default...

          Show
          Petr Škoda added a comment - - edited tested on Windows with latest PHP 5.3, Czech locale selected: the [:alnum:] and [:alpha:] do match šš???š? as alphanumeric characters, so all uses of :alnum: where developer expects only a-z as absolutely incorrect Please note that the /u is mandatory on some installations if you want to match UTF8 chars, on others it is on by default...
          Hide
          Petr Škoda added a comment -

          grrr, this thing is eating my utf8 chars and shows ??? instead only

          Show
          Petr Škoda added a comment - grrr, this thing is eating my utf8 chars and shows ??? instead only
          Hide
          David Mudrak added a comment -

          And have you tested if [a-z] matches the Czech characters, too?

          Show
          David Mudrak added a comment - And have you tested if [a-z] matches the Czech characters, too?
          Hide
          Anthony Borrow added a comment -

          Just an FYI that I posted http://moodle.org/mod/forum/discuss.php?d=153905 to encourage all folks with contrib code to review their code for any of the PHP 5.3 deprecated functions. Peace - Anthony

          Show
          Anthony Borrow added a comment - Just an FYI that I posted http://moodle.org/mod/forum/discuss.php?d=153905 to encourage all folks with contrib code to review their code for any of the PHP 5.3 deprecated functions. Peace - Anthony

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: