Use new inputs from UI-framework in Study Programme #1189
Conversation
| ->withValue($prg->getDescription()) | ||
| ], | ||
| $txt("prg_edit"), | ||
| "" |
There was a problem hiding this comment.
One thing I do not like is, that the title and description go after the content when constructing a section.
| // to the section they originated from. | ||
| return call_user_func_array("array_merge", $values); | ||
| })); | ||
| } |
There was a problem hiding this comment.
I do not really like that much nesting in terms of shuffling into arrays each other. However if you look at this, note, that you might also generate your inputs like so:
$input_a = $ff->numeric($txt("prg_points"))->...
$input_b = $ff->select($txt("prg_status"), ....
an then pack them subsequentially:
$ff->section([$input_a ,$input_b],$txt("prg_assessment"),"")
and so on. I think there is no good or bad way here, it's just a matter of personal taste.
There was a problem hiding this comment.
Yes, I partially agree. In the style I used, without temporary variables, one directly finds the structure of the form as it is shown on the GUI without following references. In general I like that, but this might also get out of hand at some point. ATM I think I would start to use methods for the single sections once the form gets bigger then, say, one page one my screen. I would use methods (and not temporary variables) because this would allow me to have a reading flow from general-form-layout to specific sections and inputs, while I would need to define the specifics (sections, inputs, etc.) before the general structure if I use variables.
|
Nice work, good thinking to make a PR out of this. |
| $form->addCommandButton("cancel", $this->lng->txt("cancel")); | ||
|
|
||
| return $form; | ||
| $prg->setPoints($data[self::PROP_POINTS]); |
There was a problem hiding this comment.
What about input filtering (at least with known mechanisms like ilUtil::stripSlashes() implicitly done in legacy form handling)? Currently the raw HTTP post data (checked with constraints) is persisted in database by calling $prg->update();. I don't see any sanitizing step here. When merging this, this process will be (probably more) vulnerable against XSS (as with legacy forms).
There was a problem hiding this comment.
Although $data might look superficially similar to raw HTTP-post data, it in fact is already processed and "filtered". The core of this functionality is here. The values supplied via post are checked with isClientSideValueOk first. This is is_numeric for the Numeric Field. If this does not work, an exception is thrown. The values are then checked and transformed via additional validations and transformations, where Numeric Field again checks for is_numeric via a constraint (which is already to much, because the value certainly is numeric already) Only if this worked for every field in the form, the user gets the checked and transformed data from Form::getData. There might be bugs in this mechanism (as always) and there are fields (I'm looking at you, Select Field) that are missing necessary checks, but this is in now way raw data from HTTP-Post. Also I cannot see why we would want to treat every input with ilUtil::stripSlashes if there are simpler means (like checking is_numeric for the Numeric Field, or checking against available options for Select Field) to discard malicious input. We might need to discuss how to treat general text-input in that regard, though.
There was a problem hiding this comment.
Maybe my comment points to the wrong line of code. It is not about the numeric fields (here: points). It's about the title and description. I could not see any constraints bound to these fields. And of course something like ilUtil::stripSlashes() MUST NOT be part of the Input service, but I expected something like a MustNotContainInvalidChars constraint or some kind of transformation (making use of \ilUtil::stripSlashes(), or the Input filter service in future) for the text fields already shipped with this initial/first form migration.
|
I like this! I will maybe migrate some of my simple forms (those without esoteric form elements, drop zone based uploads, ...) for 5.4.x if there will be some input filtering available. The |
|
Hi @mjansenDatabay, I tried to outline the general mechanism of input checking for the UI-framework as an inline comment above. You may also attach more transformations and validations (like here to your form fields or sections to further strengthen defenses in your use cases. The set of validations and transformations we currently ship in the trunk surely needs to be enhanced and we will need to have a more detailed lock on the default validations and transformations for certain fields, but the mechanism to filter inputs from forms definitely exist. What would you expect from an Input Filter Service on top of this for the form use case? |
|
Hi @mjansenDatabay, I fixed the Then I had a long long look into
Thus So, as far as I understand that stuff, the usage of Best regards! |
|
@klees You are right, we have the |
This shows how a switch from
Service\Formto the new inputs from the UI-framework might look like. Although I'm the maintainer of the SP (and thus may just merge this) I decided to open a PR to give others the possibility to scrutinize the changes, since this basically is the first time the new input-API is used in a real scenario. Thus I'm very interested in the stuff you like or dislike in the new code-wise appearance of forms and inputs and also in the questions you might have.