Hello,
for a long time I was pulling my hairs when dealing with array parameters in optional_param(), required_param() and clean_param(). The problem is that at lower levels you sometimes get random arrays in an unexpected places which usually end in fatal errors.
I have tried to change the API a bit, it would accept only non-array types in optional_param(), require_param() and clean_param(). The array parameters would be handled by new optional_param_array(), required_param_array() and clean_params_array().
You can see the code in MDL-26796, please comment here or in the tracker issue, thanks for any feedback!
Petr
I think that this is a definite improvement to the API.
Whatever short-term pain we have to go thorugh to fix our code (and I think the pain will be minimal) will be worth it for making our API sane.
Thanks for tackling this Petr.
I agree this is a good change. But also whilst making changes in this aread it seems like a good place to add unit tests to (assuming that is easy to 'spoof' $_POST/$_GET data)?
Ah, right, I agree now is the right time to add the missing unit tests.
Thanks.
Thanks.
It makes more sense... you never really knew what was going to happen with array parameters before.
I do agree that this new API is nice because it is explicit and may remove some confusion or unwanted behavior, but I have the following concerns:
- clean_param throws an exception now, which obviously completely breaks legacy code. We have a lot of our own code plus third party code and it will take a long time to comb through it all and make and test the appropriate changes (maybe also waiting on changes from a 3rd party). Plus, if we miss anything, it's very harsh on our end users. Can we please make this backwards compatible?
- Is there a reason for not having a recursion param on required/optional_param_array? Wouldn't we have to break coding standards to use recursion then? EG: clean_param_array($_REQUEST['foo'], PARAM_INT, true);
Cheers and thanks,
Mark
1/ All these param functions throw exceptions only if developer did something wrong or when user tries to hacks the parameters. Please note that since 2.0 there were some debugging messages warning devs to supply proper type parameters.
2/ Yes, the reason is you would be probably using forms for complex stuff or some ajax script with JS UI.
2/ Yes, the reason is you would be probably using forms for complex stuff or some ajax script with JS UI.
For 1, I was specifically referring to this exception vs this backwards compatible debug message. I was hoping that we could change the exception one to a debug message with backwards compatibility.
For 2, I'm sorry, but I don't understand your response. I don't see having an input element with name="foo[bar][1]" as being advanced and it seems reasonable that the required/optional array cleaning params should handle it. If there is a workaround, I'd be happy to see it.
Cheers,
Mark
1. Ah, that one, sorry. I went through all the uses we have at present in the core - luckily there were very few of these. Hopefully there are not many of these. In any case developers read the mod/upgrade.txt and test all they custom stuff for each new major release, right?
2. Custom form processing is discouraged since Moodle 1.7, you do not need food[bar][1] when you are using recommended coding style (that is formslib), right? Theoretically we could add one more parameter but I do not think we should encourage this coding style.
If you do not agree let's talk about your existing code, maybe it would be easier to change coding style there. I made sure the proposed changes are suitable for everything in standard Moodle distribution.
Thanks a lot for the feedback!
2. Custom form processing is discouraged since Moodle 1.7, you do not need food[bar][1] when you are using recommended coding style (that is formslib), right? Theoretically we could add one more parameter but I do not think we should encourage this coding style.
If you do not agree let's talk about your existing code, maybe it would be easier to change coding style there. I made sure the proposed changes are suitable for everything in standard Moodle distribution.
Thanks a lot for the feedback!
Petr,
1. Invalid assumption or maybe more wishful thinking. This is actualy the first time in 4+ years of working on Moodle I have ever heard of mod/upgrade.txt and I have never read it. I do read docs.moodle.org for changes in the development docs and apis.
2. Custom forms processing comes up a fair amount in our line of work where we are creating integrations with 3rd party applications and vendors. We have written new settings types for almost every version of Moodle to support 3rd party development.
Regardless of 1 or 2 I have to agree with Mark please reconsider changing the warning about deprecation to a thrown exception. This will create a significant amount of work for the Moodle partners and community developers to upgrade Moodle 1.9 code to Moodle 2.0. We have hundreds of blocks, activities, question types, reports and other plugins that need to be upgraded to Moodle 2.0. Throwing an exception in any one of these plugins because we or a community developer missed one clean_param call when upgrading the code causes damage to our company's reputation. Where as a warning this code is deprecated can be ignored by a client because the plugin will still function.
1. It is easier to keep the description of significant changes closer to the code, hopefully all plugins will have these instructions for upgrades in the future. The problem with docs is that they get easily out-of-sync. That file is supposed to be a sort of checklist with links to docs pages with the actual description of changes.
2. This in not about upgrade from 1.9->2.0 (which is huge I agree), but 2.1->2.2 because upgrade from older versions will not be probably supported. Each version brings incompatible changes and requires update of code, I personally prefer exceptions that force devs to upgrade code instead of developer debug messages that are being often ignored.
If you value your reputation please test all your plugins in each new major version, there is no warranty it will work. The 2.1 release was a bit special (few changes) because it was under active development for a short time only.
Please do not forget this is also a potential security issue, until we remove the debugging messages we did not solve this problem. This is the reason why I would vote for removing of the array debugging with fallbacks in optional_param() and required_param() before the Moodle 2.2 release, in any case debugging should be removed from 2.3.
2. This in not about upgrade from 1.9->2.0 (which is huge I agree), but 2.1->2.2 because upgrade from older versions will not be probably supported. Each version brings incompatible changes and requires update of code, I personally prefer exceptions that force devs to upgrade code instead of developer debug messages that are being often ignored.
If you value your reputation please test all your plugins in each new major version, there is no warranty it will work. The 2.1 release was a bit special (few changes) because it was under active development for a short time only.
Please do not forget this is also a potential security issue, until we remove the debugging messages we did not solve this problem. This is the reason why I would vote for removing of the array debugging with fallbacks in optional_param() and required_param() before the Moodle 2.2 release, in any case debugging should be removed from 2.3.