some more Form tests and some patches proposals

Le 11/09/2014 17:46, Richard Griffith a écrit :
>
> In htdocs/xoops_lib/Xoops/Form/GroupFormCheckbox.php:
>
> > +        $this->optionTree[$itemId]['parent'] = $itemParent;
> > +        $this->optionTree[$itemId]['name'] = $itemName;
> > +        $this->optionTree[$itemId]['id'] = $itemId;
> > +    }
> > +
> > +    /**
> > +     * Loads all child ids for an item to be used in javascript
> > +     *
> > +     * @param int   $itemId    item id
> > +     * @param array &$childIds child ids
> > +     *
> > +     * @return void
> > +     */
> > +    private function loadAllChildItemIds($itemId, &$childIds)
> > +    {
> > +        if (!empty($this->itemTree[$itemId]['children'])) {
>
> I have serious doubts about proliferating that ugly (and poorly 
> documented) code to facilitate testing. The only legitimate consumer 
> of the class is GroupPermissionForm -- the two are tied so tightly 
> that successfully using GroupFormCheckbox in any other context is 
> unlikely.
>
> Can we just add a predefined "tree" to satisfy testing? This code:
>
> |$form->addItem(1, 'permission 1');
> $form->addItem(2, 'permission 2');
> |
>
> results in this:
>
> |$optionTree = array(
>      0 => array(
>          'children' => array(1, 2),
>          'allchild' => array(1, 2),
>      ),
>      1 => array (
>          'parent' => 0,
>          'name' => 'permission 1',
>          'id' => 1,
>          'allchild' => array(),
>      ),
>      2 => array (
>          'parent' => 0,
>          'name' => 'permission 2',
>          'id' => 2,
>          'allchild' => array(),
>      ),
> );
> |
>
> Could we just use a array like that in the tests to inject a known 
> tree into setOptionTree() ?
>
> Rewriting/refactoring these two classes is best ultimate solution. A 
> lot of the complexity is due to the related client side javascript 
> (and support for a parent/child mechanism that does not seem to be 
> used anywhere.) A more elegant solution could be found, but for now, 
> it still works.
>
> —
> Reply to this email directly or view it on GitHub 
> <https://github.com/XOOPS/XoopsCore/pull/214/files#r17428868>.
>

Ok it is not a problem.

I will cancel my patch and revert to the version with setOptionTree()and 
inject a known option tree as You suggest.

I'd rather to write :

|$form->addItem(1, 'permission 1');
$form->addItem(2, 'permission 2');
|

than :

|$optionTree = array(
     0 => array(
         'children' => array(1, 2),
         'allchild' => array(1, 2),
     ),
     1 => array (
         'parent' => 0,
         'name' => 'permission 1',
         'id' => 1,
         'allchild' => array(),
     ),
     2 => array (
         'parent' => 0,
         'name' => 'permission 2',
         'id' => 2,
         'allchild' => array(),
     ),
);
|

But I will take into account your position. No code modification, just 
ensure verification of existing methods.

best regards.