Fix bug in `JDataSet` when unsetting within a foreach loop. #1730

Merged
merged 1 commit into from Dec 4, 2012

Projects

None yet

3 participants

Contributor

The use of offsetUnset on the current iterator point within a foreach
loop was causing unpredictable results. If the current pointer is being
unset, the pointer is moved back one unless it's the first element of
the array, in which case the pointer is set to false. The next method
then checks for this special case of false and rolls to be 'first'
element of the array (which would have been the 'next' one if the offset
had not been unset).

This PR also adds a new clear method to reset the contents of the set,
and a keys method that works in a similar way to the array_keys
function.

Tests and documentation have been updated accordingly. The tests on JData::count are also consolidated.

@eddieajau eddieajau Fix bug in `JDataSet` when unsetting within a foreach loop.
The use of `offsetUnset` on the current iterator point within a foreach
loop was causing unpredictable results. If the current pointer is being
unset, the pointer is moved back one unless it's the first element of
the array, in which case the pointer is set to false. The `next` method
then checks for this special case of `false` and rolls to be 'first'
element of the array (which would have been the 'next' one if the offset
had not been unset).

This PR also adds a new `clear` method to reset the contents of the set,
and a `keys` method that works in a similar way to the `array_keys`
function.

Tests and documentation have been updated accordingly.
b1a9f3f
@elinw elinw commented on the diff Dec 3, 2012
tests/suites/unit/joomla/data/dataTest.php
@@ -233,6 +233,29 @@ public function testBind_exception()
}
/**
+ * Tests the count method.
+ *
+ * @return void
+ *
+ * @covers JData::count
+ * @since 12.3
+ */
+ public function testCount()
elinw
elinw Dec 3, 2012 Contributor

From the way I've been taught it's always better to have separate tests that way when one fails the later ones still run plus it's much easier to trace failures.

eddieajau
eddieajau Dec 3, 2012 Contributor

I get what you are saying, and that method has been suggested to me as well, and I certainly use it and deliberately ignore to fit the occasion. The tests here were so trivial it's not worth breaking them up. Whatever the case, the test case is valid in it's own right anyway (assert an initial condition; do some incremental work; assert; do final work; assert). If the initial set is not empty, no point in proceeding. The second assertion is more or less redundant.

@LouisLandry LouisLandry merged commit dae7247 into joomla:staging Dec 4, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment