The docblock suggests that either Iterator
or IteratorArray
ought to be implemented. First, there is no IteratorArray
- there's ArrayIterator
but (second), it's a class, not an interface, so it would have to be extended, not implemented. I've updated the docblock to reflect this, and provided a second example accordingly.
I don't actually KNOW if it's true that it'd be fine to extend ArrayIterator
, but if it is, then the docblock should be spot-on.
Comment | File | Size | Author |
---|---|---|---|
#19 | DBStatement_docblock_341038-19.patch | 1.37 KB | kathyh |
#12 | 341038-12.patch | 1.5 KB | jhodgdon |
#9 | 341038.patch | 1.5 KB | jhodgdon |
DatabaseStatement_docblock.patch | 1.27 KB | sdboyer | |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedGood catch: IteratorArray should be IteratorAggregate (that was a typo), but this is the only required change.
Comment #2
sdboyer CreditAttribution: sdboyer commentedYeah, given our discussion over in #341031: DatabaseStatementInterface extends Traversable for no reason, that's now quite clearly a typo :)
Comment #3
jhodgdonThis is quite an old issue... Is it still relevant, is the patch correct?
Comment #4
jhodgdonDatabaseStatement_docblock.patch queued for re-testing.
Comment #5
jhodgdonComment #6
jhodgdonThis needs to be postponed until standards for documenting classes and interfaces are agreed upon.
#718596: Lacking standards for documenting classes, interfaces, methods
Comment #7
jhodgdonDoc standards are now defined:
http://drupal.org/node/1354#classes
Comment #8
jhodgdonbump.
Any thoughts on relevance or correctness of this patch -- I guess at a minimum it needs a reroll and a fix as in comment #1 above, but is it still necessary and/or a good idea?
Comment #9
jhodgdonHere's a new patch, which cleans up the DatabaseStatementInterface doc, and makes the change as in #1 above.
Comment #10
jhodgdon#9: 341038.patch queued for re-testing.
Comment #12
jhodgdonHere's a reroll.
Comment #13
jhodgdon#12: 341038-12.patch queued for re-testing.
Comment #14
jhodgdonupdating, will click retest in a sec...
Comment #15
jhodgdon#12: 341038-12.patch queued for re-testing.
Comment #17
jhodgdonLooks like this needs a reroll. Probably a good novice project?
Comment #18
kathyh CreditAttribution: kathyh commentedComment #19
kathyh CreditAttribution: kathyh commentedRe-roll to clean up the DatabaseStatementInterface doc and correct to use IteratorAggregate
Comment #20
jhodgdonThis looks OK to me, and appears to satisfy the concerns raised by Damien above, as well as the original post. Thanks!
Comment #21
Dries CreditAttribution: Dries commentedCommitted to 7.x and 8.x. Thanks!