? Success

User tests: Successful: Unsuccessful:

avatar Fedik
Fedik
2 Sep 2015

this is fix for #7797
caused by changes in #7381

The main purpose of #7381 was make sure that the client has sent all fields that in form.xml.
The installation use hidden field "db_type" in summary.xml form for determine which "sample data" can be installed ... but do not render this field, this is cause "db_type" value reset on the last step.

This pull fix it

test
make sure that installation work correct

avatar Fedik Fedik - open - 2 Sep 2015
avatar Fedik Fedik - change - 2 Sep 2015
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 2 Sep 2015
Labels Added: ?
avatar Kubik-Rubik Kubik-Rubik - change - 2 Sep 2015
Milestone Added:
avatar Kubik-Rubik
Kubik-Rubik - comment - 2 Sep 2015

Thank you @Fedik! I prepared the same solution for this issue but you were first. :-)

avatar anibalsanchez anibalsanchez - test_item - 2 Sep 2015 - Tested successfully
avatar Bakual
Bakual - comment - 2 Sep 2015

But doesn't that mean that the original PR could still cause 3rd party extensions to fail with Joomla 3.4.4? If they do a similar thing as our installation does?

avatar Fedik
Fedik - comment - 2 Sep 2015

@Bakual I not seen such, but yes ...
and I do not think that it is right usage of the form :smile:

avatar Kubik-Rubik
Kubik-Rubik - comment - 2 Sep 2015

The problem is that it worked "by accident" before. We've set a hidden field with no value, so this has to be considered in the processing of the request. The behavior now is correct, we just used a bug in the installation process. Wrong assumption, see here: #7799 (comment)

avatar Bakual
Bakual - comment - 2 Sep 2015

I do not think that it is right usage of the form

First, that doesn't matter and secondly, we did use it ourself. So it's hard to argue that way when introducing a B/C break.
The safe way would be to revert the original PR.

avatar Fedik Fedik - change - 2 Sep 2015
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2015-09-02 15:50:26
Closed_By Fedik
avatar Fedik
Fedik - comment - 2 Sep 2015

well, I just suggested the fix

avatar Fedik Fedik - close - 2 Sep 2015
avatar Fedik Fedik - close - 2 Sep 2015
avatar Fedik Fedik - change - 2 Sep 2015
Status Closed New
Closed_Date 2015-09-02 15:50:26
Closed_By Fedik
avatar Fedik Fedik - reopen - 2 Sep 2015
avatar Fedik Fedik - reopen - 2 Sep 2015
avatar Fedik
Fedik - comment - 2 Sep 2015

sorry wrong button :smile:

avatar alikon
alikon - comment - 2 Sep 2015

honestly i'm unable to quantify the potential "side-effect" for 3dp extensions, templates ect...
.but.. should be better to not discover the day after :boom:

avatar Fedik
Fedik - comment - 2 Sep 2015

Potential issue can happen when developer has some field in the form.xml but do not render it in form HTML(for some tricky hack) ... so when form will be submitted then JForm::filter() set this value to "null" ... as empty.
Side effect will depend from further script logic ...

From other side, such JFrom behavior fix issue when need to submit unchecked checkbox(es) or unselected "multiselect" <select multiple="multiple">

avatar Bakual
Bakual - comment - 2 Sep 2015

It's actually perfectly fine to specify fields in the XML which will not be rendered in the form.
It may be due to ACL where some fields get removed, or there may be parameters which disable part of functionality.
There quite sure also exist layout overrides which reduce the form to the bare minimum. Which is also perfectly fine.

There is also another example how that PR #7381 broke core:
Edit an article and assign an intro image to it. Then go to frontend and edit and save the article there. With default settings for com_content, the intro image will be removed. This is because by default we don't show the "Images and Links" tab in frontend editing. Thus those fields will not be rendered despite being specified in the XML.

So I think the solution is to revert that PR because it has a wrong assumption to it. The XML doesn't specify which fields need to be present in a form. It only specifies which fields may be present. But there may be more or less fields than specified.

avatar Kubik-Rubik Kubik-Rubik - change - 2 Sep 2015
Status New Closed
Closed_Date 0000-00-00 00:00:00 2015-09-02 21:58:24
Closed_By Kubik-Rubik
avatar Kubik-Rubik
Kubik-Rubik - comment - 2 Sep 2015

@Bakual Very valid points and I agree that we have to change the behaviour that was introduced with the PR #7381. But I also do not like how it is handled in the previous PR #2617. Because it is not consistent with our other fields! If the developer sets a default value for a multiple checkbox (e.g. default="1,2"), then the user should no be able to deselect all options. It should behave just like all other fields (e.g. text). If you define a default value, then this value is set automatically if no value is transferred. The same with a non checked checkbox, the request variable does not have to be set at all (as it is done in normal requests too).

