Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Refactoring JFormField & Co. #1684

Closed
wants to merge 6 commits into from

5 participants

@dongilbert
Collaborator

After several varying discussions on the topic with @elinw and others, and some of my own frustrations, I decided to give a go at refactoing JFormField and it's sister elements. The reason I did this is that a lot of times, the field specific implementation of getInput() would process some logic that was already completed by JFormField, such as deciding if an element is readonly or disabled.

Doubling up on the workload like that is never good. Now, JFormField::setup() does most of the heavy lifting when it comes to determining what settings are active/inactive, and the child classes simply worry about the html markup that those determinations require. This allows for broader, more expressive rulesets to determine functionality in the parent class, without having to be overly verbose in the child class. For example:

/**
 * instead of the child class doing this
 * (which is insufficient, what if the field was disabled="disabled"?)
 */
$disabled = $this->element['disabled'] == true ? 'some html' : '';

// it now does this
$disabled = !empty($this->disabled) ? 'some html' : '';

I did this to make it easy to add new features. In my case, I added the $placeholder attribute as a protected property to JFormFieldText and then extended similar fields that would use said attribute from that parent class.

Another new features is the $prependToClass property in JFormField. This allows you to set a class name that will always be prepended to the field. For example, look at JFormFieldEmail. It extends JFormFieldText, but also overrides the getInput() method, just to put in a validate-email class at the beginning of the input class. This also occurs in a couple other places, and adding his feature cuts down on that code bloat. Instead of rewriting getInput(), do this instead:

protected $prependToClass = 'validate-email';

Also new to the mix is that each child JFormField* class that has it's own specific attributes now has it's own setup() method that handles assigning those specific attributes to the field object. See JFormFieldTextarea for a good example of that - https://github.com/dongilbert/joomla-platform/blob/abf409ad2feeee98005c0e779dd6c20d4b3eb249/libraries/joomla/form/fields/textarea.php#L69

Last thing - I deprectated the $forceMultiple property. It has it's use, but that use can be fulfilled by the $multiple property instead. See the JFormField::setup() for how I implemented this - https://github.com/dongilbert/joomla-platform/blob/abf409ad2feeee98005c0e779dd6c20d4b3eb249/libraries/joomla/form/field.php#L427

I'm not claiming this is perfect, but it does pass all the tests. And it shouldn't break any backwards compatibility. I just want to start the conversation, since code speaks louder than words. :)

@mbabker
Owner

At a glance, things look pretty good to me. I'd have to play with this some to make sure nothing breaks, but you've got something good here IMO.

@elinw

Just so people understand, $forceMultiple exists in essence to solve a problem with checkboxes which is that they are required to accept multiples according to the W3C standard (the only field like this as far as I know). That means even if developers don't say multiple-="multiple" multiple still has to be true.

@dongilbert
Collaborator

@elinw - Thanks for the clarification on that. I'm not against $forceMultiple per sé - it's just that you can accomplish the same goal by using an existing property that makes just as much, if not more sense. This new implementation reduces 5 lines of code to 1, which is a good thing imho, and it doesn't break b/c (at least until 13.3, if $forceMultiple is removed as marked)

@elinw

I haven't played with it but I think it may make more sense to deal with the special needs of checkboxes in checkboxes, but maybe there is a more reason for this that I'm not aware of. I'm not sure how many people are changing checkboxes to not accept multiple values given that checkboxes itself was problematic unless you extended it until not that long ago

