Make WordPress Core

Opened 18 years ago

Last modified 4 months ago

#4463 accepted enhancement

Pretty permalink on search

Reported by: markjaquith's profile markjaquith Owned by: aristath's profile aristath
Milestone: Future Release Priority: normal
Severity: normal Version: 2.3
Component: Permalinks Keywords: has-patch has-unit-tests needs-dev-note changes-requested
Focuses: Cc:

Description

Ryan:

In trunk, with cruft free links, I get stuff like this:

http://foo.blog/page/3/?s=test

That's not right. Maybe we should revert back to pre [5454] to fix the trunk problems.

[5454] was the commit for #3930

If at all possible, I'd like to work with the new code.

Attachments (2)

4463.diff (956 bytes) - added by wonderboymusic 12 years ago.
4463_1.diff (1.1 KB) - added by faishal 12 years ago.
New patch

Download all attachments as: .zip

Change History (45)

#1 @Nazgul
18 years ago

  • Keywords needs-patch added
  • Milestone changed from 2.3 (trunk) to 2.4 (future)

#2 @MtDewVirus
17 years ago

Tested with 7236.

Whenever performing searches, the search results do have URLs in the form of /?s=test.

#3 @DD32
16 years ago

  • Component changed from General to Template
  • Priority changed from high to normal

What would be a suggested fix for this ticket? /search/search-term/page/2 ? /?s=search-term&paged=2 ?

#4 @Denis-de-Bernardy
16 years ago

+1 for this: /?s=search-term&paged=2

#5 @Denis-de-Bernardy
16 years ago

  • Component changed from Template to Permalinks
  • Owner changed from anonymous to ryan

#6 @hakre
15 years ago

/search/search-term/page/2 with a configureable option in backend for wording of "search" would be pretty nice to fillup serps.

anyway isn't this pretty uninteresting anyway as long as it doesn't lead to a 404?

#7 @janeforshort
15 years ago

  • Milestone changed from 2.9 to Future Release

No patch, punting.

#8 @wonderboymusic
12 years ago

  • Keywords reporter-feedback added; needs-patch removed

Is this still relevant?

#9 @SergeyBiryukov
12 years ago

  • Keywords needs-patch added; reporter-feedback removed

Paged search links still look as described: http://trunk.wordpress/page/2/?s=test.

Before [5454], they used to be http://trunk.wordpress/?s=test&paged=2.

#10 @wonderboymusic
12 years ago

  • Keywords has-patch added; needs-patch removed

Makes search links with pretty permalinks on used paged in the query string

@wonderboymusic
12 years ago

#11 @wonderboymusic
12 years ago

  • Milestone changed from Future Release to 3.6

Refreshed patch against trunk - this ticket is like 19,000 tickets old, but the patch works

#12 @wonderboymusic
12 years ago

#21748 was marked as a duplicate.

@faishal
12 years ago

New patch

#13 @faishal
12 years ago

  • Cc saiyedfaishal@… added

#14 @MikeHansenMe
12 years ago

  • Cc mdhansen@… added

Both 4463.diff and 4463_1.diff apply and work as intended. In 4463_1.diff there is a couple addition lines for is_single ! is_home and ! is_front_page. I am not sure if that is necessary as 4463.diff seems to work without it.

#15 @nacin
12 years ago

  • Keywords 3.7-early added
  • Milestone changed from 3.6 to Future Release

Longstanding bug.

#16 @wonderboymusic
12 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

#17 @wonderboymusic
11 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from 3.7 to 3.8

#18 @dd32
11 years ago

  • Milestone changed from 3.8 to Future Release

#19 @ryan
11 years ago

  • Owner ryan deleted
  • Status changed from new to assigned

#20 @chriscct7
9 years ago

  • Keywords 3.7-early removed
  • Milestone changed from Future Release to 4.4
  • Owner set to chriscct7
  • Status changed from assigned to reviewing

#21 @chriscct7
9 years ago

  • Milestone changed from 4.4 to Future Release
  • Owner chriscct7 deleted

#22 @Mte90
23 months ago

  • Summary changed from Strange paging links to Pretty permalink on search

Just adding a more context focused title to avoid reading all the history in this 16 years ticket.

I don't know if it is something that we want, I mean now it is a standard that on wordpress the only parameter not pretty is s for the search.

The topic was opened because at the time there were some changes on adding as pretty the paged value.

