Objective

  1. \Drupal\Component\Utility\MapArray exists to work around a PHP bug with array_combine().
  2. https://bugs.php.net/bug.php?id=34857 was fixed in PHP 5.4.0.
  3. MapArray can also be used as a shortcut to combine array_combine() and array_map(), although that feature is very rarely used (if at all) in core.

Proposed resolution

  1. Remove \Drupal\Component\Utility\MapArray and replace all usages with array_combine(), and potentially array_map().

API changes

From HEAD: MapArray has been removed.
From D7: drupal_map_assoc() has been removed.

CommentFileSizeAuthor
#10 interdiff.txt9.54 KBParisLiakos
#10 2092095-10-remove-MapArray.patch62.11 KBParisLiakos
#6 2092095-6-remove-MapArray.patch61.05 KBtstoeckler
#6 2092095-4-6-interdiff.txt781 byteststoeckler
#4 2092095-4-remove-MapArray.patch56.74 KBtstoeckler

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue summary: View changes
Issue tags: +PHP 5.4, +API clean-up

Correction:

https://bugs.php.net/bug.php?id=34857 was fixed around the time of PHP 5.1, but is only available since PHP 5.4:

http://php.net/manual/en/function.array-combine.php#refsect1-function.ar...

5.4.0 Previous versions issued E_WARNING and returned FALSE for empty arrays.

So once #2152073: Bump Drupal core's PHP requirement to 5.4.2 is possible and done, we can eliminate MapArray entirely.

The optional $callback support is completely obsolete since PHP 5.3 already, because that's the same as:

$result = array_map(function ($value) {
  return $value * 2;
}, array_combine($list, $list));
ParisLiakos’s picture

ParisLiakos commented
Related issues: +#2207021: Remove legacy PHP 5.3 code from PhpMail plugin
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Hmm... interesting that's very weird documentation then on the PHP site, because that clearly says "PHP Version: 5.1.0RC1"

Anyway, since I'm assigned to this anyway, taking a stab at this.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Assigned: tstoeckler » Unassigned
Status: Active » Needs review
StatusFileSize
new 2092095-4-remove-MapArray.patch56.74 KB

Here we go.

Unassigning for now, as this is not exactly very high on my hitlist right now.

Status: Needs review » Needs work

The last submitted patch, 4: 2092095-4-remove-MapArray.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
StatusFileSize
new 2092095-4-6-interdiff.txt781 bytes
new 2092095-6-remove-MapArray.patch61.05 KB

Oops.

Status: Needs review » Needs work

The last submitted patch, 6: 2092095-6-remove-MapArray.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: +PHP BC cleanup
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: +@deprecated

Actually, this removes the @deprecated function, too. :)

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Needs review
StatusFileSize
new 2092095-10-remove-MapArray.patch62.11 KB
new interdiff.txt9.54 KB

wrong order of arguments:)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Oh, wow, that was quite stupid. Thanks @ParisLiakos!!

ParisLiakos’s picture

ParisLiakos commented

No, worries.. messing up order of arguments in PHP is a common fail..php--

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Wow, this looks awesome - great job!

I guess we need to prepare a draft change notice for the API change of removing drupal_map_assoc(), describing how to achieve the same with native PHP (as demonstrated in this patch).

ParisLiakos’s picture

ParisLiakos commented

draft change record: https://drupal.org/node/2207453

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Change record looks great. Added some links to the PHP functions and an example of the Drupal 8 MapArray version.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

That PHP bug isn't the reason we have MapArray, this is. Let's see if that's still valid first and whether we care or not.

https://drupal.org/comment/913381#comment-913381

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Fixed

Ignore me, I had another issue that got reverted, but the one linked above got in. So yes indeed that's the only reason, and we're on 5.4 now.

Committed/pushed to 8.x, thanks! Nice to see us finally get rid of this after trying six years ago.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented

Fyi: drupal_map_assoc is used by drush currently, so if you'll have some drush problems with current d8 HEAD until its fixed.

Status: Fixed » Closed (fixed)

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