And while we are there change the name to something more descriptive.

Array::copyValuesToKeys() anyone

Files: 
CommentFileSizeAuthor
#31 1973312-31.patch5.88 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 56,103 pass(es).
[ View ]
#26 drupal-1973312-26.patch5.87 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1973312-26.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#26 interdiff.txt419 bytesdawehner
#25 1973312-25.patch5.87 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,382 pass(es).
[ View ]
#25 interdiff-1973312-25.txt1.95 KBdamiankloip
#21 1973312-21.patch5.79 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 56,098 pass(es).
[ View ]
#21 interdiff-1973312-21.txt2.79 KBdamiankloip
#18 drupal-MapArray-1973312-18.patch5.81 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 56,665 pass(es).
[ View ]
#18 interdiff.txt667 bytesParisLiakos
#16 1973312.patch5.83 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 55,723 pass(es).
[ View ]
#11 drupal-MapArray-1973312-11.patch5.86 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 55,389 pass(es).
[ View ]
#7 1973312.patch5.62 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 54,570 pass(es).
[ View ]
#6 1973312.patch5.63 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 54,538 pass(es).
[ View ]
#2 drupal-array_component-1973312-1.patch2.9 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/common.inc.
[ View ]

Comments

ParisLiakos’s picture

Component:aggregator.module» base system

yeah sure..probably base system

ParisLiakos’s picture

Status:Active» Needs review
StatusFileSize
new2.9 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/common.inc.
[ View ]

i think we should get rid of the function too..
i thought about adding a unit test, but does it make sense? it would feel like i am testing array_combine and array_map

Status:Needs review» Needs work

The last submitted patch, drupal-array_component-1973312-1.patch, failed testing.

ParisLiakos’s picture

yeah, obviously Array is not a valid class name:P
we need another one...ideas?

msonnabaum’s picture

Just wanted to add my +1 to copyValuesToKeys. The name of this function never made any sense to me.

No idea what to do about the class name.

RobLoach’s picture

Title:Move drupal_map_assoc to an Array Utility Component» Move drupal_map_assoc to a Map Utility Component
Status:Needs work» Needs review
StatusFileSize
new5.63 KB
PASSED: [[SimpleTest]]: [MySQL] 54,538 pass(es).
[ View ]

I suggest Map, since it does have to do with array mapping. I also suggest adding in some PHPUnit tests, as attached.

RobLoach’s picture

StatusFileSize
new5.62 KB
PASSED: [[SimpleTest]]: [MySQL] 54,570 pass(es).
[ View ]
+++ b/core/tests/Drupal/Tests/Component/Utility/MapTest.phpundefined
@@ -0,0 +1,102 @@
+      array(1, 2, 3, 4, 5),
+      array(
+          1 => 2,
+          2 => 4,
+          3 => 6,
+          4 => 8,
+          5 => 10,

Four spaces instead of two.

ParisLiakos’s picture

Map sounds too generic..MapArray is maybe better

RobLoach’s picture

Status:Needs review» Needs work

^^... or ArrayMap? Either way, a re-roll is called for.

ParisLiakos’s picture

we have right now:

DifArray
NestedArray

i only put Map as prefixto be consistent with those two classes

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new5.86 KB
PASSED: [[SimpleTest]]: [MySQL] 55,389 pass(es).
[ View ]

here is a reroll for MapArray

RobLoach’s picture

Status:Needs review» Reviewed & tested by the community
alexpott’s picture

Status:Reviewed & tested by the community» Needs work

Very nice to be unit testing this function finally :)

+++ b/core/includes/common.incundefined
@@ -2004,28 +2005,13 @@ function drupal_exit($destination = NULL) {
+ * @deprecated as of Drupal 8.0.
+ *   Use \Drupal\Component\Utility\MapArray::copyValuesToKeys() instead.

As for the unicode patch... let's not do this.

+++ b/core/includes/common.incundefined
@@ -2004,28 +2005,13 @@ function drupal_exit($destination = NULL) {
+  return MapArray::copyValuesToKeys($array, $function);

Not 100% certain about this as a name as it only covers the default case and not case when you pass a function.

ParisLiakos’s picture

about the name: i had the same thought..you cant imagine that this method accepts a callable until you see the documentation:/
definitely a lot better than drupal_map_assoc though.
ideas?:)

RobLoach’s picture

RobLoach’s picture

Status:Needs work» Needs review
StatusFileSize
new5.83 KB
PASSED: [[SimpleTest]]: [MySQL] 55,723 pass(es).
[ View ]
  1. Renamed $function to $callback so that it makes a bit more sense for what it does.
  2. Removed the @deprecated key
alexpott’s picture

Status:Needs review» Needs work
+++ b/core/lib/Drupal/Component/Utility/MapArray.phpundefined
@@ -0,0 +1,42 @@
+    if (isset($callback) && is_callable($callback)) {

The isset is unneeded here... calling is_callable(NULL) does not produce a warning for me and I have error_reporting set to E_ALL | E_STRICT

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new667 bytes
new5.81 KB
PASSED: [[SimpleTest]]: [MySQL] 56,665 pass(es).
[ View ]

yeap indeed its not needed:) we also have tests that prove it:

<?php
public function testCopyValuesToKey($input, $expected, $function = NULL) {
   
$output = MapArray::copyValuesToKeys($input, $function);
?>

i dont get any warning or notice when running them, when dataprovider doesnt not specify callback

ParisLiakos’s picture

ParisLiakos’s picture

Title:Move drupal_map_assoc to a Map Utility Component» Move drupal_map_assoc to a MapArray Utility Component
damiankloip’s picture

StatusFileSize
new2.79 KB
new5.79 KB
PASSED: [[SimpleTest]]: [MySQL] 56,098 pass(es).
[ View ]

Rerolled and made a couple of changes. The tests look good, I haven't used the data providers stuff before, that's a nice idea!

- Changed params to $callable instead of $callback
- Fixed test (Needed static method instead)
- Remove ternary from function (from closed dupe in #19), there is no need for this, it will also make a cope of the array in memory.

It's also a good idea to check the dupe issues you are closing, there could be some changes etc... that could be incorporated. Just closing and doing nothing isn;t a great help...

Status:Needs review» Needs work
Issue tags:-PHPUnit Blocker

The last submitted patch, 1973312-21.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
Issue tags:+PHPUnit Blocker

#21: 1973312-21.patch queued for re-testing.

dawehner’s picture

+++ b/core/tests/Drupal/Tests/Component/Utility/MapArrayTest.phpundefined
@@ -95,7 +95,7 @@ public function providerCopyValuesToKey() {
    * @param $n

just a single nitpick: should be int + provide an @return

damiankloip’s picture

StatusFileSize
new1.95 KB
new5.87 KB
PASSED: [[SimpleTest]]: [MySQL] 55,382 pass(es).
[ View ]

Yep, How about a few docs tweaks like this?

dawehner’s picture

StatusFileSize
new419 bytes
new5.87 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1973312-26.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Another nitpick.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Ehem I wanted to RTBC it.

Status:Reviewed & tested by the community» Needs work
Issue tags:-PHPUnit Blocker

The last submitted patch, drupal-1973312-26.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review

#26: drupal-1973312-26.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+PHPUnit Blocker

The last submitted patch, drupal-1973312-26.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new5.88 KB
PASSED: [[SimpleTest]]: [MySQL] 56,103 pass(es).
[ View ]

Rerolled.

RobLoach’s picture

Status:Needs review» Reviewed & tested by the community
alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 1649489 and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.