Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix JForm::load() not replacing form field in same location #129

Closed
wants to merge 1 commit into from

6 participants

@rvsjoen

When loading another form xml definition on a JForm object, and the new xml definition contains field names which overlap, the fields should be replaced in the same location.

In the current implementation, the old field is removed, and then the new field is added to the end of the form, changing the ordering.

This patch fixes the problem with reordering, but I am not sure if it has any other adverse effects, I will leave that up to someone who knows more about the workings of JForm.

@ianmacl

1) JFormTest::testLoad
Line:1243 There are 2 original ungrouped fields, 1 replaced and 4 new, resulting in 6 total.
Failed asserting that integer:7 matches expected integer:6.

2) JFormFieldHiddenTest::testGetInput
Line:81 The label of a non-hidden element should be some HTML.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-foo
+

3) JFormFieldSpacerTest::testGetLabel
Line:101 The getLabel method should return something without error.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-spacer
+spacer

4) JFormFieldSpacerTest::testGetTitle
Line:101 The getLabel method should return something without error.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-spacer
+spacer

Tests need to be fixed before this is committed.

@elinw

These tests are failing because when the replacement happens the results in the two fields replaces, which are hidden and spacer, are different than what happens when they are not replaced and therefore the actual and expected do not match. For example the non hidden element in the basic test is replaced with a hidden element which should not return html.

@Hackwar

Stupid question, but how is the original field replaced with your change? Does simpleXML recognize the fields by the name attribute or something? I thought you would have to find the current element and replace that element itself with your new element...

@joomla-jenkins
Collaborator

This pull request could not be tested since the changes could not be cleanly merged.

@elinw

Yes the name attribute.

@chdemko

@rvsjoen Any news on this (the pull request cannot be merged)?

@ianmacl

We'd like to get this in once we have tests and are confident it works correctly.

@ianmacl

(and once it has been rebased, etc)

@elinw

Ian Aaron posted a pull request for this.

#1069

@ianmacl

Closing in light of [#1069].

@ianmacl ianmacl closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 1 addition and 7 deletions.
  1. +1 −7 libraries/joomla/form/form.php
View
8 libraries/joomla/form/form.php
@@ -661,13 +661,7 @@ public function load($data, $replace = true, $xpath = false)
if ($current = $this->findField((string) $field['name'], implode('.', $groups))) {
// If set to replace found fields remove it from the current definition.
- if ($replace) {
- $dom = dom_import_simplexml($current);
- $dom->parentNode->removeChild($dom);
- }
-
- // Else remove it from the incoming definition so it isn't replaced.
- else {
+ if (!$replace) {
unset($field);
}
}
Something went wrong with that request. Please try again.