Make WordPress Core

Opened 11 years ago

Closed 8 years ago

Last modified 8 years ago

#22300 closed enhancement (fixed)

enhance urlencode_deep()

Reported by: wpmuguru's profile wpmuguru Owned by: johnbillion's profile johnbillion
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.4
Component: Formatting Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

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

Attachments (8)

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

Download all attachments as: .zip

Change History (32)

@wpmuguru
11 years ago

#1 @scribu
11 years ago

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

@wpmuguru
11 years ago

remove the unneeded {/} in 22300.diff

#2 @wpmuguru
11 years ago

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

#3 @nbachiyski
11 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 ).

#4 @nacin
11 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.

#5 @nbachiyski
11 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:

https://gist.github.com/ac7c4d5f783880917758

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.

#6 @scribu
11 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

#7 @nbachiyski
11 years ago

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

#8 @wpmuguru
11 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.

#9 @nbachiyski
11 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().

@nbachiyski
11 years ago

#10 @scribu
11 years ago

  • Milestone changed from Awaiting Review to Future Release

+1 for map_deep().

#11 @wonderboymusic
11 years ago

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

#12 @nbachiyski
11 years ago

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

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

#13 @wpmuguru
11 years ago

map_deep.diff looks good.

#14 @SergeyBiryukov
11 years ago

  • Version changed from trunk to 3.4

#15 @jdgrimes
11 years ago

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

@boonebgorges
9 years ago

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

#16 @chriscct7
9 years ago

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

@MikeHansenMe
8 years ago

#17 @MikeHansenMe
8 years ago

  • Keywords needs-refresh removed

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

@chriscct7
8 years ago

#18 @chriscct7
8 years ago

  • Keywords commit added

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

#19 @johnbillion
8 years 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).

@realloc
8 years ago

Unit test for urlencode_deep

#20 @DrewAPicture
8 years ago

  • Owner changed from chriscct7 to johnbillion

@johnbillion see 22300.test1.diff for proposed tests

@johnbillion
8 years ago

#21 @johnbillion
8 years ago

  • Keywords has-unit-tests commit added; needs-unit-tests removed

22300.4.diff changes the order of the parameters in map_deep() so they match array functions in PHP such as array_walk_recursive(). It also implements a tidied up version of the original tests for map_deep() from 22300-tests.diff in addition to the tests from 22300.test1.diff, and improves docs.

This is good to go.

#22 @johnbillion
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 35252:

Introduce map_deep(), a utility function that recursively maps a callable function to every item in an array or object. Works like array_walk_recursive() but works with objects too.

Updates rawurlencode_deep(), urlencode_deep(), and stripslashes_deep() to use map_deep(). Introduces urldecode_deep() for completeness.

Props wpmuguru, nbachiyski, boonebgorges, MikeHansenMe, chriscct7, realloc, johnbillion
Fixes #22300

#23 @dd32
8 years ago

In 36100:

Allow map_deep() to work with object properties containing a reference. Restores the previous behaviour of stripslashes_deep().

Props jeff@…, swissspidy.
See #22300, [35252].
Fixes #35058.

#24 @dd32
8 years ago

In 36101:

Allow map_deep() to work with object properties containing a reference. Restores the previous behaviour of stripslashes_deep().

Merges [36100] to the 4.4 branch.
Props jeff@…, swissspidy.
See #22300, [35252].
Fixes #35058.

Note: See TracTickets for help on using tickets.