|
[
Permalink
| « Hide
]
Michael Blake added a comment - 06/Jan/09 01:54 PM
Dongsheng, please review this patch.
Verified, and patched, thanks Jason.
I have no idea what this setting is, and the help text does not explain it to me.
Having looked at the code, I can now see, and 50 seems like a stupidly large default. Surely the default should be 0, meaning no limit. (Since this is now in the admin tree, the existing isset logic in course/lib.php is no longer any good.) Also, if I was doing this, I would probably have fixed the name to be maxcategorydepth, so it followed the coding guidelines, even though that would have been a bigger change. Rant over. Jason, do you want to come up with an improved patch? A new patch fixed the bugs Tim found out.
isset($CFG->maxcategorydepth) && ($CFG->maxcategorydepth != 0)
would be better as !empty($CFG->maxcategorydepth) And sitemaxcategorydepthhelp - Oh, just noticed, all the other strings like this are called something like configsitemaxcategorydepth, anyway - I think This specifies the maximum depth of nested categories shown when courses are listed on teh site front page. Also, in the admin setting, the '0' option needs to be displayed as 'Unlimited' instead of 0. Thanks Tim.
I attached a new patch here. committed, please review, thanks
This is just my $0.02 on this: once a naming convention is suggested, it is probably a good idea to stick to it, as the change from the first patch to the second actually creates a regression for anyone who has implemented the first patch.
Note the underscores and lack thereof:
Thanks! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||