Failure
Related to # 5147

User tests: Successful: Unsuccessful:

avatar okonomiyaki3000
okonomiyaki3000
28 May 2013

First, make JFormFieldCheckboxes and JFormFieldRadio subclasses of JFormFieldList so that there is only one implementation of getOptions().

New class JFormOption and assorted option-type classes to be used to generate lists of options based on the xml element.
Other related changes to JForm and JFormHelper.

avatar okonomiyaki3000 okonomiyaki3000 - open - 28 May 2013
avatar okonomiyaki3000
okonomiyaki3000 - comment - 28 May 2013

So what is this? It's an improvement or evolution of what I started here #1143.

It makes obsolete: JFormFieldCacheHandler, JFormFieldDatabaseConnection, JFormFieldFileList, JFormFieldFolderList, JFormFieldImageList, JFormFieldInteger, JFormFieldLanguage, JFormFieldPlugins, JFormFieldSessionHandlers, JFormFieldSQL.

These can now all be created using the list, checkboxes, or radio field types by supplying an <option/> of the proper type.

It's now possible to create your own option type (requires only a getOptions function) that is automatically usable by JFormFieldList, JFormFieldCheckboxes, and JFormFieldRadio.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 28 May 2013

Probably my @since tags and other things are wrong. I'm not sure what they should be set to now.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 29 May 2013

