Description: ------------ It would be nice to be able to move an element before or after another. //moves el2 before el1 $form->moveElementBefore('el2', 'el1'); //moves el2 after el1 $form->moveElementAfter('el2', 'el1'); //moves el2 to position 5 (starting at 0) $form->moveElement('el2', 5); //adds element el2 after el1 $el2 = HTML_QuickForm::createElement('text', 'el2', 'some text', 'more text'); $form->addElementAfter($el2, 'el1'); //adds element el2 before el1 $form->addElementBefore($el2, 'el1'); I would find this funcitonality EXTREMELY helpful. The add addElementBefore/After funcitons aren't needed if the move functions are included.

I've created a patch that implements moveElement, moveElementAfter and moveElementBefore. moveElement maybe should just be taken out as hidden elements can easily create confusion as to where to move something. http://www.reversefold.com/PEAR/QuickForm_moveElement.patch

Note: before 6/21/2004 4:38PM PST the patch had an error in moveElementBefore. It should be fixed now.

I would love to see this patch applied to HTML_QuickForm; very useful when passing a form to and from different classes. Might even be good to have an additional "prependElement()" method to add to the top of the element stack instead of at the end.

The patch is incomplete. It does not take into account the possible existence of several elements with the same name (Think radios). The cleaner implementation may f.e. consist of passing the instance of element to move before/after instead of its name, but it will likely require PHP5.

Indeed, without PHP5, you'd have to give each element a unique id in order to find its position in the elements array. This feature will surely go into a QF4 version, I am not sure yet about QF3 having this.

Ok, I'll change it to fail if the element is a duplicate. In that case, the user will have to use a unique named group. Moving elements is something that would be *really* useful, even if it fails to work in some cases.

Patch updated. Duplicate elements can't be moved, it results in a returned PEAR_Error with a new error code, QUICKFORM_AMBIGUOUS_ELEMENT_NAME.

Added proposed prependElement() to patch.

Changed new prependElement to work like addElement (allow passing of an element or element args).

This is very useful -- even if it does not work in all cases (same-named elements case in point). Comes in handy when adding HTML_QuickForm elements to a formBuilder generated form.

I have updated the patch to conform to PEAR CS, added docblocks for the functions, added folding markers per the rest of the class (although I don't knwo why they're needed), and added more error returning (for when an element doesn't exist). Hopefully the patch can be applied to CVS now. AFAIK, there should be no problem with this patch now and it would be very useful to many people.

Fixed a possible bug in the patch when elements are removed from the form prior to moving elements. Is anyone currently looking at this patch for inclusion?

I've found another bug which could replace elements with copies of other elements due to reference strangeness. I've added two unset statements which should handle this to the patch.

Finally reviewed the patch. While the functionality it provides is indeed quite useful, the implementation is not acceptable for the following reasons: 1) the patch is way too big: 10% of current QuickForm.php either in line count or bytes. 2) It adds 4 new public functions to already bloated QuickForm API, of which * moveElement() uses internal element indexes which are not documented and not accessible from outside; * moveElementAfter(), moveElementBefore() and prependElement() are mostly redundant, the same functionality can be achieved with moveElementBefore() only Do not take it personal, I understand that you spent quite a bit of time writing the stuff, but adding bloat to main class is not the way we try to follow now. Since 1) QuickForm already has removeElement() method; 2) adding an element to a specific position is probably a more common task than moving an already added element; 3) DOM standard defines insertBefore() function and next major version of QuickForm will probably have API modelled after DOM; I suggest to rework the functionality of the patch: add only one new public method with signature insertElementBefore(&$element, $elName) In case you don't want to waste more time with QuickForm, I can do this myself, just let me know to prevent duplication of efforts.

Here is the script that shows the bugs in the proposed patch. Apply the patch and try running it. <?php require_once 'HTML/QuickForm.php'; $form =& new HTML_QuickForm('frmTest', 'get'); $form->setDefaults(array( 'iradTest' => 2, 'buglet' => 'What the?..' )); $form->addElement('header', '', 'Test moveelement patch'); $form->addElement('text', 'after', 'Test Text:'); $form->addElement('radio', 'iradTest', 'Test Radio Buttons:', 'Check the radio button #1', 1); $form->addElement('text', 'toRemove', 'I will be removed:'); $form->addElement('text', 'buglet', 'I will be left here:'); $form->addElement('radio', 'iradTest', '(Not a group)', 'Check the radio button #2', 2); $form->addElement('text', 'testMove', 'I will be moved forward:'); $form->addElement('submit', 'isubTest', 'Test Submit'); $form->removeElement('toRemove'); $form->moveElementBefore('testMove', 'after'); var_dump($form->exportValue('iradTest')); $form->display(); ?>

