Make WordPress Core

Opened 3 years ago

Last modified 5 days ago

#22300 assigned enhancement

enhance urlencode_deep()

Reported by: wpmuguru Owned by: chriscct7
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.4
Component: Formatting Keywords: has-patch needs-unit-tests
Focuses: Cc:


It would be handy if urlencode_deep() handled objects as well as arrays. Patch includes corresponding urldecode_deep()

Attachments (7)

22300.diff (2.5 KB) - added by wpmuguru 3 years ago.
22300.2.diff (2.5 KB) - added by wpmuguru 3 years ago.
remove the unneeded {/} in 22300.diff
map_deep.diff (3.5 KB) - added by nbachiyski 3 years ago.
22300-tests.diff (2.6 KB) - added by boonebgorges 9 months ago.
Unit tests, previously in the repo but removed as per #30284.
22300.3.diff (3.5 KB) - added by MikeHansenMe 2 weeks ago.
22300.4.patch (3.6 KB) - added by chriscct7 6 days ago.
22300.test1.diff (1.3 KB) - added by realloc 5 days ago.
Unit test for urlencode_deep

Download all attachments as: .zip

Change History (26)

@wpmuguru3 years ago

comment:1 @scribu3 years ago

You don't really need the squigly braces there: $value->$property.

@wpmuguru3 years ago

remove the unneeded {/} in 22300.diff

comment:2 @wpmuguru3 years ago

Can someone remove the extra patch file? The .2 and .2.2 are the same file.

comment:3 @nbachiyski3 years ago

Thanks for the patch, wpmuguru!

The two functions look really similar to me. I think we should have a common map_deep() and these two will just return map_deep( 'urldecode', $value) and map_deep( 'rawurlencode', $value ).

comment:4 @nacin3 years ago

Something like map_deep(), while it could have a few other use cases in core, could really slow down the deep mapping it is trying to do.

comment:5 @nbachiyski3 years ago

Actually, in my testing map_deep() is around twice faster for me for non-literal string values. I have no explanation.

Here is a very lame and crude testing script:


I tried with op-code cache on and off, also I tried swapping the order of running the urldecode_deep/map_deep, I also tried using different functions, not only urldecode().

There is a chance I did some stupid mistake in the script, so I'd love a second look.

comment:6 @scribu3 years ago

I get PHP Warning: urldecode() expects parameter 1 to be string, array given in /home/scribu/Desktop/bench.php on line 19 when running bench-map-deep.php

comment:7 @nbachiyski3 years ago

This wasn't affecting how the script worked. I fixed it, though.

comment:8 @wpmuguru3 years ago

I reversed the order in which the loops are done (rawurldecode_deep first) & the loop executed first is still about 3 times faster than the second loop.

I do think the map_deep function does have merit though as WP has a few other *_deep functions. map_deep would be something plugin & theme devs would be able to use.

comment:9 @nbachiyski3 years ago

It turned out that the idea of running them sequentially wasn't so good. I tested some more and the final result was that there is almost zero difference. Which means we should go ahead with the map_deep().

Here is a patch for the introduction of map_deep() and converting all older *_deep() functions to using it.

All the old tests pass and here are some for map_deep().

@nbachiyski3 years ago

comment:10 @scribu3 years ago

  • Milestone changed from Awaiting Review to Future Release

+1 for map_deep().

comment:11 @wonderboymusic3 years ago

Every time I see this, I think is says "mad deep"

comment:12 @nbachiyski3 years ago

The path from map_deep() to "mad deep", to "truly, madly, deeply" isn't very long.

Last edited 3 years ago by nbachiyski (previous) (diff)

comment:13 @wpmuguru3 years ago

map_deep.diff looks good.

comment:14 @SergeyBiryukov3 years ago

  • Version changed from trunk to 3.4

comment:15 @jdgrimes2 years ago

+1 for map_deep(). Patch is still applying cleanly.

@boonebgorges9 months ago

Unit tests, previously in the repo but removed as per #30284.

comment:16 @chriscct75 weeks ago

  • Keywords needs-refresh added
  • Milestone changed from Future Release to 4.4
  • Owner set to chriscct7
  • Status changed from new to assigned

@MikeHansenMe2 weeks ago

comment:17 @MikeHansenMe2 weeks ago

  • Keywords needs-refresh removed

22300.3.diff is a refresh and passes the unit tests in 22300-tests.diff.

@chriscct76 days ago

comment:18 @chriscct76 days ago

  • Keywords commit added

Added missing since in the docbloc on map_deep. Otherwise looks good

comment:19 @johnbillion5 days ago

  • Component changed from General to Formatting
  • Keywords needs-unit-tests added; commit removed

map_deep() looks handy.

Let's get some unit tests on this (for example, by testing urlencode_deep() which doesn't currently have any test coverage).

@realloc5 days ago

Unit test for urlencode_deep

Note: See TracTickets for help on using tickets.