Make WordPress Core

Opened 16 years ago

Closed 14 years ago

#2784 closed defect (bug) (fixed)

Make all foreach() loops cast to array

Reported by: markjaquith's profile markjaquith Owned by: jacobsantos's profile jacobsantos
Milestone: 2.7 Priority: normal
Severity: normal Version: 2.1
Component: Optimization Keywords: has-patch needs-testing dev-feedback
Focuses: Cc:

Description

Every so often, we get bitten by a random foreach() error due to the input not being an array.

The elegant solution is this:

foreach ( (array) $maybe_array as $foo ) :

The attached patch is just the result of a giant regular expressions find/replace:

search: foreach( *)\(( *)\$
replace: foreach$1( (array) $

this is more proof of concept than anything else. I'm not getting any errors after applying it. I'd love to banish foreach errors once and for all.

Attachments (6)

cast_to_array.diff (153.1 KB) - added by markjaquith 16 years ago.
foreach_refresh.diff (149.8 KB) - added by markjaquith 16 years ago.
Patch refresh for /trunk/
arraytypecast.patch (43.0 KB) - added by darkdragon 14 years ago.
Corrected version based off of r6563 for adding array type casting to foreach loops in wp-includes folder
2784.diff (39.4 KB) - added by jacobsantos 14 years ago.
type casting for foreach in wp-includes.
2784.r8483.diff (40.4 KB) - added by jacobsantos 14 years ago.
type casting for foreach in wp-includes based off of r8483
2784.r8483.patch (39.5 KB) - added by jacobsantos 14 years ago.
type casting for foreach in wp-includes based off of r8483

Download all attachments as: .zip

Change History (32)

#1 @davidhouse
16 years ago

The _real_ solution is for PHP to change their crappy foreach semantics :) But yeah, nice.

#2 @Nazgul
16 years ago

  • Keywords has-patch added

@markjaquith
16 years ago

Patch refresh for /trunk/

#3 @markjaquith
16 years ago

Patch refreshed for the bug hunt

#4 @markjaquith
16 years ago

  • Component changed from Administration to Optimization
  • Milestone set to 2.1
  • Owner changed from anonymous to markjaquith
  • Status changed from new to assigned

Because there is a good chance this will rot many existing patches, let's wait on this until right before we release a 2.1 beta zip.

#5 @matt
15 years ago

  • Milestone changed from 2.1 to 2.2

#6 @foolswisdom
15 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 2.2 to 2.3

Likely needs refresh

#7 @Nazgul
15 years ago

  • Milestone changed from 2.3 to 2.4 (next)

A candidate for doing very early in the 2.4 cycle.

#8 @foolswisdom
15 years ago

  • Keywords early added

#9 @darkdragon
14 years ago

I don't think bug rot will be an issue for this, since the phpdoc has done that already. Will submit updated patch.

#10 @darkdragon
14 years ago

Patch contains items which would corrupt trunk, please do not commit. Will fix issues and resubmit.

@darkdragon
14 years ago

Corrected version based off of r6563 for adding array type casting to foreach loops in wp-includes folder

#11 follow-up: @darkdragon
14 years ago

  • Keywords has-patch added; needs-patch removed

Updated patch fixes four spaces to tabs issue, however patch does not display in Trac, so I'm worried it won't patch for committing.

#12 in reply to: ↑ 11 @DD32
14 years ago

Replying to darkdragon:

however patch does not display in Trac, so I'm worried it won't patch for committing.

I've found that you need to save as a .diff instead of a .patch.
Simply renaming a .patch to .diff doesnt work either.

D

#13 @darkdragon
14 years ago

Is this ticket being completely ignored or is there hope that one of the patches will be applied?

#14 @darkdragon
14 years ago

If there is something that is missing then let me know and I'll add it.

#15 @darkdragon
14 years ago

What is up with this? Is this ticket defunct? Close it as won't fix if nothing is going to be done.

#16 @santosj
14 years ago

  • Owner changed from markjaquith to jacobsantos
  • Status changed from assigned to new

#17 @santosj
14 years ago

  • Milestone changed from 2.9 to 2.7

Time to update patch. Hopefully, if I'm going to spend another hour on this it will get through faster, than the last time.

#18 @santosj
14 years ago

Patch made, will upload in 1 hour.

@jacobsantos
14 years ago

type casting for foreach in wp-includes.

#19 @jacobsantos
14 years ago

It is discomforting when the diff does not display.

#20 @jacobsantos
14 years ago

  • Keywords needs-testing added; early removed

#21 @DD32
14 years ago

It is discomforting when the diff does not display.

Try removing the extra blank line at the end of the file, I've found that causes it sometimes.. It is anoying though, I wish i could work out what causes it sometimes.

@jacobsantos
14 years ago

type casting for foreach in wp-includes based off of r8483

@jacobsantos
14 years ago

type casting for foreach in wp-includes based off of r8483

#22 @jacobsantos
14 years ago

Well, hopefully one of those works.

#23 @jacobsantos
14 years ago

The only problem I suppose is that this doesn't check wp-admin, which will need to be for this ticket to be closed.

#24 @santosj
14 years ago

  • Keywords dev-feedback added

I would rather this not go another 3 months without being looked at.

#25 @santosj
14 years ago

Suggest closing as wontfix if not going to commit.

#26 @markjaquith
14 years ago

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

(In [8572]) Cast to array when using foreach(). Props santosj (and thanks for your perseverance!). fixes #2784

Note: See TracTickets for help on using tickets.