Moodle 401 fixes #90
Moodle 401 fixes #90
Conversation
Proxy settings were being set with set_config() then read from $CFG. If a proxy is already set in the development environment's config.php, this will pick up the config.php settings rather than the test ones.
| * @param array $args | ||
| * @return mixed|void | ||
| */ | ||
| public function __call(string $method, array $args) { |
There was a problem hiding this comment.
I also think this is great. I think the method description should make it clearer it should only be used for polyfills, and the comment about which method it's polyfill-ing be moved next to the call itself.
This can then be potentially made into an array of [oldmethod -> newmethod] mappings and looped over.
If kept, I would like to see a comment line clearly saying something like, Remove if no longer supporting Moodle X.XX, and once the list of things is empty, we can delete the whole function.
What are your thoughts on making this more standard practice? It is definitely "better" than having an if/else block wherever backwards compatibility is called, since now there should be a single place to check for them, and with the above suggestions it should make it easier to maintain should the list need to be changed.
There was a problem hiding this comment.
The only other caveat to making it standard is some methods aren't 100% identical with what they are changed to, and might need more/less parameters and maybe a different order too. But it can always be implemented as a custom if block inside the __call method if required.
|
@marxjohnson there is a conflict in fixing assertMatchesRegularExpression. I actually like your (more elegant IMHO) way of dealing with that |
| @@ -22,6 +22,8 @@ | |||
| * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later | |||
| */ | |||
|
|
|||
| namespace search_elastic\enrich\image; | |||
There was a problem hiding this comment.
This namespace doesn't feel right, everything else is in the top level namespace
There was a problem hiding this comment.
It does look ok based on coding styles:
When using namespaces, the namespace of the test class should match the namespace of the code under test.
|
Thanks @marxjohnson for you work. I merged the rebased code as part of #93 as needed it today. Closing this one... |
Fixes for #89