I've updated the patch. I've remove moveElement() as it isn't needed. I've left in _moveElement (the core method), moveElementBefore, moveElementAfter, and prependElement. I kept moveElementAfter() because you may want to move an element before, say, a radio button that is a duplicate. To do this you have to move it after the preceding element. IMHO, it's important to have a "move" rather than an "insert" method as some people may want to move an already created element. Say some code is making a form and they can't / don't want to touch it, so moving the element later would be useful. prepend is just a convenience method which could be removed. I just kept it in because I happen to like it. If we want less code, a method could always be added that has a parameter for whether it's a "move before" or a "move after". This would about halve the moveElementBefore and moveElementAfter lines of code.

The updated patch is *still* broken --- exportValue('iradTest') in the above example returns wrong result. I've commited my implementation of insertElementBefore() to CVS, it can be used to move the existing form elements. If you look at the semantics of DOM XML extension's insert_before() method [1], you'll see the following: ---------- (PHP >= 4.3 only) If newnode already is part of a document, it will be first unlinked from its existing context. ---------- In other words, it behaves much like the proposed moveElementBefore() method. This is, unfortunately, not easily doable in current PHP4 version, but will be implemented in the PHP5 one. [1] http://www.php.net/manual/en/function.domnode-insert-before.php

I'm going to take one last stab at this. I just realized what you meant about reindexing _duplicateIndex. I didn't catch that one thing from your example as you didn't explicitly give expected / wrong output. That problem is now fixed and tested with your testing code and with my code which has been in production for a while now (with large forms from DB_DO_FB). I've also stripped out lots of the duplicate code in the if/else in _moveElement and added one more function, _moveElementBA, to do all of the duplicate and existing checks for moveElementBefore and moveElementAfter. For further line savings, the bulk of _moveElement could be moved into the call in _moveElementBA. I left it out as it could possible be useful for other functions in its current state. Comparing added lines, your patch is 65 lines and mine is 119. If you remove the comments and whitespace, yours in 39 and mine is 63, 6 lines of which is basically stubs. IMHO, the size difference is more than made up for by the increase in functionality. You can move elements before or after any other. After is important if you want to put an element just before a duplicate. A comparison user's possible usage: Inserting an element before another: Your patch insertElementBefore($el, 'beforeElName'); My patch $form->addElement($el); $form->moveElementBefore($el, 'beforeName'); Moving an element before another Your patch $el =& $form->getElement('elName'); $form->removeElement('elName'); $form->insertElementBefore($el, 'beforeName'); My patch $form->moveElementBefore('elName', 'beforeName'); My patch ends up with one less average line comparing the two. This is a fairly trivial difference, but you could easily add an insertElementBefore() call to my patch, making it even simpler: function &insertElementBefore(&$el, $beforeElName) { $this->addElement($el); $this->moveElementBefore($el->getName(), $beforeElName); return &$beforeElName; } You could add a moveElementBefore function with your patch easily, but a moveElementAfter function wouldn't work for all cases and would be somewhat of a hack. In addition, insertElementBefore duplicates addElement's job of checking for duplicates and (in its current form) fails to check for the correct class of $element. With my patch, the adding code stays only in addElement. I understand your wish to stay with a DOM-like implementation but from what I've heard of from others in support of *moving* in particular and from the code differences outlined above I think that my patch for moving elements is not only more flexible and more wanted, but also more orthogonal to the current functionality in HTML_QuickForm.

I changed removeElement() so that it returns a reference to an element being removed. Thus now moving an existing element is a matter of one line: $form->insertElementBefore($form->removeElement('foo', false), 'bar'); Your comments about possible refactoring of addElement()/insertElementBefore() are valid, but I do not feel this stuff is urgent. Please also bear in mind that without PHP5 semantics of === operator for objects all these solutions are more or less hacks. And moveElementAfter() / insertElementAfter() is even a bigger hack, since it is not defined in DOM. And yes, I'd like to stick with DOM since that is the kind of API QF4 will most probably use.

I don't see how the === operator would fix anything. To move an element, you still have to move thigns around. All it would change is that you could move duplicated elements, but only if you were passing handles around instead of using names. IMHO names are much more easily usable anyway. I'm disappointed about all this. insertElementBefore is not a total fix for the reasons I outlined in my last post. It also gives much less functionality. The # of lines for the move call wasn't my main objective.