Make WordPress Core

Opened 13 years ago

Closed 10 years ago

#18002 closed defect (bug) (fixed)

Entering page number for spam comments takes you to page 1

Reported by: indigojo's profile IndigoJo Owned by:
Milestone: 3.3 Priority: normal
Severity: normal Version: 3.2
Component: Administration Keywords: dev-feedback has-patch 2nd-opinion
Focuses: Cc:

Description

When browsing through spam comments, I attempt to view pages in the middle of the set (e.g. page 9 of 13) using the number-entry feature (at the top right) introduced in WP 3.1. Unfortunately, in WP 3.2 this feature has been broken, and when I press Enter, it substitutes '1' for the page number I enter, and then takes me to page 1. I have tried this when browsing posts or normal comments, and it doesn't happen; it only happens when viewing spam comments. I have tried it on Chrome in Linux and Safari on Mac OS X and the same thing happens.

Attachments (1)

18002.diff (675 bytes) - added by MattyRob 13 years ago.

Download all attachments as: .zip

Change History (30)

#1 @nacin
13 years ago

  • Milestone changed from Awaiting Review to 3.2.1

Unconfirmed but moving to 3.2.1 for review.

#2 @kawauso
13 years ago

Confirmed on trunk. It appears to occur any time that the paged parameter is present for any comment type.

Version 0, edited 13 years ago by kawauso (next)

#3 @kawauso
13 years ago

  • Keywords needs-patch added

#4 @MattyRob
13 years ago

Confirmed on 3.2 also.

Perhaps related to the multiple uses of 'paged' as a form parameter. It's the name give in the URL, it's also a form value where you can type in the page number on the comments page and it's also a hidden value on the comments page. I suspect a conflict in the names and when there is a $_GET URL that is being used over user input.

I've double checked on the Posts pages and there is no issue there, there is also no hidden paged parameter. That said removing the hidden parameter from Comments does not resolve this issue. I'll keep looking!

#5 @MattyRob
13 years ago

  • Keywords dev-feedback added

I think I may need to correct myself. It would appear that removing the hidden paged parameter from the comments page resolves this issue, but I'm not sure why that entry is in the form in the first place so removing it may have consequences.

I'll attach a patch later today if I get time.

#6 @MattyRob
13 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

I've removed the hidden input tag from the edit-comments.php file in wp-admin and in my brief testing it seems to work. Patch attached, can someone independently verify please. Also, still needs dev-feedback on why that hidden input tag is there - I don't want to fix this bug and break some other feature.

@MattyRob
13 years ago

#7 @dllh
13 years ago

  • Cc daryl@… added

I can confirm that your patch resolves the issue, but it is puzzling that the field is there to begin with. Maybe it's an artifact from before new pagination bits were added?

#8 @ryan
13 years ago

That code came in on [15491], the initial list table commit.

#9 @nacin
13 years ago

I don't see why this code is here.

It looks like [18237] turned this from an artifact into a bug.

#10 @nacin
13 years ago

In 3.0, if you go to page X, then filter, you still end up on that page.

The code (the variable then was non-standardized, apage) seemed like a bug.

#11 @nacin
13 years ago

Tracked it down to #7412 and [8459]. Looks like it was intended to fix something. How relevant is this still?

#12 @azaozz
13 years ago

Don't remember the exact case but seems it fixes the bulk-actions returning you to page 1 after use.

[18237] is a hacky way to fix #17685, presumed it's temporary until we look again in list tables.

#13 @MattyRob
13 years ago

My fix does affect bulk actions in terms that after a bulk action is applied you are returned to page 1.

Could the hidden filed be renamed and processed in some way?

If I get time I'll do some more digging but as yet I can't figure out why get_pagenum() in class-wp-list-table.pgp returns 1 (or is overridden) on a bulk action.

#14 @azaozz
13 years ago

Apparently [18237] breaks maintaining the page number when using bulk actions anyways, so perhaps we should go with the proposed patch and fix the other regression later. I can make a patch for it but it's more complicated so no good for point release.

#15 follow-ups: @nacin
13 years ago

When you trigger a bulk action, I'm not sure I don't expect you return to page 1. (Except for a bulk edit.)

#16 @azaozz
13 years ago

Patched for trunk in [18436]

#17 @azaozz
13 years ago

Closed #18074 as duplicate.

#18 in reply to: ↑ 15 @MattyRob
13 years ago

Replying to nacin:

When you trigger a bulk action, I'm not sure I don't expect you return to page 1. (Except for a bulk edit.)

I considered this too but I'm not sure now. If you have multiple pages of comments and are spamming or deleting some and you've worked your way as far as page X, the last thing you'd want as an end user is to spam or delete a few comments only to be returned to page 1. You've then got to remember where you were up to in order to resume your task.

Looking at the size of he core patch perhaps we should leave this for 3.2.1 as 'maybelater' or 'wontfix'

#19 follow-up: @azaozz
13 years ago

  • Milestone changed from 3.2.1 to 3.2.2

Lets make sure [18436] works in all cases and add it in 3.2.2.

#20 in reply to: ↑ 19 @MattyRob
13 years ago

Replying to azaozz:

Lets make sure [18436] works in all cases and add it in 3.2.2.

Works pretty well, the only strange behaviour I noted is detailed below:

Test on 4 comments and assigned to list 1 per page in the Screen Options.

  • When Bulk managing - current page is retained as expected.
  • When entering a page numer when viewing all comments or spam comments page number is retained and not over ridden with '1'
  • Strange - when amending a comment on page 3/3 of spam and making it 'non spam' you are left on page '3 of 2' - now this may have been to do with only having one comment per page but the scenario exists where there may only be one comment on the last page and when this is 'changed' in status the user will be left on page X+1 of X - which is a little unusual looking.

#21 in reply to: ↑ 15 @JohnONolan
13 years ago

Replying to nacin:

When you trigger a bulk action, I'm not sure I don't expect you return to page 1. (Except for a bulk edit.)

As mentioned in #18074 - When bulk editing posts (or comments) on page 3, I expect to return to page 3

#22 @danielbachhuber
13 years ago

  • Cc d@… added

#23 @ocean90
13 years ago

There is a problem with the fix in [18436] .

We need to check first if select[name="action"] and select[name="action2"] does exist and then check their values.

#24 @nacin
12 years ago

  • Milestone changed from 3.2.2 to 3.3

#26 @jane
12 years ago

If this still needs the fix ocean90 brought up 2 weeks ago, are we punting this to the next release, or does someone have the fix ready to go?

#27 follow-up: @ocean90
12 years ago

There was a ticket which does need this change, but I can't find it anymore. :(

#28 in reply to: ↑ 27 @DrewAPicture
12 years ago

Replying to ocean90:

There was a ticket which does need this change, but I can't find it anymore. :(

Could jane be referring to the entered-pagination patch you submitted in regards to the theme showcase pagination the other week?

#29 @ryan
12 years ago

  • Milestone changed from 3.3 to Future Release

#30 @nacin
10 years ago

  • Component changed from General to Administration
  • Milestone changed from Future Release to 3.3
  • Resolution set to fixed
  • Status changed from new to closed

Anything left can be brought up in a new ticket. The bulk of this issue was knocked out years ago.

Note: See TracTickets for help on using tickets.