WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 8 months ago

Last modified 6 months ago

#10948 closed enhancement (fixed)

wp_list_comments() always assumes walker will echo.

Reported by: MikeLittle Owned by: 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 5 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 5 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 5 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 5 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 23 months ago.
10948.diff (860 bytes) - added by DrewAPicture 14 months ago.
Refreshes return-wp-list-comments.diff
10948.2.diff (4.7 KB) - added by MikeLittle 11 months 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 9 months ago.
Updated: Still makes comment walker consistent, but doesn't change any function signatures.
10948-test-2013.diff (542 bytes) - added by MikeLittle 9 months 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)

MikeLittle5 years ago

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

MikeLittle5 years ago

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

comment:1 MikeLittle5 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.

MikeLittle5 years ago

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

MikeLittle5 years ago

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

comment:2 MikeLittle5 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.

comment:3 nacin5 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.

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

comment:5 nacin5 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.

comment:6 dd324 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.

comment:7 shidouhikari4 years ago

  • Cc shidouhikari added

comment:8 nacin4 years ago

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

comment:9 wonderboymusic23 months ago

  • Keywords has-patch added; needs-patch removed

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

comment:10 MikeLittle23 months ago

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.

Last edited 23 months ago by MikeLittle (previous) (diff)

comment:11 wonderboymusic18 months ago

  • Milestone changed from Future Release to 3.6

Gauging interest in this

comment:13 MikeLittle18 months 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 18 months ago by MikeLittle (previous) (diff)

DrewAPicture14 months ago

Refreshes return-wp-list-comments.diff

comment:14 DrewAPicture14 months 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.

comment:15 ryan13 months ago

  • Milestone changed from 3.6 to Future Release

comment:16 wonderboymusic11 months ago

  • Milestone changed from Future Release to 3.7

Still applies / works

comment:17 nacin11 months 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.

MikeLittle11 months 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.

comment:18 nacin9 months 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.

MikeLittle9 months ago

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

MikeLittle9 months ago

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

comment:19 nacin8 months 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.

comment:20 markoheijnen7 months ago

  • Milestone changed from Future Release to 3.8

comment:21 needle7 months 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.

comment:22 MikeLittle7 months 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.

comment:23 needle7 months 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.

comment:24 MikeLittle6 months ago

Ah. OK I see what you mean.

Note: See TracTickets for help on using tickets.