-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tests/webserver/configphp.php tests for $CFG being rewritten but... #125
Comments
hi @Arantor, no this test is critical. The default config.php must be changed or the clean urls plugin will not work correctly, please follow the steps here https://github.com/brendanheywood/moodle-local_cleanurls#installation In particular this step: https://github.com/brendanheywood/moodle-local_cleanurls#step-4-turn-it-on-and-configure Also please note that this plugin is fairly experimental |
Ok, I didn't see that part of the setup. Though I haven't seen any obvious problems on my test setup when using the default configuration, but my testing has been far from extensive. It's for a hobby project, so I can change the configuration without any headaches anyway. Just struck me as really weird. I'm fine with experimental, have seen this kind of thing done in other environments before through manipulation of output buffers, and this is much cleaner. If I do find anything else, I'll try to post it, but as I said it's for a hobby project, so time is somewhat limited. I did speak to a few people here at Moodlemoot UK but no one seems to be overly concerned about the URL structure. I'd have loved to get someone onboard and getting some funding to do some testing and bug finding/fixing, as I think a joint Catalyst AU/EU project would be cool (still in awe of the stuff done thus far!) |
So there's a test for $CFG being 'lost' while testing that the correct value is inserted in $CFG.
On my fresh 3.4 installation, the config.php file is generated but the first thing it does is unset $CFG before creating a new $CFG instance from scratch.
Might be worth cutting that aspect of the test?
The text was updated successfully, but these errors were encountered: