Make WordPress Core

Opened 15 years ago

Closed 7 years ago

#10792 closed defect (bug) (fixed)

Forward Slashes convert to hyphens in post slugs

Reported by: alxndr's profile alxndr Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 4.9 Priority: low
Severity: minor Version: 3.8
Component: Permalinks Keywords: dev-feedback
Focuses: Cc:

Description

In slugs for taxonomies or post permalinks, slashes (/\) and ampersands (&) are stripped out. More useful URLs would be created by turning slashes into hyphens, and ampersands into the word "and".

e.g.:

"songs by Lennon/McCartney"
expected slug: "songs-by-lennon-mccartney"
actual slug: "songs-by-lennonmccartney"

"Us & Them"
expected slug: "us-and-them"
actual slug: "us-them"

Attachments (6)

slug-slash-ampersand.2.patch (594 bytes) - added by alxndr 15 years ago.
replaces slashes with hyphens in post title slugs and taxonomy term slugs; replaces ampersands with "and" and "&c" with "etc" in post title slugs
10792-forward-slash-to-hyphen.diff (467 bytes) - added by GhostToast 10 years ago.
convert forward slash to hyphen
10792.diff (3.2 KB) - added by jtsternberg 10 years ago.
Covers the use of &c, &c., &, and /. Includes unit tests and patch.
10792-only-slashes-w-unittests.diff (1.5 KB) - added by corvidism 9 years ago.
10792.patch (1.6 KB) - added by corvidism 7 years ago.
The recreated patch.
10792-01.patch (1.6 KB) - added by corvidism 7 years ago.
patch with correct direction

Download all attachments as: .zip

Change History (40)

@alxndr
15 years ago

replaces slashes with hyphens in post title slugs and taxonomy term slugs; replaces ampersands with "and" and "&c" with "etc" in post title slugs

#1 @alxndr
15 years ago

This patch doesn't extend the ampersand fix to category/tag slugs (but the slash fix works). Perhaps ampersands are being stripped out before the title hits sanitize_title_with_dashes() ? I haven't dug around enough yet...

#2 @scribu
15 years ago

  • Milestone changed from Unassigned to Future Release

Agree with the slash replacement.

Don't agree with the & replacement due to internationalization problems: if you write a blog in french, you won't want '&' to be replaced with 'and'.

#3 @alxndr
15 years ago

Oh right. ('and') perhaps?

#4 @alxndr
15 years ago

Arg... that should have been

__('and')

#5 @SergeyBiryukov
13 years ago

  • Component changed from General to Formatting
  • Keywords has-patch added
  • Milestone changed from Future Release to 3.3

Related: #10823

#6 @SergeyBiryukov
13 years ago

  • Keywords needs-refresh added

#7 follow-up: @SergeyBiryukov
13 years ago

We already have __(' and ') string in .pot from wp_sprintf_l(), so we probably can use it here.

#8 @SergeyBiryukov
12 years ago

  • Milestone changed from 3.3 to Future Release

#9 @blogbuzz
12 years ago

How can I force to use the slash in the url? I want to use a slash in my post url, but when I save my custom slug in the post editor it gets stripped out. Should be possible cause there are situations were you want to use slashes.

Last edited 12 years ago by blogbuzz (previous) (diff)

#11 @GhostToast
10 years ago

Any traction on this? Particularly involving the forward slash "/" not being converted into a dash/hyphen "-", I have long thought this behavior odd, as it defaults to slugs which are unreadable. Are there any instances where hyphen would not be desired in place of a slash?

#12 in reply to: ↑ 7 @F J Kaiser
10 years ago

Replying to SergeyBiryukov:

We already have __(' and ') string in .pot from wp_sprintf_l(), so we probably can use it here.

I'm not too sure how someone would react if they got a site where the internal language is defined to be Korean, but the post is written in English. Example Title:

Me & her spent a wonderful weekend together

URL after the patch converted & to __('and') and therefor to Korean:

http://my-korean-blog.kr/me-와-her-spent-a-wonderful-weekend-together

Very sure that it would be better to simply replace an ampersand with a dash.

Last edited 10 years ago by F J Kaiser (previous) (diff)

@GhostToast
10 years ago

convert forward slash to hyphen

#13 @GhostToast
10 years ago

  • Keywords needs-testing added; needs-refresh removed
  • Version set to 3.8

#14 @GhostToast
10 years ago

  • Summary changed from ampersands and slashes stripped out of slugs to Forward Slashes convert to hyphens in post slugs

#15 @nacin
10 years ago

  • Keywords needs-unit-tests added; needs-testing removed

I agree with the slash (only). I can never remember how this works — is this safe to change without affecting existing slugs? Seems like.

Unit tests would be great.

#16 @GhostToast
10 years ago

I have tested various configurations of saving/pending/draft/publish and have found the following:

  • If a title Computers/Robots was saved prior to my patch, it's slug computersrobots remains unchanged despite various publish/unpublish status changes (unless intentionally edited).
  • If a title Computers/Robots is pending for publish, and auto-slug generation occurs, computers-robots is assigned. Manually editing this slug back to computersrobots if desired, will be honored, despite any publish/save/unpublish status changes.
  • If a title Computers/Robots has been published with the patch, it's slug becomes computers-robots until manually edited.

This all seems to be working as intended.

Last edited 10 years ago by GhostToast (previous) (diff)

#17 @jtsternberg
10 years ago

  • Keywords dev-feedback 2nd-opinion added; needs-unit-tests removed

