Make WordPress Core

Opened 14 years ago

Closed 10 years ago

Last modified 10 years ago

#10948 closed enhancement (fixed)

wp_list_comments() always assumes walker will echo.

Reported by: mikelittle's profile MikeLittle Owned by: nacin's profile nacin
Milestone: 3.8 Priority: normal
Severity: normal Version: 2.8.6
Component: Comments Keywords: has-patch
Focuses: Cc:

Description

wp_list_comments() always assumes the walker called will echo it's constructed content (the default Walker does this).

However, if you want to use your own walker which doesn't echo, or you want to post process the output of the default walker in some way, you cannot.

The changes required are to wp_list_comments() which should check for an 'echo' arg and respond appropriately (like wp_list_pages does). The built in comment walker class Walker_Comment needs to write it's output to the passed $output reference. Finally the function edit_comment_link() needs to take an echo parameter.

The attached patch allows this functionality whilst preserving the status quo by adding a default echo => 1 argument to wp_list_comments and edit_comment_link.

Attachments (9)

comment-walker-echo-issue-trunk-12022.diff (5.5 KB) - added by MikeLittle 14 years ago.
Patch to fix issue. Diff-ed against trunk ar rev 12022
comment-walker-echo-issue-284.diff (5.5 KB) - added by MikeLittle 14 years ago.
Patch to fix issue in version 2.8.4. Diff-ed against tag 2.8.4
comment-walker-echo-issue-trunk-12304.diff (5.6 KB) - added by MikeLittle 14 years ago.
Patch to fix issue. Diff-ed against trunk at rev 12304
comment-walker-echo-issue-286.diff (5.5 KB) - added by MikeLittle 14 years ago.
Patch to fix issue in version 2.8.6. Diff-ed against tag 2.8.6
return-wp-list-comments.diff (1.3 KB) - added by wonderboymusic 12 years ago.
10948.diff (860 bytes) - added by DrewAPicture 11 years ago.
Refreshes return-wp-list-comments.diff
10948.2.diff (4.7 KB) - added by MikeLittle 11 years ago.
Going with the first option -- make comment walker behave like the others -- (a.k.a my original approach) the attached patch adds use of $output throughout the class. It also wraps the two instances of user callbacks with output buffering, and adds the output parameter and output buffering to the new ping(), comment(), and html5_comment() functions. Autodocs also updated.
10948.3.diff (3.1 KB) - added by MikeLittle 10 years ago.
Updated: Still makes comment walker consistent, but doesn't change any function signatures.
10948-test-2013.diff (542 bytes) - added by MikeLittle 10 years ago.
Simple test: make 2013 echo wp_list_comments() after adding echo => false. Output is xactly the same as without the patch.

Download all attachments as: .zip

Change History (33)

@MikeLittle
14 years ago

Patch to fix issue. Diff-ed against trunk ar rev 12022

@MikeLittle
14 years ago

Patch to fix issue in version 2.8.4. Diff-ed against tag 2.8.4

#1 @MikeLittle
14 years ago

  • Keywords has-patch added; has_patch removed

I should note this has been tested against both 2.8.4 and trunk. The output of various comments displays (default theme, with and without paging) was compared before and after the patch.

Apart from white space, the outputs were identical.

@MikeLittle
14 years ago

Patch to fix issue. Diff-ed against trunk at rev 12304

@MikeLittle
14 years ago

Patch to fix issue in version 2.8.6. Diff-ed against tag 2.8.6

#2 @MikeLittle
14 years ago

  • Cc wordpress@… added
  • Version changed from 2.8.4 to 2.8.6

Added updated patches - against 2.8.6 and against trunk at rev 12304.
It would be great to get this into 2.9 if at all possible.

#3 @nacin
14 years ago

Looks good, applies clean, appears to work correctly. It looks like all filters are applied as before as well.

FWIW, you only need to patch this against trunk. Only security changes are backported to the 2.8 branch.

#4 @ryan
14 years ago

  • Keywords early added
  • Milestone changed from 2.9 to 3.0

We might want to do get_edit_comment_link() instead of an echo flag to match what we do elsewhere. Otherwise, looks good. Unfortunately, we overlooked this ticket in time to get it into 2.9. Let's mark as early 3.0 and get this in as soon was we branch.

#5 @nacin
14 years ago

Now that I look, there is get_edit_comment_link(), which returns the URL only, so they function slightly differently. edit_comment_link() calls get_...(). I can think of a few different solutions but I'm not sure which would be best.

#6 @dd32
14 years ago

  • Milestone changed from 3.0 to 3.1

This has missed the boat for 3.0 IMO, I'm marking as 3.1 early instead.

#7 @shidouhikari
14 years ago

  • Cc shidouhikari added

