User tests: Successful: Unsuccessful:
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.
Probably my @since
tags and other things are wrong. I'm not sure what they should be set to now.
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.
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.
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 ?
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.
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.
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.
Nice! Thanks for the examples, helps understand.
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.
@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).
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.
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.
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="..." />
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.
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?
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.
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.
I love this!
I have some suggestions:
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.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.
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!
I like these ideas. I will try to use them.
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)
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.
Hey guys, I still think this is a pretty keen idea.
Should I change this to the staging branch?
The birthday for this awesome PR is coming up. I'm thinking of throwing a party, is anyone interested in attending?
Status | New | ⇒ | Pending |
Labels |
Removed:
?
|
Category | ⇒ | Libraries |
@okonomiyaki3000 Could you merge in the latest changes, so that this could be tested?
Can you close this one then?
Status | Pending | ⇒ | Closed |
Rel_Number | ⇒ | 5147 | |
Relation Type | ⇒ | Related to |
Set to "closed" on behalf of @brianteeman by The JTracker Application at issues.joomla.org/joomla-cms/1192
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-11-20 10:12:33 |
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
, orradio
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 byJFormFieldList
,JFormFieldCheckboxes
, andJFormFieldRadio
.