Moodle

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

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.

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

All yours Andrew!

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.

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.

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.

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

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

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

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.

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.

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

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.

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

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.

Thanks Andrew. Nice work.

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

+1 from me too

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.

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

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

+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.

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.

"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.

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!

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.

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.

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?

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

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

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.

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...

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

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

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