Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.4] refactoring mapWithKeys #16564

Merged
merged 10 commits into from
Nov 28, 2016
Merged

[5.4] refactoring mapWithKeys #16564

merged 10 commits into from
Nov 28, 2016

Conversation

fernandobandeira
Copy link
Contributor

Ping @vlakoff

With this change mapWithKeys() won't renumber the keys and will also maintain the order of the input array.

Example:

[
    ['id' => 1, 'name' => 'A'],
    ['id' => 3, 'name' => 'B'],
    ['id' => 2, 'name' => 'C'],
]

When using the function:

$data->mapWithKeys(function ($item) {
    return [$item['id'] => $item];
 });

Results into

[
    1 => 'A',
    3 => 'B',
    2 => 'C'
]

Intead of the current behavior:

[
    0 => 'A',
    1 => 'B',
    2 => 'C'
]

@vlakoff
Copy link
Contributor

vlakoff commented Nov 27, 2016

I really don't understand why people think all these imbricated methods are simpler than a plain loop… :trollface:

Please see my revised version on #16552 for supporting multiple keys, as currently.

@fernandobandeira
Copy link
Contributor Author

fernandobandeira commented Nov 27, 2016

Yep, this one looks simpler and lets you return multiple keys, i'll update the PR.

And about the imbricated methods, they look so cool 😄

$this->assertEquals(
[1, 3, 2],
$data->keys()->all()
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertSame should be considered instead, as it does strict comparison thus also compares array orders. See Array Operators.

@vlakoff
Copy link
Contributor

vlakoff commented Nov 27, 2016

If there is an agreement to continue supporting multiple keys (I think it should), an unit test for this definitely should be added to prevent future regression.

@fernandobandeira
Copy link
Contributor Author

fernandobandeira commented Nov 27, 2016

Changed to assertSame and added multiple keys tests accordingly =)

@vlakoff
Copy link
Contributor

vlakoff commented Nov 28, 2016

Another overlook, callback should also receive the key:

        foreach ($this->items as $key => $value) {
            $assoc = $callback($value, $key);

            foreach ($assoc as $mapKey => $mapValue) {
                $result[$mapKey] = $mapValue;
            }
        }

I guess this should also be covered by tests…

@taylorotwell
Copy link
Member

Is this ready to merge now?

@fernandobandeira
Copy link
Contributor Author

Yes, we should've covered all the possible usecases with this...

@jvzanatta
Copy link

Is there any chances this change made the mapWithKeys function stop working with multidimensional return? This worked fine in 5.3 and broke after I upgraded to 5.4

$config = Config::get()->mapWithKeys(function ($item) {

        return [
            [
                $item->index => [
                    $item->sub_index => [
                        $item->parameter => $item->value
                    ]
                ]
            ]
        ];
    }
)->all();

Used to return:

[
  "index1" => [
    "sub_index1" => [
      "parameter1" => 30
      "parameter2" => [
        0 => 100
      ]
    ]
    "sub_index2" => [
      "parameter3" => "15"
    ]
  ],
  [
  "index2" => [
    "sub_index3" => [
      "parameter4" => 14.003185337396
      "parameter5" => "2"
    ]
    "sub_index4" => [
      "parameter6" => "default"
      "parameter7" => "/assets/img/logo.png"
    ]
    "sub_index4" => [
      "parameter8" => "900"
      "parameter9" => "15"
    ]
  ]
]

Now it only returns:

[
  "index2" => [
    "sub_index4" => [
      "parameter9" => "15" // least one in the list is still returned, others are ignored
    ]
  ]
]

@vlakoff
Copy link
Contributor

vlakoff commented Jun 21, 2017

In your code above, the returned associative array always have a 0 key.

I guess you should remove that upper layer:

return [
    $item->index => [
        $item->sub_index => [
            $item->parameter => $item->value
        ]
    ]
];

@jvzanatta
Copy link

@vlakoff Thanks for the reply. I tried but did not had any effect. I'm going to check the file history to see what I can find about it changing it's behavior, just wanted to check if there is a chance this commit has something to do about it.

@vlakoff
Copy link
Contributor

vlakoff commented Jun 22, 2017

I just noticed you were merging the data (e.g. parameter1 and parameter2 in your example). mapWithKeys() was never supposed to do this.

I'm pretty sure your code was relying on some quirks of a previous implementation, so breakage could (and did) happen.

TBH, you would be better using regular PHP functions and loops, rather than these obscure functions. array_merge_recursive() might help you.

@vlakoff
Copy link
Contributor

vlakoff commented Jun 22, 2017

This message by crynobone links to an article that may be of interest to you: array_merge_recursive() vs. array_replace_recursive().

@jvzanatta
Copy link

Yes, I ended up solving it with a regular loop. As you previously stated:

I really don't understand why people think all these imbricated methods are simpler than a plain loop…

Btw, thanks for the references. I'm going to take a look at them out of curiosity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants