WordPress.org

Make WordPress Core

Opened 11 years ago

Closed 8 years ago

#2784 closed defect (bug) (fixed)

Make all foreach() loops cast to array

Reported by: markjaquith Owned by: 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 11 years ago.
foreach_refresh.diff (149.8 KB) - added by markjaquith 10 years ago.
Patch refresh for /trunk/
arraytypecast.patch (43.0 KB) - added by darkdragon 9 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 8 years ago.
type casting for foreach in wp-includes.
2784.r8483.diff (40.4 KB) - added by jacobsantos 8 years ago.
type casting for foreach in wp-includes based off of r8483
2784.r8483.patch (39.5 KB) - added by jacobsantos 8 years ago.
type casting for foreach in wp-includes based off of r8483

Download all attachments as: .zip

Change History (32)

#1 @davidhouse
11 years ago

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

#2 @Nazgul
10 years ago

  • Keywords has-patch added

@markjaquith
10 years ago

Patch refresh for /trunk/

#3 @markjaquith
10 years ago

Patch refreshed for the bug hunt

#4 @markjaquith
10 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
10 years ago

  • Milestone changed from 2.1 to 2.2

#6 @foolswisdom
10 years ago

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

Likely needs refresh

#7 @Nazgul
9 years ago

  • Milestone changed from 2.3 to 2.4 (next)

A candidate for doing very early in the 2.4 cycle.

#8 @foolswisdom
9 years ago

  • Keywords early added

#9 @darkdragon
9 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
9 years ago

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

@darkdragon
9 years ago

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

#11 follow-up: @darkdragon
9 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
9 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
9 years ago

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

#14 @darkdragon
9 years ago

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

#15 @darkdragon
9 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
8 years ago

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

#17 @santosj
8 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
8 years ago

Patch made, will upload in 1 hour.

@jacobsantos
8 years ago

type casting for foreach in wp-includes.

#19 @jacobsantos
8 years ago

It is discomforting when the diff does not display.

#20 @jacobsantos
8 years ago

  • Keywords needs-testing added; early removed

#21 @DD32
8 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
8 years ago

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

@jacobsantos
8 years ago

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

#22 @jacobsantos
8 years ago

Well, hopefully one of those works.

#23 @jacobsantos
8 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
8 years ago

  • Keywords dev-feedback added

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

#25 @santosj
8 years ago

Suggest closing as wontfix if not going to commit.

#26 @markjaquith
8 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.