Toolkit\Obj: support array as the $property argument for getting multiple properties

[adamkiss]

This PR …

Features

Added support for getting multiple properties from Toolkit\Obj objects and derived objects, like so:

$thing = new Obj(['one' => '👋', 'two' => 'Kirby']);

$properties = $thing->get(['one', 'three'], ['three' => 'fallback']);
// results in ['one' => '👋', 'three' => 'fallback']

Ready?

• Unit tests for fixed bug/feature
• In-code documentation (wherever needed)
• Tests and checks all pass

For review team

Because it uses A::get() internally, getting non-existent properties in the array returns ['no' => null], which I'm not quite sure how to handle - it makes sense from a consistency POV, but might not be expected. I didn't want to pollute the PR with more robust edge case handling.

• Add changes to release notes draft in Notion
• Add to website docs release checklist (if needed)

[distantnative]

Not sure how the functionality should be

$thing = new Obj(['one' => '👋', 'two' => 'Kirby']);
$properties = $thing->get(['foo', 'bar'], 'or-not');

// result: 'or-not'
// result: ['foo' => 'or-not', 'bar' => 'or-not']

[adamkiss]

@distantnative In my opinion, it should be result: 'or-not', because in less contrived example, the $fallback will probably a default array and that makes more sense.

$props = $thing->get(['id', 'name'], ['id'=>null]);

I am open to suggestions though

[lukasbestle]

But what happens if only one of the multiple properties doesn't exist? I think the method should then return the actual values for the properties that do exist and the default only for the others that don't. What about this:

$thing = new Obj(['one' => '👋', 'two' => 'Kirby']);

$thing->get(['one', 'foo', 'bar'], 'or-not'); // ['one' => '👋', 'foo' => 'or-not', 'bar' => 'or-not']

$fallback = ['one' => '1', 'foo' => 'fooo', 'bar' => 'pretty-high'];
$thing->get(['one', 'foo', 'bar'], $fallback); // ['one' => '👋', 'foo' => 'fooo', 'bar' => 'pretty-high']

[lukasbestle]

Thinking about it, maybe we shouldn't even support the "non-array fallback" case here. Otherwise you could pass an array by accident with the expectation that this value will be used for all non-existing items. If we always handle it like in my second example and fail on non-array fallbacks, the reliability would be improved.

[adamkiss]

@lukasbestle In that case, the whole thing would need to be quite a bit more robust, because simple merge in PHP would result in null overwriting the fallback

// example
$obj->get(['key'], ['key' => 'fallback']);

// naive way
A::merge($fallback, A::get($this->toArray(), $property);

// result: ['key' => null]

It would need to be something completely different to this naive A::get($this->toArray()) - which I'm fine with writing and testing, but I worry about the impact.

[lukasbestle]

Could be something along these lines:

if (is_array($fallback) !== true) {
    throw new InvalidArgumentException('Fallback must be an array when getting multiple properties');
}

$result = [];
foreach ($property as $key) {
    $result[$property] = $this->$key ?? $fallback[$key] ?? null;
}

return $result;

[adamkiss]

@lukasbestle Updated as per your idea.

I also updated the test, and I also added a test for the invalid fallback argument, but even if it's according to the docs/example from Kirby suite, it still doesn't validate the exception, but fail. I've no idea what's wrong with the test

[lukasbestle]

The PHPUnit error is indeed weird...
Can't see anything that could be wrong.

A few minor comments, maybe the resulting rerun of the CI will fix the PHPUnit error as well. ;)
To avoid multiple CI runs, you can add all suggestions you agree with to the batch and commit them at once from the GitHub UI.

[lukasbestle]

LGTM 👍

Leaving the final review to @distantnative.

[afbora]

Codes OK 👍

Just a little thought out loud;
I have a feeling that we should do it in a new/separate method. When I saw this method, I immediately thought of the A::pluck() method.

[adamkiss]

@afbora That is an option, but both A::get() and Collection::get() exist and while they are a bit different (collection methods vs single objects), you could also look at it as "collection of properties", and then it's in line with the other two.

Mainly - I've used Obj as the Base Class for my "sort of ORM" in two projects this last week, and $object->get([keys]) fits right in with the other A, $collection and $query methods.

[lukasbestle]

@afbora A::pluck() plucks a column from every element of a two-level array while $obj->get() returns multiple properties of the same single-level object. I agree with Adam that it fits more to A::get() than it does to A::pluck().