#8 @nacin
13 years ago

  • Keywords needs-patch added; has-patch early removed
  • Milestone changed from Awaiting Triage to Future Release

#9 @wonderboymusic
12 years ago

  • Keywords has-patch added; needs-patch removed

Added my own patch - it's less intrusive (IMO) to use output buffering

#10 @MikeLittle
12 years ago

That patch seems to work OK (s you would expect), but I thought there were sometimes issues with using output buffering. This may be historical (old PHP on old Windows) and irrelevant.

Version 0, edited 12 years ago by MikeLittle (next)

#11 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.6

Gauging interest in this

#13 @MikeLittle
11 years ago

The return-wp-list-comments.diff​ patch still applies cleanly for me on today's trunk (revision #23296).

The code works fine:
Adding an 'echo' => false parameter to the call to wp_list_comments in comments.php in twentytwelve theme produces no comment output (so the flag works).
Adding an echo to the front of that function call produces exactly the same output as without the patch.

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

@DrewAPicture
11 years ago

Refreshes return-wp-list-comments.diff

#14 @DrewAPicture
11 years ago

10948.diff refreshes return-wp-list-comments.diff following [23694].

wp_list_comments() continues to work for me as expected with 10948.diff applied.

#15 @ryan
11 years ago

  • Milestone changed from 3.6 to Future Release

#16 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.7

Still applies / works

#17 @nacin
11 years ago

Every other walker in core sets $output instead of directly echoing. I feel like Walker_Comment is doing it wrong and should be updated to do it properly. We would need to add an output buffer around the callback trigger in start_el() and such.

The only other issue is if someone is extending Walker_Comment, especially only some of it. I am not sure what would happen then.

So, do we:

  • Try to make the walker work more like all other walkers.
  • Add output buffering to where the walker is called.
  • ??

I'm fine with committing 10948.diff but it would still be nice to look at the walker inconsistencies.

@MikeLittle
11 years ago

Going with the first option -- make comment walker behave like the others -- (a.k.a my original approach) the attached patch adds use of $output throughout the class. It also wraps the two instances of user callbacks with output buffering, and adds the output parameter and output buffering to the new ping(), comment(), and html5_comment() functions. Autodocs also updated.

#18 @nacin
10 years ago

  • Milestone changed from 3.7 to Future Release
  • Type changed from defect (bug) to enhancement

This looks pretty good.

Rather than changing the signature of protected methods, what if we just moved the output buffering to outside of and around those method calls? Seems like it would be pretty clean and would prevent the need to touch those methods, which conveniently mirror existing custom callbacks otherwise.

Only other thing, it appears there are some tabs-versus-spaces issues.

Punting because you can always use an output buffer as a workaround for this, so this is low priority.

@MikeLittle
10 years ago

Updated: Still makes comment walker consistent, but doesn't change any function signatures.

@MikeLittle
10 years ago

Simple test: make 2013 echo wp_list_comments() after adding echo => false. Output is xactly the same as without the patch.

#19 @nacin
10 years ago

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

In 26353:

Add echo arguments to wp_list_comments() and Walker_Comment, allowing non-echo usage.

props mikelittle.
fixes #10948.

#20 @markoheijnen
10 years ago

  • Milestone changed from Future Release to 3.8

#21 @needle
10 years ago

Thanks for the commit on this long-standing issue. This is just a note to say that comment output of any theme using a custom walker may now appear unexpectedly broken. It might be worth some kind of announcement on make (or elsewhere) to highlight this change, because the effects are subtle yet important.

#22 @MikeLittle
10 years ago

There shouldn't be any breakage as by default the behaviour does not change. I can't think of a situation where a custom walker's behaviour would be modified by this change.
Please provide an example if you can and I'll look to see if any issues can be mitigated.

#23 @needle
10 years ago

@MikeLittle when the walker was first introduced, it didn't allow for wrapping comments in ordered lists, just unordered ones. So, way back when, I implemented a custom walker to generate output using <ol> tags by overriding start_lvl() and end_lvl() and echoing ordered list markup. I realise that the walker has supported <ol> for a long time now, but as with many projects, because nothing broke when that was introduced, it got overlooked and my custom walker remained in place.

But since start_lvl() and end_lvl() now (properly) use $output instead of echoing, I only just noticed that I was getting spurious ordered lists being written into the markup by my custom walker's methods. But because this only happens when there are nested comments, it was not immediately obvious.

Having said all of that, I suspect that my theme is probably one of the few to do this (or have done this). I just wanted to highlight that any custom walker that overrides start_lvl() and/or end_lvl() will be affected.

#24 @MikeLittle
10 years ago

Ah. OK I see what you mean.

Note: See TracTickets for help on using tickets.