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.
Thanks.
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);
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
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.
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.