@dongilbert
Collaborator
libraries/joomla/form/fields/textarea.php
((42 lines not shown))
+ {
+ parent::setup($element, $value, $group);
+
+ if (!empty($this->element['cols']))
+ {
+ $this->cols = (int) $this->element['cols'];
+ }
+
+ if (!empty($this->element['rows']))
+ {
+ $this->rows = (int) $this->element['rows'];
+ }
+
+ if (!empty($this->element['placeholder']))
+ {
+ $this->placeholder = (int) $this->element['placeholder'];

Have to correct the cast to string:

$this->placeholder = (int) $this->element['placeholder']; to $this->placeholder = (string) $this->element['placeholder'];

@dongilbert Collaborator

Thanks - this has been fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
libraries/joomla/form/fields/password.php
@@ -30,6 +32,53 @@ class JFormFieldPassword extends JFormField
protected $type = 'Password';
/**
+ * Whether or not to use the strength meter
+ *
+ * @var boolean
+ * @since 12.3
+ */
+ protected $strengthmeter;
+
+ /**
+ * Strength threshold for the strength meter
+ *
+ * @var string
+ * @since 12.3
+ */
+ protected $threshold;
+

This field type could also make use of the placeholder attribute.

@dongilbert Collaborator

Since this class now extends JFormFieldText, it falls back on the setup method there, so it should have access to the $placeholder property

@dongilbert Collaborator

We just need to add the placeholder variable in the retuned input string in the getInput method.

Absolutely right. Just adding to the getInput() works fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mariopro

Don, just a though and I would like to know your thoughts about it. In the sql field type, wouldn't make sense now to have a json_decode true/false parameter? I ask because some of the multiselect fields that are populated from the sql, require the data to go through this function in the model.

@mariopro

Don, in the radio.php file, imho, seems to make some sense to set the checked radio as disabled. The reason for this comment might seem not so obvious because a checked radio won't change its value or state if the user clicks repeatedly over it, but, if developers want to use the radios to show/hide content, the checked radio not being disabled will respond to the user click and trigger any events that might be associated with it (imho).
In line 56, it could be set as :
$checked = ((string) $option->value == (string) $this->value) ? ' checked="checked" disabled' : ''; and perhaps change the on click behavior.

@dongilbert
Collaborator
@mariopro

You're abs right. Thanks Don.

@elinw

Radio buttons are also a weird special case since if there is only one it can't be unselected and multiple is forced to be false.

@dongilbert
Collaborator

The comment was supposed to say "Fixing undefined variable error"

@dongilbert
Collaborator

Any further discussion on this topic? Improvements I should make?

@dongilbert
Collaborator

I'm going to add some documentation for JForm - should I attach it to this PR or create a new one? The reason I ask is there currently is none, and writing up the full docs will take some more time than just documenting what's this PR changes.

Maybe I'll document the API as it is now in a separate PR, and then document the changes from this.

@elinw

@dongilbert I was thinking about this project when I was working with a field that won't let me update to blank once there is data. A number of people have raised this before. I wonder if we could come up with an attribute that would let us optionally allow this.

@mariopro

As I previously suggested in the platform discussion about this refactoring, imho, the attribute could resemble something as - updateNulls.

@dongilbert
Collaborator
@dongilbert
Collaborator
@mariopro

Json serialize would be very useful on multiples (this is how I handle them), but I'm not sure if there is consensus on this. Imho this would be the way to go for the multiples.
I have dealt with the update nulls on multiple with a tweak in the bind array, but in a form with several multiples, this becomes somehow heavy to code.
I don't remember any other checkboxes that don't submit when empty. Probably Elin could be the best person to answer that.

@dongilbert
Collaborator

I would like to propose another addition to the api - the getAttribute() method. We would use this method to replace duplicate code within field classes that generate markup for field attributes.

// We would change this
$disabled = !empty($this->disabled) ? ' disabled="disabled"' : '';

// To this
$disabled = $this->getAttribute('disabled');

With this approach, the actual markup generated is handled by the JFormField class, and it cleans up the mass of duplicate code while also making it easier to add new attributes if needed without having to copy markup across all JFormField* classes.

The method would look something like this: (I know, we'll need to build in some exceptions to the rules here)

/**
 * Generate markup for specified attribute.
 *
 * @param   string  $attribute  The name of the attribute for which to generate markup
 * @param   string  $default    What to return if $attribute is empty.
 *
 * @return  string  Generated attribute markup if $attribute is found, otherwise it returns default.
 *
 */
protected function getAttribute($attribute, $default = '')
{
    $attr = '';

    if(!empty($this->$attribute))
    {
        $attr = sprintf(' %s="%s"', $attribute, $this->$attribute);
    }
    elseif ($default !== '')
    {
        $attr = sprintf(' %s="%s"', $attribute, $default);
    }

    return $attr;
}
@mariopro

Looks very good and an good way to optimize the attributes.

@elinw

Only check boxes as far as I know.
I agree about multiple storage being standardized though of course options would be nice.

@mariopro

In the checkboxes, wouldn't it be possible to accommodate the bootstrap data-toggle="buttons-checkbox" that, as far as I could find, in Isis is only possible on radios, while bootstrap has this checkbox feature http://twitter.github.com/bootstrap/javascript.html#buttons. I presume that the only impediment now for the attribute to be used is the <li> that is used to enclose the checkboxes fields in JFormFieldCheckboxes class.
Using this attribute instead type="list" for the checkbox could be an option, imho.

@elinw

Defnitely worth looking at.
You may have noticed that the update I did of the email field included multiple to the html5 standard. That calls for a comma separated string of addresses. I tend to think that as a lowest common denominator that works.

@mariopro

Just a thought - in the JFormFieldFolderList, when the method starts to build the options list from the list of folders, wouldn't it be useful to add a couple of levels of recursiveness in this so that the drop-down that is rendered could accommodate, for instance, three child levels of the filtered folder? Example:

<field name="path_to_images" 
    type="folderList" 
    directory="images" 
    exclude="none" 
    levels="3" 
    label="FOO_LABEL" 
    description="FOOD_DESC"/>

The parameter "levels" (or something similar), if defined, could state the max levels of child directories to search and include, hierarchically in the list. This would render, in the drop-down, something similar to what we have in the categories:
Main
|-First child
|-- Second child
|--- Third child

@bcordis
@mariopro

And suddenly I've just stumbled on the JFormFieldFolderList "smarter brother" in the media manager (https://github.com/joomla/joomla-cms/blob/master/administrator/components/com_media/models/manager.php) where the getFolderTree method (class MediaModelManager) does exactly the above...
Question: couldn't this be transposed or adapted to the JFormFieldFolderList ?

@dongilbert
Collaborator
@mariopro

For a possible future review of the the JFormField classes, imho, the onChange parameter could be evaluated for a possibility to change it to "onAction" and allow onClick, OnBlur, onFocus (...) and onChange to be set as the value instead of parameter name (I'm not sure this can be done now on standard fields). An example:

<field name="field_name" 
    onAction="onFocus('do_something_with_js_or_jQuery')"
    label="FOO_LABEL" 
    description="FOOD_DESC"/>

The method would then add the parameter to the field, depending on what was defined and who knows allow multiple, comma separated, values <field name="foo" onAction="onFocus('do_something_with_js_or_jQuery'), onBlur('do_something_with_js_or_jQuery')" />
This would give some space to implement, for instance and among other things, some of the new CSS3 cursor types, or even free some jQuery/js resources that extension's developers might need to implement to act upon a user action on a field. For instance, onClick="jQuery('.class_name').show();"

@dongilbert
Collaborator

I want to recode some of this again, and create a common field interface that can be implemented by those creating custom fields for their extensions. Would that be a welcome change? There's still a lot of work to do to make this a true refactoring worthy of implementing. A lot of what's here already consolidates common calls and cleans things up, which is good, but I want to make it easier to extend and create custom implementations.

Closing for now. :)

@dongilbert dongilbert closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 12, 2012
  1. @dongilbert
  2. @dongilbert

    fixing cs issue

    dongilbert authored
  3. @dongilbert
Commits on Nov 13, 2012
  1. @dongilbert

    Fix wrong typecasting

    dongilbert authored
Commits on Nov 15, 2012
  1. @dongilbert
Commits on Nov 26, 2012
  1. @dongilbert

    Fixing error

    dongilbert authored
Something went wrong with that request. Please try again.