wp_list_comments() always assumes walker will echo.

[MikeLittle]

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.

[MikeLittle]

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

[MikeLittle]

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

[MikeLittle]

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]

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

[MikeLittle]

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

[MikeLittle]

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.

[nacin]

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.

[ryan]

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.

[nacin]

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.

[dd32]

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

[wonderboymusic]

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

[MikeLittle]

That patch seems to work OK (as 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.

[wonderboymusic]

Gauging interest in this

[SergeyBiryukov]

Related: #16541

return-wp-list-comments.diff​ seems consistent with ticket:16541:16541.2.diff​.

[MikeLittle]

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.

[DrewAPicture]

Refreshes return-wp-list-comments.diff

[DrewAPicture]

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

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

[wonderboymusic]

Still applies / works

[nacin]

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]

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.

[nacin]

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]

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

[MikeLittle]

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

[nacin]

In 26353:

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

props mikelittle.
fixes #10948.

[needle]

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.

[MikeLittle]

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.

[needle]

@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.

[MikeLittle]

Ah. OK I see what you mean.