Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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!