Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
And while we are there change the name to something more descriptive.
Array::copyValuesToKeys()
anyone
Comment | File | Size | Author |
---|---|---|---|
#31 | 1973312-31.patch | 5.88 KB | damiankloip |
#26 | drupal-1973312-26.patch | 5.87 KB | dawehner |
#26 | interdiff.txt | 419 bytes | dawehner |
#25 | 1973312-25.patch | 5.87 KB | damiankloip |
#25 | interdiff-1973312-25.txt | 1.95 KB | damiankloip |
Comments
Comment #1
ParisLiakos CreditAttribution: ParisLiakos commentedyeah sure..probably base system
Comment #2
ParisLiakos CreditAttribution: ParisLiakos commentedi 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
Comment #4
ParisLiakos CreditAttribution: ParisLiakos commentedyeah, obviously Array is not a valid class name:P
we need another one...ideas?
Comment #5
msonnabaum CreditAttribution: msonnabaum commentedJust 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.
Comment #6
RobLoachI suggest
Map
, since it does have to do with array mapping. I also suggest adding in some PHPUnit tests, as attached.Comment #7
RobLoachFour spaces instead of two.
Comment #8
ParisLiakos CreditAttribution: ParisLiakos commentedMap sounds too generic..MapArray is maybe better
Comment #9
RobLoach^^... or ArrayMap? Either way, a re-roll is called for.
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentedwe have right now:
DifArray
NestedArray
i only put Map as prefixto be consistent with those two classes
Comment #11
ParisLiakos CreditAttribution: ParisLiakos commentedhere is a reroll for MapArray
Comment #12
RobLoachComment #13
alexpottVery nice to be unit testing this function finally :)
As for the unicode patch... let's not do this.
Not 100% certain about this as a name as it only covers the default case and not case when you pass a function.
Comment #14
ParisLiakos CreditAttribution: ParisLiakos commentedabout 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?:)
Comment #15
RobLoachComment #16
RobLoach$function
to$callback
so that it makes a bit more sense for what it does.Comment #17
alexpottThe 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
Comment #18
ParisLiakos CreditAttribution: ParisLiakos commentedyeap indeed its not needed:) we also have tests that prove it:
i dont get any warning or notice when running them, when dataprovider doesnt not specify callback
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedclosed as dupe
#2004798: Replace drupal_map_assoc with a utility class method
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedComment #21
damiankloip CreditAttribution: damiankloip commentedRerolled 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...
Comment #23
damiankloip CreditAttribution: damiankloip commented#21: 1973312-21.patch queued for re-testing.
Comment #24
dawehnerjust a single nitpick: should be int + provide an @return
Comment #25
damiankloip CreditAttribution: damiankloip commentedYep, How about a few docs tweaks like this?
Comment #26
dawehnerAnother nitpick.
Comment #27
dawehnerEhem I wanted to RTBC it.
Comment #29
damiankloip CreditAttribution: damiankloip commented#26: drupal-1973312-26.patch queued for re-testing.
Comment #31
damiankloip CreditAttribution: damiankloip commentedRerolled.
Comment #32
RobLoachComment #33
alexpottCommitted 1649489 and pushed to 8.x. Thanks!