Make pivot key a required field

[izharaazmi]

The key argument to the ArrayHelper::pivot is currently optional and defaults to null. If omitted when calling this method this will always return a blank array. So I don't understand why it was set as optional?

I'd like to discuss on this case.

[mbabker]

It's been that way since the original PR to add the method (joomla/joomla-platform#257) and there is a test case (https://github.com/joomla-framework/utilities/blob/1.3.3/Tests/ArrayHelperTest.php#L438) with $key as a null which passes.

[izharaazmi]

The test case passes as the test case is built in favor of what it does. Also the test case lack in few important input conditions. The method simply keeps ignoring the values until end, and returns an empty array, which is not what one should expect.
Try this:

$input = array(
    array('one' => 1, 'two' => 2, 'three' => 3),
    array('one' => 11, 'two' => 22, 'three' => 33),
    array('one' => 111, 'two' => 222, 'three' => 333),
);

$pivot = Joomla\Utilities\ArrayHelper::pivot($input);

You'll get:

$pivot = array();

Now try this:

$input = array(
    array('one' => 1, 'two' => 2, 'three' => 3),
    array('one' => 11, 'two' => 22, 'three' => 33),
    array('one' => 111, 'two' => 222, 'three' => 333),
    'four',
);

$pivot = Joomla\Utilities\ArrayHelper::pivot($input);

and this:

$input = array(
    array('one' => 1, 'two' => 2, 'three' => 3),
    array('one' => 11, 'two' => 22, 'three' => 33),
    array('one' => 111, 'two' => 222, 'three' => 333),
    null,
    'four',
);

$pivot = Joomla\Utilities\ArrayHelper::pivot($input);

I can see it does (somewhat) the reverse of what invert does when the input array contains scalar values by simply ignoring the $key argument. However they are not fully complementary.

See this example:

$input = array(
    '1000' => 'New',
    '1500' => 'New',
    '1750' => 'New',
    '3000' => 'Used',
    '4000' => 'Used',
    '5000' => 'Used',
    '6000' => 'Used',
    '7000' => 'Refurbished',
);

$pivot = Joomla\Utilities\ArrayHelper::pivot($input);

Outputs:

$pivot = array(
    'New'         => array('1000', '1500', '1750'),
    'Used'        => array('3000', '4000', '5000', '6000'),
    'Refurbished' => '7000'
);

Now if we pass above to invert:

$invert = Joomla\Utilities\ArrayHelper::invert($pivot);

We get:

$invert = array(
    '1000' => 'New',
    '1500' => 'New',
    '1750' => 'New',
    '3000' => 'Used',
    '4000' => 'Used',
    '5000' => 'Used',
    '6000' => 'Used',
    // '7000' => 'Refurbished', <=== Missing value
);

Points to notice here:

• ArrayHelper::pivot operates on array of scalar (considering only specific case where $key is not used).
• If the input to ArrayHelper::pivot is an array of objects or an array of arrays and $key is null it makes no sense and always returns an empty array. Should this be considered an InvalidArgumentException case?
• I think making $key = null is only valid if the input is an array of scalars. In all other case it is a required parameter. Therefore, we may choose to accept explicit null value but not as optional with a default of null. (Mind the BC)
• ArrayHelper::pivot indeed provides a good reverse lookup however it has some issues that needs to be though over.
• ArrayHelper::pivot returns an inconsistent structure that sometimes causes problems. See the output above.
• Few more test cases should be added to ArrayHelper::pivot method.
• ArrayHelper::invert operates on array of array of scalar. Non array values in the main array are ignored.
• ArrayHelper::invert is not a suitable name unless it is self complementary.

This is my understanding and perspective. Please advise!

[mbabker]

I know I'm late as all hell on this, so looking at the change alone it's definitely a B/C break changing the signature.

For 1.x, I'd suggest checking $key === null and either raising an error condition or immediately return an empty array you say it returns now and on the 2.0 branch we can change the signature.

[izharaazmi]

I agree.
I think its better to raise an exception when the input to ArrayHelper::pivot is an array of objects or an array of arrays and the $key is null instead of simply returning empty array.

About 2.0 branch, can I propose to make the pivot behaviour consistent for all cases. I mean to change current output

$pivot = array(
    'New'         => array('1000', '1500', '1750'),
    'Used'        => array('3000', '4000', '5000', '6000'),
    'Refurbished' => '7000'
);

to

$pivot = array(
    'New'         => array('1000', '1500', '1750'),
    'Used'        => array('3000', '4000', '5000', '6000'),
    'Refurbished' => array('7000') // <-- this is changed.
);

Currently it needs us to always check whether the pivoted value is a multi-value or single-value.

[mbabker]

Both ideas sound good to me.

[izharaazmi]

I was trying to add an exception message but I see there is usage of JText so far in the utilities package. So this would add an extra dependency. I'm also not sure where to put/expect the language file.

Please suggest what would be the best way to raise the error condition.

[mbabker]

The Framework is weaning away from using translated Exception messages to using hardcoded English so don't worry about that aspect of things.