Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ParisLiakos’s picture

Component: aggregator.module » base system

yeah sure..probably base system

ParisLiakos’s picture

Status: Active » Needs review
FileSize
2.9 KB

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
FileSize
5.63 KB

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

FileSize
5.62 KB
+++ 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
FileSize
5.86 KB

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
FileSize
5.83 KB
  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
FileSize
667 bytes
5.81 KB

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

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

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

Yep, How about a few docs tweaks like this?

dawehner’s picture

FileSize
419 bytes
5.87 KB

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
FileSize
5.88 KB

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.