WordPress.org

Make WordPress Core

Opened 9 years ago

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

Download all attachments as: .zip

Change History (32)

@markjaquith9 years ago

comment:1 @davidhouse9 years ago

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

comment:2 @Nazgul9 years ago

  • Keywords has-patch added

@markjaquith9 years ago

Patch refresh for /trunk/

comment:3 @markjaquith9 years ago

Patch refreshed for the bug hunt

comment:4 @markjaquith9 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.

comment:5 @matt8 years ago

  • Milestone changed from 2.1 to 2.2

comment:6 @foolswisdom8 years ago

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

Likely needs refresh

comment:7 @Nazgul8 years ago

  • Milestone changed from 2.3 to 2.4 (next)

A candidate for doing very early in the 2.4 cycle.

comment:8 @foolswisdom8 years ago

  • Keywords early added

comment:9 @darkdragon7 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.

comment:10 @darkdragon7 years ago

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

@darkdragon7 years ago

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

comment:11 follow-up: @darkdragon7 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.

comment:12 in reply to: ↑ 11 @DD327 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

comment:13 @darkdragon7 years ago

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

comment:14 @darkdragon7 years ago

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

comment:15 @darkdragon7 years ago

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

comment:16 @santosj7 years ago

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

comment:17 @santosj7 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.

comment:18 @santosj7 years ago

Patch made, will upload in 1 hour.

@jacobsantos7 years ago

type casting for foreach in wp-includes.

comment:19 @jacobsantos7 years ago

It is discomforting when the diff does not display.

comment:20 @jacobsantos7 years ago

  • Keywords needs-testing added; early removed

comment:21 @DD327 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.

@jacobsantos7 years ago

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

@jacobsantos7 years ago

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

comment:22 @jacobsantos7 years ago

Well, hopefully one of those works.

comment:23 @jacobsantos7 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.

comment:24 @santosj7 years ago

  • Keywords dev-feedback added

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

comment:25 @santosj7 years ago

Suggest closing as wontfix if not going to commit.

comment:26 @markjaquith7 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.