Instead of a fluent interface I'd suggest just return $this->setField() instead as that returns a boolean and would indicate to a dev an error happened (now the code ignores any potential boolean false returns).

Thanks for your help Michael, I modified the code as you suggested. By the way, could I ask is the check in this line of code needed

https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/form/form.php#L731. With that check, this command doesn't work:

$form = JForm::getInstance('sample-form', '<form></form>');

And I had to change it to

$form = JForm::getInstance('sample-form', '<form> </form>');

Note the space character between < form> and < /form> tags

@mbabker
I am quite fond of fluent interfaces. Especially for stuff like this, where you probably want to add multiple fields to a form in one go. So out of curiosity: why not keep the fluent interface and implement PHP exception handling for interception of any errors?

Frankly Exception handling in the CMS right now sucks and there are still a lot of places using boolean or null returns to flag errors versus Exceptions. If setField didn't have a boolean return value here it'd be OK but as it was that was being ignored. In general effort needs to go into fixing the error handling but there's only so far it can go without B/C breaks.

Thanks for the clarification,

@mbabker Actually, look at setField method, I don't see any case it return false, only return true or throws Exception

So I think the addField* method can return $this to support chaining, it is quite useful in case we need to build the form manually as showed in code example above.

How do you think about it?

As long as it isn't assumed it will always return true (I know setField
calls a couple other methods, no idea what they do for error conditions),
it'd be fine. It just needs to either allow an error to bubble up or
handle them in that method.

On Sunday, January 24, 2016, Tuan Pham Ngoc notifications@github.com
wrote:

@mbabker https://github.com/mbabker Actually, look at setField
method, I don't see any case it return false, only return true or throws
Exception

So I think the addField* method can return $this to support chaining,
it is quite useful in case we need to build the form manually as showed in
code example above.

How do you think about it?

—
Reply to this email directly or view it on GitHub
#8973 (comment).

I just had a quick check to see how the error is handled. Calling addField for a none supported field type and JForm just render textbox.

$form->addField('None_Supported_Type', 'user', array('label' => 'Group', 'description' => 'Select Your User Group'));

So with the way JForm works at the moment, I think the method addField can safely return $this.

I took a peek at setField() and agree with Tuan. But allow me to propose another approach: change addField() into newField(), returning the new element. You then leave it up to the calling routine to deal with the new element as it deems fit and/or handle any exceptions appropriately. Wouldn't that improve decoupling and flexibility?

isn't it what @mbabker suggested and how it is coded at the moment (except change name of the method from addField to newField)?

The way I understood @mbabker, is that he proposes to return $this->setField(...) from your addField() method. I propose to return $element from addField() and let the calling code determine what to do with that new element. And since you're not effectively adding anymore then, I thought it would make sense to rename the method to newField() ;)

Well, I always had trouble using setField() because of the way it is implemented. And hence I came up with a workaround. The setField() methods fails at following:

