Uploaded image for project: '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
    • Status: Closed
    • Priority: 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

      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

        Gliffy Diagrams

        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
            aborrow 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
            aborrow 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
            timhunt 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
            timhunt 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
            aborrow 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
            aborrow 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
            dougiamas Martin Dougiamas added a comment -

            All yours Andrew!

            Show
            dougiamas Martin Dougiamas added a comment - All yours Andrew!
            Hide
            andyjdavis 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
            andyjdavis 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
            andyjdavis 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
            andyjdavis 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
            aborrow 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
            aborrow 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
            andyjdavis 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
            andyjdavis 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
            timhunt Tim Hunt added a comment -

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

            Show
            timhunt Tim Hunt added a comment - I'm not sure we should be replacing these in third-party libraries like typo3 or htmlpurifier.
            Hide
            aborrow 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
            aborrow 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
            timhunt 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
            timhunt 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
            andyjdavis 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
            andyjdavis 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
            stronk7 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
            stronk7 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
            andyjdavis 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
            andyjdavis 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
            aborrow 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
            aborrow 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
            andyjdavis 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
            andyjdavis 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
            timhunt Tim Hunt added a comment -

            Thanks Andrew. Nice work.

            Show
            timhunt Tim Hunt added a comment - Thanks Andrew. Nice work.
            Hide
            stronk7 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
            stronk7 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
            skodak Petr Skoda added a comment -

            +1 from me too

            Show
            skodak Petr Skoda added a comment - +1 from me too
            Hide
            andyjdavis 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
            andyjdavis 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
            skodak Petr Skoda added a comment -

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

            Show
            skodak Petr Skoda added a comment - found regression in 1.9.x - reverting everything committed there!
            Hide
            stronk7 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
            stronk7 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
            mudrd8mz 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
            mudrd8mz 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
            danmarsden 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
            danmarsden 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
            andyjdavis 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
            andyjdavis 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
            stronk7 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
            stronk7 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            andyjdavis 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
            andyjdavis 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
            andyjdavis 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
            andyjdavis 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
            skodak Petr Skoda added a comment -

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

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

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

            Show
            skodak Petr Skoda added a comment - aaaah, the alnum is used in noncritical places only now, it was removed from parameter cleaning
            Hide
            andyjdavis 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
            andyjdavis 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            skodak Petr Skoda added a comment -

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

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

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

            Show
            mudrd8mz David Mudrak added a comment - And have you tested if [a-z] matches the Czech characters, too?
            Hide
            aborrow 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
            aborrow 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:
                  Fix Release Date:
                  24/Nov/10