As today, all the search engine doesn't have pretty permalinks for the search string value like Google.

#23 @oglekler
18 months ago

@joostdevalk can you please give your opinion on this matter. Thank you 🙏

#24 @joostdevalk
18 months ago

I'd actually prefer us to rip out the clean version of search permalinks from all WordPress sites. Basically get rid of /search/ altogether and redirect all /search/ to ?s= queries.

For search, using a parameterized URL has remained the most common pattern across all big CMSes and big websites. It makes tracking a lot easier, and not having both clean and non-clean modes actually improves cache hit rates and decreases spam attack vectors.

Paginated search URLs should then always use a second parameter, so, back to https://example.com/?s=test&paged=2 or similar.

#25 @jonoaldersonwp
18 months ago

+1. Using pretty permalinks for search causes all sorts of SEO & performance headaches. Would love to remove this.

#26 @markjaquith
18 months ago

In the sixteen years since I opened this ticket, my mind has changed. The web has spoken, and searches use query strings. The reason is rather obvious: it’s the only non-JS way to submit a typed search from a form.

I like @joostdevalk’s suggestion.

#27 @johnbillion
18 months ago

  • Keywords needs-patch added; has-patch removed
  • Type changed from defect (bug) to task (blessed)

#28 @oglekler
16 months ago

@SergeyBiryukov can we plan this for 6.5?

#29 @aristath
16 months ago

  • Owner set to aristath
  • Status changed from reviewing to accepted

#30 @SergeyBiryukov
16 months ago

  • Milestone changed from Future Release to 6.5

This ticket was mentioned in PR #5569 on WordPress/wordpress-develop by @aristath.


16 months ago
#31

  • Keywords has-patch has-unit-tests added; needs-unit-tests needs-patch removed

Trac ticket: https://core.trac.wordpress.org/ticket/4463

This PR does the following:

  • Removes pretty permalinks for search results
  • Redirects old permalinks to the new ones (using ?s=query)

#32 @rajinsharwar
15 months ago

  • Keywords needs-dev-note added

The patch looks good and seems to resolve the said problem. I believe we will need a Dev Note for this change.

#33 @joostdevalk
15 months ago

I'm happy to write the dev note.

#34 @jorbin
15 months ago

Thanks for volunteering Joost!

I really like the direction this is going, but the unit tests seem to be taking a step backwards in the current patch. I noted this on the PR, but it would be good to get those back up to shape or improved before this lands.

#35 @jorbin
12 months ago

  • Keywords changes-requested added

Wanted to note that we are 12 days from beta 1 so if this is going to make it into 6.5, the updates to the unit tests need to be completed.

@aristath commented on PR #5569:


12 months ago
#36

I've been trying to fix an issue in this PR for days. Unfortunately, so far I have not been able to, so I pushed some additional PHPUnit tests in this one to make the issue more evident.
The problem is that for some reason, if in my browser I go to /search/hello/page/2, it _should_ redirect to /?s=hello&paged=2. Instead, it goes to /page/2/?s=hello. 🤔

@SergeyBiryukov since we worked on this PR together when it first started, do you perhaps have any insights as to what may be happening here?

#37 @swissspidy
12 months ago

  • Milestone changed from 6.5 to 6.6
  • Type changed from task (blessed) to enhancement

This ticket was mentioned in Slack in #core by oglekler. View the logs.


9 months ago

#39 @audrasjb
9 months ago

  • Milestone changed from 6.6 to 6.7

No change since 4 months, let's move this one to 6.7.
Feel free to move it back to 6.6 if there an updated patch is available.

#40 @desrosj
5 months ago

Looks like @peterwilsoncc weighed in on the attached pull request a few weeks ago. @aristath are you interested and able to take a look at the feedback and bring the branch up to date with trunk?

This ticket was mentioned in Slack in #core by stoyangeorgiev. View the logs.


5 months ago

This ticket was mentioned in Slack in #core by chaion07. View the logs.


4 months ago

#43 @chaion07
4 months ago

  • Milestone changed from 6.7 to Future Release

Thanks @markjaquith for reporting this. We have reviewed this Ticket during a recent bug-scrub session. Based on the feedback received we are making the following changes:

  1. Updating the milestone to Future Release
  2. @aristath needs to address review feedback per last comment

Thanks.

Props to @mukesh27

Cheers!

Note: See TracTickets for help on using tickets.