If this is accepted (and it should be, because it's rad to the max!), I'll go ahead and make analogous changes to the Framework as well.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 5 Jun 2013

If it wasn't clear, this PR is a solution for this tracker item: http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=30884

But it's also much more.

avatar beat
beat - comment - 15 Oct 2013

Looks interesting, and I like the DRYer approach. If I understand the code correcty (maybe a few examples would help) you would have something like:

<option type="images" /> ?

Seems a bit strang to define the plural options type within the single option of a field. Shouldn't that be one level up ?

And are there any B/C issues ?

avatar okonomiyaki3000
okonomiyaki3000 - comment - 15 Oct 2013

I suppose we could call the tag "options" instead if that makes more sense. Of course the singular version still exists so I thought it was bit simpler to ignore the singular/plural distinction.

There should be no serious B/C issues as long as the old field types go through a proper deprecation phase (or just keep then around forever, it shouldn't be a problem). There is one obscure issue related to some JText keys. I don't think it would make any practical difference at all but I can give it a closer look tomorrow to make sure.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 16 Oct 2013

So, to follow up on the JText thing, in JFormFieldList, we get the text for the option by passing the string value of the <option> tag through JText::alt() with the fieldname as the 'alt' key. JFormFieldRadio and JFormFieldCheckboxes do not do it this way, they just pass the string value of the <option> tag through JText::_().

I am just using JText::_() in all cases. Theoretically this could be a problem for some lists that actually use the feature of JText::alt(). I suspect this never happens but maybe it's better to use JText::alt() in all cases since it falls back on JText::_() anyway.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 16 Oct 2013

Now, some code samples. Any of these option types can be used with the list, radio, or checkboxes field types like this:

<field name="title" type="checkboxes" label="Select some articles">
    <option type="sql" query="SELECT id AS value, title FROM #__content" />
</field>

The SQL option type also supports this nifty feature:

<field name="title" type="checkboxes" label="Select some articles">
    <option type="sql" query="SELECT id AS value, title FROM #__content" >
        <query driver="mysqli" query="SELECT `id` AS `value`, `title` FROM `#__content`" />
        <query driver="sqlsrv" query="SELECT [id] AS [value], [title] FROM [#__content]" />
    </option>
</field>

This means that you can write a default query (in the <option> tag) and then override it for specific drivers by using a <query> tag.

If you only need a very simple query that returns a whole table, you don't even need to write any SQL, just use the table option type:

<field name="title" type="checkboxes" label="Select some articles">
    <option type="table" table="#__content" value="id" text="title" />
</field>

There are option types to replace all the existing field types that inherit from JFormFieldList but you get a lot of extra benefits. Most obviously, you have the option of using them with checkboxes or radio buttons instead of just selects. Also, since options can be combined, you can create lists that wouldn't have been possible before like:

<field name="title" type="checkboxes" label="Select some images">
    <option type="images" directory="images/folder1" />
    <option type="images" directory="images/folder2" />
</field>

I think there are a lot of benefits to this approach. Some of them are maybe obvious from these examples. Also, instead of having a dozen or so different list-like field types, each with their own specific supported attributes, we need only have list, radio, and checkboxes. The specific attributes needed or supported by each option type go in the option tag instead. This doesn't really make things simpler, it just shifts complexity around a bit but I somehow feel it's cleaner and easier to understand.

avatar beat
beat - comment - 16 Oct 2013

Nice! Thanks for the examples, helps understand.

avatar eddieajau
eddieajau - comment - 26 Nov 2013

My first impression is that we aren't separating the "model" (the SQL query) from the form. I hindsight, I would never have put an SQL type and try to define queries in XML. That's crazy and you've given an example why (different queries to support different db engines when we've already solved that with the DatabaseQuery class.

However, I think the "idea" is worth exploring. I'd be thinking about an <options /> tag which you could link to a PHP model that the developer includes in their extension. Thinking out loud:

<field name="my_list" type="list" label="We only need a generic list field really">
    <option>A fixed option</option>
    <options model="SomeModel" />
    <option>Another fixed option</option>
</field>

In that example, the tag is just a wrapper to execute some code the developer has already defined, probably as an interface JFormOptionsInterface which requires a getOptions method. That PHP class can then construct a query the right way, probably using a model from the source component.

The more I think about it, the more I think we should deprecate the ability to construct queries in the XML. Yes I know it's convenient but so is using globals and we all know why we don't use them.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 27 Nov 2013

@eddieajau I don't disagree with you about the SQL type. It's very convenient but maybe shouldn't exist at all. I've only recreated it as an option type for completeness, not as an endorsement.

What you are suggesting is basically what I've done here. Rather than 'model' I'm calling it 'type' and handling the extension aspect of it through the same paradigm that already exists for adding new Form, Field, or Rule types. So that, if your extension needs special option types, you can put an addoptionpath attribute in your xml and all of your new option types will be found.

The issue of <option> vs <options> is worth considering. I don't have a strong opinion either way. I chose simplicity of code over correctness of grammar by just making everything <option> regardless of how many options would actually be created. It's worth noting that, even if we call the fixed option <option> and the other types <options>, it's entirely possible that the plural tag could end up being only one option (or even zero, maybe).

avatar eddieajau
eddieajau - comment - 27 Nov 2013

Rather than 'model' I'm calling it 'type'

Sure, it's just that we use "type" so much it losses it's meaning and it would be good to avoid a clash with the <field> type. If it's called "model" then there is an explicit meaning to it: oh, that's the link to the data-provider thing in the PHP code.

I chose simplicity of code over correctness of grammar by just making everything

I think the way you could explain it is that the <option> tag is mirroring the true HTML tag. The <options> plural version, however, is a directive with a special Joomla meaning (an <option> tag loader, you could even call it <loadOptions model="foo" param1="bar" /> but it doesn't sound as nice).

even if we call the fixed option and the other types , it's entirely possible that the plural tag could end up being only one option (or even zero, maybe).

Yes, it could be the only one. I just added the other two to show there would be an opportunity to prepend and append fixed values. Currently we only allow the ability to prepend, or is it append.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 27 Nov 2013

Yes, it could be the only one. I just added the other two to show there would be an opportunity to prepend and append fixed values. Currently we only allow the ability to prepend, or is it append.

This is not exactly what I was getting at. I mean that if you have an <options> tag which results in some options being created by a model or whatever, that model could return only one or even zero options. So that the <options> tag does not necessarily stand in for multiple options. But again, I'm not too concerned either way.

avatar eddieajau
eddieajau - comment - 27 Nov 2013

that model could return only one or even zero options

Yes that's right, but in PHP would we write a list getter as getThings in the plural. We use the same convention in RESTful services, eg GET /things/?filter=foo.

Hrm, the other thing we should think about is does it translate to JSON format well (thinking in the future JForm is not tied to XML).

{
  "fieldset" : {
    "group": "basic",
    "label": "BASIC_TAB",
    "fields": [
      {
        "type": "list",
        "label": "FOO_LABEL",
        "options": [
          {
            "text": "An option",
            "value": "the-value"
          },
          {
            "dataProvider": "\\Vendor\\Models\\BasicOptions"
          }
        ]
      }
    ]
  }
}

Hrm, maybe you are right. Stick with option, either:

<option value="value">Text</option>
<option dataProvider="someClass" params="..." />
avatar okonomiyaki3000
okonomiyaki3000 - comment - 27 Nov 2013

There are only two hard things in Computer Science: cache invalidation and naming things.

-- Phil Karlton

Well, I still like 'type' but I'm willing to go with 'dataProvider' or anything else if this feature can become a reality.

avatar eddieajau
eddieajau - comment - 27 Nov 2013

Well, I still like 'type' but I'm willing to go with 'dataProvider' or
anything else if this feature can become a reality

I think go with either "model" or "dataProvider"; "model" is shorter.
Regardless, what would the plan be? Deprecate a heap of JFormField classes,
adding a heap of models to replace the data-getting, and then refactor
the code to use more basic field types?

Thinking wider, does the "idea" translate well to other field types? Can we
use the same principle for a captcha field, etc?

avatar okonomiyaki3000
okonomiyaki3000 - comment - 27 Nov 2013

Regardless, what would the plan be? Deprecate a heap of JFormField classes,
adding a heap of models to replace the data-getting, and then refactor
the code to use more basic field types?

Yes. That's basically what I was thinking.

Thinking wider, does the "idea" translate well to other field types? Can we
use the same principle for a captcha field, etc?

I honestly have never given the captcha field a moment's thought. I really only considered this as being useful by List, Checkboxes, and Radio. One thing I guess I need to consider is how JFormFieldGroupedList would or should be affected. I don't know if it's possible but I think that, ideally, there would be no GroupedList class, the regular List class should be able to be grouped or not grouped on a case-by-case basis. After all, this is how it works in HTML anyway. You use the same <select> tag whether it's a grouped list or not. So maybe this can be done. A question then would be, if you have grouped options for a Checkboxes or Radio field, should these be rendered in some special way? I could imagine that this might be useful in some cases.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 27 Nov 2013

Ah! There was a reason for using 'type'. Since nearly all of these new option types are based on an older field type, to make the transition as painless as possible, they also use the same attributes. Any attribute that didn't exist for <field> tags but would be standard across all <option> tags could potentially cause a conflict. So keeping the same attribute names made a lot of sense. I don't expect 'model' to cause any conflict though, so I don't mind using it.

avatar phproberto
phproberto - comment - 6 Dec 2013

I love this! :+1:

I have some suggestions:

  • I would change the option tag too. It will be easier to identify the new options system. The more "natural" to me is options. Yes it can be 1 or none but it's the same concept behind the getOptions method, you can get 0, 1 or more options.
  • For the type I think I like provider. This is a new feature so I think it's good to use a new attribute. I don't like model because it restricts the concept to one data provider. Something like provider="model" would do the same but be more powerful:
<options provider="model" model="ContentModelArticles" params="...">

If you define a getOptions in your model it will be used automatically. The helper would basically load an instance of the model with ignore_request. Then setState with the params received, call getItems and populate options with the results.

  • I hate using JHtml to generate options. It just adds a dependency and complexity to create an object. I would replace:
                                $options[] = JHtml::_(
                                        'select.option',
                                        $item->$key,
                                        $translate ? JText::_($item->$value) : $item->$value,
                                        'value',
                                        'text',
                                        $disable ? (bool) $item->$disable : false
                                );

with:

$options[] = (object) array(
    'value'   => $item->$key,
    'text'    => $translate ? JText::_($item->$value) : $item->$value
    'disable' => $disable ? (bool) $item->$disable : false
);

IMO easier to read and without dependencies. Most of the times is just a text + value.

Thanks for your work!

avatar okonomiyaki3000
okonomiyaki3000 - comment - 9 Dec 2013

I like these ideas. I will try to use them.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 9 Dec 2013

I have a question. So, right now, the getOptions function is like this:

public static function getOptions(SimpleXMLElement $option, $fieldname = '')

$fieldname is included because some option types use it to translate the text of the option (maybe all should do this, actually). But would this actually be better?

public static function getOptions(SimpleXMLElement $option, SimpleXMLElement $field)
avatar okonomiyaki3000
okonomiyaki3000 - comment - 10 Dec 2013

OK, so I've changed from calling the various types of option tags types to calling them providers (I still like types better for consistency with fields, rules, and other things but I don't care enough to fight about it).

As @phproberto suggested, I've stopped using JHtml to generate option objects.

Then, I added new providers for Class and Model. Both just call a function on a class specified by the JForm xml tag but Model does as @phproberto suggested and passes ignore_request and sets the state using the xml tag's attributes.

Updated all the tests.

What I didn't do is change from <option /> to <options />. The main reason is that, as @eddieajau pointed out, we may someday be able to define JForms with JSON instead of xml and then we will need options as the name of the JSON property containing all options. Anyway, it's not necessary to specifically draw a distinction between the old option system and the new one. The standard option tag is transparently supported by this system anyway. No one needs to know or care that there is a difference in implementation.

I think $fieldname needs to be used a bit more consistently though. Its usage was quite inconsistent in all the field list types and has remained so in the option types but maybe now is the time to clean that up.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 18 Apr 2014

Hey guys, I still think this is a pretty keen idea.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 7 May 2014

Should I change this to the staging branch?

avatar okonomiyaki3000
okonomiyaki3000 - comment - 20 May 2014

The birthday for this awesome PR is coming up. I'm thinking of throwing a party, is anyone interested in attending?

avatar brianteeman brianteeman - change - 21 Aug 2014
The description was changed
Status New Pending
avatar nicksavov nicksavov - change - 21 Aug 2014
Labels Removed: ?
avatar brianteeman brianteeman - change - 22 Sep 2014
Category Libraries
avatar Hackwar
Hackwar - comment - 17 Nov 2014

@okonomiyaki3000 Could you merge in the latest changes, so that this could be tested?

avatar okonomiyaki3000
okonomiyaki3000 - comment - 20 Nov 2014

@Hackwar This is so old that it was to master instead of staging. What do you want me to do about that?

avatar okonomiyaki3000
okonomiyaki3000 - comment - 20 Nov 2014

Just in case it's better on staging, I've got a PR for that #5147

avatar Hackwar
Hackwar - comment - 20 Nov 2014

Can you close this one then?

avatar brianteeman brianteeman - change - 20 Nov 2014
Status Pending Closed
Rel_Number 5147
Relation Type Related to
avatar jissues-bot
jissues-bot - comment - 20 Nov 2014
avatar jissues-bot jissues-bot - close - 20 Nov 2014
avatar jissues-bot jissues-bot - change - 20 Nov 2014
Closed_Date 0000-00-00 00:00:00 2014-11-20 10:12:33

Add a Comment

Login with GitHub to post a comment