I will close this PR now because this is not the correct solution and will provide a PR based on #2617 but without the multiple checkbox processing. Thank you @Fedik for your effort!

avatar Kubik-Rubik Kubik-Rubik - close - 2 Sep 2015
avatar Kubik-Rubik Kubik-Rubik - close - 2 Sep 2015
avatar Fedik
Fedik - comment - 3 Sep 2015

I stay on point: The XML specify which fields must be present in the form.

In joomla case the Form provided by Model, and so Model expected the values of defined fields, not more not less, so we have more control of what user do. Otherwise why we need that JForm at all, we can just render all in HTML layout :smile:

About ACL example and hiding the field in HTML.
Developers should hide/remove the field in Model level, on prepareForm, if they do it in HTML layout then I can simply bypass this "high protected" form, just add needed input using Browser DevTools, and such system will eat it.

Well, but :bug: in articles images is not good :smile:
I think at least we need to review this topic for Joomla 4.

About #2617 there couple problem:

  • it forse array for multiple field, but must be null for empty value
  • it load all field instances, that not efficient (actually this made some problem in one of my work)
  • it still do not fix "unchecked single checkbox" problem
avatar Fedik
Fedik - comment - 3 Sep 2015

ps: I think, current pull still valid, as can protect us from future problem, related to unrendered db_type field in the Installer :wink:

avatar Bakual
Bakual - comment - 3 Sep 2015

The XML specify which fields must be present in the form.

That assumption is wrong.
Yes, a user may alter the HTML and thus the form. It is actually a feature of Joomla if you do it in a template override.
The form isn't "high protected" at all. Why should it be?
An extension which hides a field based on ACL must do the proper ACL checks also during save. For those exact reasons. Otherwise the extension is vulnerable.

avatar Fedik
Fedik - comment - 3 Sep 2015

That assumption is wrong

that assumption can save a lot of time for searching the reason of the data inconsistency :smile:

An extension which hides a field based on ACL must do the proper ACL checks also during save. For those exact reasons. Otherwise the extension is vulnerable.

I tried to explain, if developer prepare the form correct, (eg. do ACL check on prepareForm so can be sure that all data will be filtered correct (remove unused field values)), then he/she will have less work later on data save

avatar mbabker
mbabker - comment - 3 Sep 2015

Yes, a user may alter the HTML and thus the form. It is actually a feature of Joomla if you do it in a template override.

That's actually a very serious design flaw with Joomla's form API. Other projects in the PHP world have a high level of default validations, one of which includes ensuring a submitted form does not include data for fields that are not defined in the form's definition. The ability to add and remove fields from a form is no issue at all, but that is not an action that is to be left in the hands of the rendering engine (otherwise the validation API would have to check whether the form is defined and whether it is rendered, and that's totally inefficient).

We're screwed with the B/C policy, but in 4.0 the validation rules must be toughened. If you really want extra fields in a form, you have to do it through a plugin to ensure everything is validated and processed properly. Adding random fields to a form in a layout should cause an Exception to be thrown when validating the submitted data because a field is not defined. You could even make an argument for an Exception to be thrown if there is no data set (this includes a null value) for defined fields when processing the submitted data, this means someone's incorrectly stripping form elements somewhere.

avatar Bakual
Bakual - comment - 3 Sep 2015

Sure, for 4.0 we could change this. But honestly I don't see it as an issue we need to solve.
If someone adds more fields to a rendered form, who cares. It will be ignored by the extension who created the form (JForm already strips them afaik). But it may very well be processed by a plugin.

If someone removes fields, then imho the only concern would be when that field is required. That is something which should be checked by JForm. But for fields that are optional? Why would we want to enforce the presence? It should be perfectly fine to make our option filled edit forms simpler by stripping them to the minimum in a layout override. It shouldn't need plugins to do such basic things.

Other projects in the PHP world may have other requirements for their forms. We're talking here about a CMS where administrators should be able to manipulate the output using template overrides. We shouldn't restrict them when there is no real reason for it.

avatar mbabker
mbabker - comment - 3 Sep 2015

I'm talking about frameworks which are used to build CMS'. There is strict data validation in these projects; you cannot render a field that isn't defined, and you cannot submit data in a field that is not defined. Quite frankly, if the renderer gets to make the ultimate decision in a form's structure, then JForm is bloat and should be dropped.

avatar Bakual
Bakual - comment - 3 Sep 2015

you cannot render a field that isn't defined, and you cannot submit data in a field that is not defined.

I don't think you can restrict the rendering of additional fields as long as we have our template system. Even if you did, it still could be added by the user in the browser. So that would be pointless.

The submitting part we already do. After the form validation, you only have the formfields specified in the XML. Additional fields are stripped. Which of course is fine and correct.
Plugins which want to process additional fields have to fetch the raw data again, basically running their own form validation or processing raw inputs.