1. It assumes that the $group must be a name of existing <fields> group, effectively not allowing new groups to be inserted.
2. It injects the new field in the same <fieldset> as of the said $group (if any) otherwise places it inside the default fieldset always. No way to put the new field in any desired specific <fieldset>.
3. No way to place the new field before/after a specific form field. (I haven't found any fix/workaround for this yet)

Sometimes this is sufficient and sometimes not! So I used the workaround as below:

$form = new JForm('TestForm');

$xml = '<form>
            <fieldset name="test_fieldset">
                <fields name="test_group">
                    <field name="test_field_1" type="list" label="Test Field 1">
                        <option value="1">One</option>
                        <option value="2">Two</option>
                    </field>
                    <!-- ... more fields go here -->
                </fields>
                <!-- ... more groups go here -->
            </fieldset>
            <!-- ... more fieldsets go here -->
        </form>';

$form->load($xml, false);

This worked for me since long. I may be wrong in doing so, but that solved my issues effectively.

What I intend to say here is, if you could address above problems within your solution, that would be great. Otherwise I see not significant improvement with the addField() method. However, this is relatively easier than the existing setField().

PS: (unrelated) If you can do something about the feature to allow adding/removing list options to existing list type fields (radio/list/checkboxes etc.), that would be awesome.

@izharaazmi I see your points about the limitation of setField method. However, I think if we could address it, that would be a different PR. Proposing this PR, I just want to:

1. Make it easier to add a new field to existing form without having to create SimpleXMLElement to define the form field as we are having to do at the moment.

2. Allows building a dynamic form (like a form extension where fields are defined and stored in a database table instead of in an XML file).

3. Providing a way to use Joomla built-in HTML make up of form fields instead of writing plain html code like

<input type="text" name="first_name" value="First Name" class="input-large"  />

@joomdonation Yes, I agree. And the height of coincidences, I am right now working on such a form plugin .
With your patch my world could be simpler.

Why not PR against framework (Form) than cms ? this is for third application purpose no?

Although I can not find any (un)official guide lines, the general consensus seems to be to only use @throws to document methods that actually throw an exception.

I would also like to draw attention to my earlier suggestion to not call setField() from addField(), but instead change addField() into newField() returning the constructed element and leaving it up to the calling code to pass it on to setField() and/or handle any exceptions.

IMO, the idea of newField() does not fit into the JForm class as it is then no way associated with the JForm object in question.
However the same concept can be applied if we put that logic in JFormField class as a new static method such as JFormField::build() or JFormField::createElement(); or JFormField::fromArray() etc. with appropriate code adjustment.

However the same concept can be applied if we put that logic in JFormField class as a new static method such as JFormField::build() or JFormField::createElement(); or JFormField::fromArray() etc. with appropriate code adjustment.

Please use static methods minimally. They are very difficult to adequately test because you cannot mock any internal dependencies. Simple static methods (doing mathematic operations, anything relying only on the injected parameters, or factory methods with next to no business logic that only return new instances (not singletons, those are worse)) are OK, but the second your static method starts calling something like JFactory or AnyClassWithSingletonStorage::getInstance() then you're creating a nightmare to test and maintain.

I have tested this item ✅ successfully on ef41b6e

tested @icampus

added example code to a component, it creates a simple form with some fieds in it.
addField() creates easily field elements with different types without creating a xml file.

I have tested this item ✅ successfully on ef41b6e

Applied the patch and tested the example code in my component.
It's really nice that you can add elements so easy direct in the php file.
Tested@icampus

RTC

I like the idea to be able to add field in another way than simpleXmlElement, but why not "pushing" the idea further ?

This PR add only 1 "new way" to add field (php array), maybe we could use json, yaml,...

// Start with an empty form
$form = JForm::getInstance('sample-form', '<form> </form>');

// new "system"
$formBuilder = JFormBuilder();
// adding 1 field: $argument could be any type (array, json, ...)
// maybe $argument could be an array of many fields
$formBuilder->add($argument);

// with formBuilder we can reorder, delete, update....

$xml = $formBuilder->form();
$form->load($xml);

So:

• Make it easier to add a new field to existing form without having to create SimpleXMLElement to define the form field as we are having to do at the moment.
• Allows building a dynamic form (like a form extension where fields are defined and stored in a database table instead of in an XML file).
• Providing a way to use Joomla built-in HTML make up of form fields instead of writing plain html code like
• It injects the new field in the same as of the said $group (if any) otherwise places it inside the default fieldset always. No way to put the new field in any desired specific .
• No way to place the new field before/after a specific form field.

Maybe my idea is bad (to high for the cms) , maybe better to move this for the framework

@Devportobello Some nice ideas but definitely out-of-scope for this PR. Each idea seems to be big enough for a PR on it's own. The only one I have my doubts with is creating a pseudo HTML.

I set this to needs review because there is a problem with adding fields this way. You need to define the fieldset you'll add it to, if you don't then the field wont be displayed when you output a form like this

foreach ($this->form->getFieldsets() as $fieldset) 
{
    $fieldsInFieldset = $this->form->getFieldset($fieldset->name);

    foreach ($fieldsInFieldset as $field) 
    {
        echo $field->label
        echo $field->input;
    }
}
```  <hr /><sub>This comment was created with the <a href="https://github.com/joomla/jissues">J!Tracker Application</a> at <a href="https://issues.joomla.org/tracker/joomla-cms/8973">issues.joomla.org/joomla-cms/8973</a>.</sub>

@rdeutz I looked at it before. Unfortunately, there is no easy way to add fields to a fieldset using JForm API at the moment (you can see the same limitation in setField method)

So if defining the fieldset is a requirement, please close this PR.

I have worked on a PR #10350 adding this functionality but while writing better test instructions I have found a bug in my code. Fixing it is on my list, if I am done with it you can base your PR on it. Stay tuned :-)

closed on accident, button too close to each other.

@rdeutz did you write that PR ?

no on my list