I have some unit tests for this which cover the use of &c, &c., &, and /. Are we saying we DON'T want to convert ampersands to 'and'? If not, are saying it should instead replace with a hyphen? I'll upload my diff as-is (including the unit-test) so you can test it out.

@jtsternberg
10 years ago

Covers the use of &c, &c., &, and /. Includes unit tests and patch.

#18 @corvidism
9 years ago

Hi,
the slash-to-hyphen part of this is something I would really appreciate getting fixed - I'm maintaining several sites with a lot of pages named "2012/2013" and the like, and the disappearing forward slashes are causing some seriously ugly slugs. ("2012/2013" --> "2122013-2")

How can I help? I have no experience with working on WP Core, but I definitely can do manual testing or run unit tests if that's necessary.

#19 follow-up: @miqrogroove
9 years ago

  • Keywords needs-unit-tests added; dev-feedback 2nd-opinion removed
  • Severity changed from trivial to minor

To move this forward, let's see some tests, and let's make sure multiple slashes or spaces never convert into multiple hyphens. I'm in favor of jtsternberg's idea to convert ampersand to a hypen rather than to strip, assuming we are not dealing with HTML encoded strings.

#20 in reply to: ↑ 19 ; follow-up: @corvidism
9 years ago

Replying to miqrogroove:

To move this forward, let's see some tests, and let's make sure multiple slashes or spaces never convert into multiple hyphens. I'm in favor of jtsternberg's idea to convert ampersand to a hypen rather than to strip, assuming we are not dealing with HTML encoded strings.

Okay, lets see.
I tried to apply the jtsternberg's patch, but that failed: there were some other changes made to formatting.php since then and the patched file doesn't even pass the current unit tests.
I did, however, get to work the earlier patch by GhostToast, the one that only adds the "forward slash to dash" functionality. I wrote a bunch of unit tests for multiple slashes and positions, so it seems to be safe.
So... what now?
IMO replacing "&" to "-and-" or "&c." to "-etc-" is wrong, because it ignores languages other then English. Should we replace it with a dash, too?

#21 @crysman
9 years ago

+1 here, I wish this bug was fixed soon at last, too! I cannot develop currently, but if you need some tests, I might run some...

#22 in reply to: ↑ 20 ; follow-up: @SergeyBiryukov
9 years ago

Replying to corvidism:

IMO replacing "&" to "-and-" or "&c." to "-etc-" is wrong, because it ignores languages other then English. Should we replace it with a dash, too?

Yes, let's replace a standalone ampersand (not a part of an HTML entity) with a hyphen as well.

#23 in reply to: ↑ 22 @corvidism
9 years ago

  • Keywords dev-feedback added; needs-unit-tests removed

Replying to SergeyBiryukov:

Replying to corvidism:

IMO replacing "&" to "-and-" or "&c." to "-etc-" is wrong, because it ignores languages other then English. Should we replace it with a dash, too?

Yes, let's replace a standalone ampersand (not a part of an HTML entity) with a hyphen as well.

That's the current way of dealing with them, so no more coding or testing needed :) I think this might be done then!
Now... I'm not sure what I'm supposed to do. Oops. :B I removed the needs-unit-tests tag, and tagged this dev-feedback - if I'm messing up, somebody please correct me.

#24 @crysman
9 years ago

  • Component changed from Formatting to Permalinks

Hello, I just want to know how it's goin' on in here? Will the patch be implemented? The slashes bug is a real pain!

Moreover, I do not think it's "enhancement", I consider it a bug. Making 20122013-2 out of 2012/2013 makes no sense at all.

Thanks

Last edited 9 years ago by crysman (previous) (diff)

#25 @Zurd
7 years ago

bump... just came across this 7 years old SEO bug. It's an easy fix, replace / with -

Like people said, don't use "and" as it's not multi-language friendly.

#26 @swissspidy
7 years ago

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

Seems like we can build upon 10792.diff and 10792-only-slashes-w-unittests.diff to properly replace slashes and ampersands with hyphens.

@corvidism
7 years ago

The recreated patch.

#27 @corvidism
7 years ago

  • Keywords needs-unit-tests needs-patch removed

I recreated the old patch (including the unit tests). What else do I need to do to get this actually commited this time?

#28 @swissspidy
7 years ago

Thanks, @corvidism! It looks like additions and removals are reversed in your patch, i.e. there's more red than green in 10792.patch. Plus, 10792-only-slashes-w-unittests.diff seems to have a few tests for slashes as well that could be included.

@corvidism
7 years ago

patch with correct direction

#29 @corvidism
7 years ago

Replying to swissspidy:

Thanks, @corvidism! It looks like additions and removals are reversed in your patch, i.e. there's more red than green in 10792.patch. Plus, 10792-only-slashes-w-unittests.diff seems to have a few tests for slashes as well that could be included.

Ooops, yeah, sorry about that - I followed the instructions on https://make.wordpress.org/core/handbook/contribute/git/ and they're wrong (they do the diff in the wrong direction).

I used all those tests from 10792-only-slashes-w-unittests.diff, though, they're in the fixed patch.

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


7 years ago

#31 @corvidism
7 years ago

  • Type changed from enhancement to defect (bug)

#32 @swissspidy
7 years ago

#41718 was marked as a duplicate.

#33 @SergeyBiryukov
7 years ago

  • Milestone changed from Future Release to 4.9
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#34 @SergeyBiryukov
7 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 41318:

Formatting: In sanitize_title_with_dashes(), convert forward slash to hyphen on save.

Props corvidism, jtsternberg, GhostToast, alxndr.
Fixes #10792.

Note: See TracTickets for help on using tickets.