The only point we're not doing is if fields are removed and not sent at all. JForm currently doesn't care about that. imho, that's fine as well as long the field isn't specified as requried. I don't see a reason to enforce the presence. Not sure why other projects seem to enforce that.

avatar Fedik
Fedik - comment - 3 Sep 2015

I don't see a reason to enforce the presence.

example you have the field that do not submit the value when it unchecked/unselected or removed by javascript logic ...
and:
at first submit: User selected there some value,
at next submit: User "unselected" previous value,
so, how you determine that value changed from old value to null if that field does not sent any value at all? :wink:

avatar zero-24 zero-24 - change - 3 Sep 2015
Milestone Removed:
avatar mbabker
mbabker - comment - 3 Sep 2015

I don't think you can restrict the rendering of additional fields as long as we have our template system. Even if you did, it still could be added by the user in the browser. So that would be pointless.

You can restrict it. If you're looking for extra fields to not dump into the active form then you leave out all the jform prefixes for the HTML element names and IDs. If you're adding new fields to a form, it really needs to be done the right way through JForm's API to ensure the data persists and is processed correctly. Adding fields via the browser's console results in at least Symfony's API throwing Exceptions when it finds fields in the form that shouldn't be there (which you can bypass by not including the form element prefixes but at that point it isn't part of the form object in question so it isn't as big of a concern). Long and short, if you are adding a field to com_content's editor that you want to be considered as part of the article form, it has to be through the API. If you're just adding a field with some random function that isn't connected to the form object, go ahead and add it in the layout without the form prefixes; it isn't processed as part of the form then, no issue.

After the form validation, you only have the formfields specified in the XML. Additional fields are stripped. Which of course is fine and correct.

What we don't throw errors on, and should, is if someone injects additional fields to a JForm API defined form. In com_content, if the hits data is not part of the form but I add it via my browser with the correct markup, some sort of warning should be logged or thrown somewhere alerting the user that they're manipulating the form in a way that isn't allowed. Ignoring it seems like a bad idea to me. If I add a general hits field without the form markup, JForm doesn't consider it at all, and that's the correct behavior (consistent with other APIs too).

The only point we're not doing is if fields are removed and not sent at all. JForm currently doesn't care about that. imho, that's fine as well as long the field isn't specified as requried. I don't see a reason to enforce the presence. Not sure why other projects seem to enforce that.

Data integrity. What is the expected behavior if I strip the state field from com_content.article? Because of our API's inconsistency (as a whole, not just JForm now), it could either result in the existing value being retained, the existing value being nulled out, or the default being applied. The field isn't marked required in the form so by your argument it is perfectly fine for me to strip it. Other projects enforce the entire defined form to be rendered and submitted (even if that means you're using hidden fields for stuff you don't want the user to "see" with empty values), why do we have an API that makes it so easy to break the form definitions?

avatar Bakual
Bakual - comment - 3 Sep 2015

it could either result in the existing value being retained, the existing value being nulled out, or the default being applied.

I would expect it to keep existing values if I edit an item. Which imho is current behaviour.
For new items I would expect it to apply default values, which it currently doesn't do. We usually add those during save in the model/table where needed.

example you have the field that do not submit the value when it unchecked/unselected

So it's only a concern for checkboxes since those behave that (very strange) way. I think you already proposed a solution using a hidden field before the checkboxes? So we could determine if the field was present and unchecked or not present at all.

avatar Fedik
Fedik - comment - 4 Sep 2015

I think you already proposed a solution using a hidden field before the checkboxes?

Yes, that was the start point.
But later I thought as JForm already know all Fields why we should play with that "hidden field trick" at all ... make it in JForm::filter() will be more clear solution.
Also I have remembered my experience with Drupal form, there you have the form instance (it actually array of the fields) and that instance used to check the fields, if value was not submitted it will take the default one from that instance, so in result you always have all values of that fields ... if i right remember :smile:

Yes it can make a bit complicated to hide the field in layout ...
But, as @mbabker already wrote, it not right,
I also think the HTML layout should not change the App logic (means if JFrom has 5 field then the layout should render all 5 fields) ... developer need to make the plugin for it, at least.

Looking further.
Even the controller fields should be provided by Form instance (task, token etc). Then the template developer will not worry about what field should be in "that form layout" and just render all fields provided by the From instance.
This is safe for future updates with keep B/C ... example: we decide to use the additional controller field 'redirect' in some form, in current state the template developer must add it manually in his override, but if the field will be in Form instance he will not worry about that changes.
But this is another topic.

So I think it is right direction. Control the fields through JFrom.

avatar Fedik Fedik - head_ref_deleted - 6 Sep 2015

Add a Comment

Login with GitHub to post a comment