WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#29573 closed defect (bug) (fixed)

Hebrew post slug is not properly cut on Edit Post screen

Reported by: KingYes Owned by:
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.0
Component: Permalinks Keywords: has-patch fixed-minor commit dev-feedback
Focuses: Cc:
PR Number:

Description

Hey.. I got bug in Hebrew lang and I have a patch to fix this.

When I press בניית אתרי וורדפרס and I just press "OK" I got this:

http://i.imgur.com/7qy5v0c.png

Thanks :)

Attachments (8)

trac_29573.patch (676 bytes) - added by KingYes 5 years ago.
Fixes
29573.patch (687 bytes) - added by SergeyBiryukov 5 years ago.
29573.2.patch (2.6 KB) - added by SergeyBiryukov 5 years ago.
29573.3.patch (3.2 KB) - added by SergeyBiryukov 5 years ago.
29573.4.patch (5.0 KB) - added by SergeyBiryukov 5 years ago.
29573.5.patch (1.7 KB) - added by azaozz 5 years ago.
slug.png (11.7 KB) - added by azaozz 5 years ago.
slug-edit.png (11.4 KB) - added by azaozz 5 years ago.

Download all attachments as: .zip

Change History (43)

@KingYes
5 years ago

Fixes

#1 follow-up: @SergeyBiryukov
5 years ago

  • Component changed from General to Permalinks
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.1

#2 follow-up: @SergeyBiryukov
5 years ago

  • Summary changed from Summery permelink in Edit Page not work with Hebrew to Hebrew post slug is not properly cut on Edit Post screen

#3 in reply to: ↑ 1 @ariel.k
5 years ago

Replying to SergeyBiryukov:
This patch should not go into version 4.0.1?

Last edited 5 years ago by ariel.k (previous) (diff)

#4 in reply to: ↑ 2 ; follow-up: @tenpura
5 years ago

Replying to SergeyBiryukov:

Why don't we have mb_strlen() fallback in compat.php? I think it makes things easier and you've already had one: ticket:21013:21013.2.patch

#5 @bainternet
5 years ago

This is actually the case for most none English based languages,
for example Arabic:
http://i.imgur.com/xyNMk3p.png

And Its a major major bug which should be fixed in the next release.

#6 @miqrogroove
5 years ago

It is not necessary to check existence of mb_strlen twice. I suggest expanding the patch to make the code easier to read with more if statements.

#7 follow-up: @miqrogroove
5 years ago

  • Keywords has-patch removed
  • Version changed from trunk to 4.0

Sergey, this bug was introduced in [28948] and needs to be reverted in 4.0.1.

#8 @KingYes
5 years ago

@SergeyBiryukov, You can add seems_utf8() but you need to add my if statements too.

#9 in reply to: ↑ 7 ; follow-up: @SergeyBiryukov
5 years ago

  • Milestone changed from 4.1 to 4.0.1

Replying to miqrogroove:

Sergey, this bug was introduced in [28948] and needs to be reverted in 4.0.1.

You're right.

Replying to bainternet:

And Its a major major bug which should be fixed in the next release.

Note that it's only a cosmetic bug with with the representation, the actual permalink is correct,

trac_29573.patch can still display an invalid character if mbstring is disabled. I think a correct fix would be not to cut UTF-8 slugs in that case, see 29573.patch. Working on a unit test.

#10 in reply to: ↑ 9 @KingYes
5 years ago

Replying to SergeyBiryukov:

You're right.

#11 in reply to: ↑ 4 @SergeyBiryukov
5 years ago

Replying to tenpura:

Why don't we have mb_strlen() fallback in compat.php? I think it makes things easier and you've already had one: ticket:21013:21013.2.patch

Well, we only use it twice in core (the other instance is in POMO_Reader), and it ended up not being required for #21013.

#12 @SergeyBiryukov
5 years ago

  • Keywords has-patch added

29573.2.patch includes a unit test.

#13 @SergeyBiryukov
5 years ago

  • Keywords commit added

To summarize, there are two issues:

  • If mbstring is disabled, we don't properly cut non-latin slugs with strlen() more than 30. This part is not a regression, see [6633], [6794], and [9986].
  • Since [28948], the same applies to non-latin slugs with mb_strlen() of 30 or less and strlen() more than 30, if mbstring is enabled. This part is a regression.

29573.2.patch fixes both of these issues.

Last edited 5 years ago by SergeyBiryukov (previous) (diff)

#14 @miqrogroove
5 years ago

  • Keywords commit removed

I disagree. This patch would disable string truncation when mb_strlen is unavailable. Please add another unit test:

$post_name = 'This is a very long post name that is both UTF-8 and > 30 chars.';

If you could run that with the mb_strlen function disabled, you would find the string is not 'abridged'.

I think for 4.0.1 the best solution is to revert [28948].

#15 @tenpura
5 years ago

ASCII is a subset of UTF-8, so if the reason you used ! seems_utf8() is to skip multibyte UTF-8 characters, it should not work as intended.

#16 @SergeyBiryukov
5 years ago

Two more takes:

29573.3.patch uses the same approach as we use in wp_trim_words() if mb_strlen() is not available.

29573.4.patch introduces a compat version of mb_strlen() to make things easier.

Replying to miqrogroove:

I think for 4.0.1 the best solution is to revert [28948].

I think so too.

#17 @nacin
5 years ago

  • Keywords commit added

I agree with reverting [28948] for 4.0.1. Marking that part for commit and will handle.

This ticket was mentioned in IRC in #wordpress-dev by SergeyBiryukov. View the logs.


5 years ago

This ticket was mentioned in IRC in #wordpress-dev by nacin2. View the logs.


5 years ago

#20 @miqrogroove
5 years ago

#29800 was marked as a duplicate.

#21 @nacin
5 years ago

In 30410:

Revert [28948] for the 4.0 branch, which caused a regression.

see #29573 for 4.0. see #28350.

#22 @nacin
5 years ago

  • Keywords fixed-minor added

#23 @ocean90
5 years ago

  • Keywords commit removed
  • Milestone changed from 4.0.1 to 4.1

#24 @SergeyBiryukov
5 years ago

  • Keywords commit added

Either 29573.3.patch or 29573.4.patch should be good to go. We just need to decide whether we want to add a compat version of mb_strlen(), or just fix the issue at hand. I'm leaning towards 29573.4.patch.

#25 @ocean90
5 years ago

Maybe I'm missing something but: 29573.4.patch includes if ( ! in_array( $charset, array( 'utf8', 'utf-8', 'UTF8', 'UTF-8' ) ) ) which can be reduced to 'UTF-8' === $charset because of _canonical_charset().

We should do the same for _mb_substr() and wp_check_invalid_utf8().

#26 @SergeyBiryukov
5 years ago

I was just following [17621], [21467], and [21842]. Yeah, we can clean that up a bit.

#27 @azaozz
5 years ago

Shouldn't we be shortening that string from JS? Seems more appropriate and would work better. Perhaps try that in 4.2?

#28 @DrewAPicture
5 years ago

  • Keywords dev-feedback added

Seems like we need a decision and a new patch here. Any takers?

#29 @azaozz
5 years ago

IMHO best would be to just revert [28948] like in 4.0.1 for now and split and shorten that string from JS in 4.2.

If I remember right some years ago we had mb_strlen() compat function but dropped it later (not sure if it made it to a release). Seemed nearly all hosts that host non English sites have the mb_* functions enabled. For 29573.3.patch, the u modifier might not work on some hosts (PCRE is without UTF-8 support).

Last edited 5 years ago by azaozz (previous) (diff)

#30 @azaozz
5 years ago

Thinking more about this: why do we stick the ellipsis in the middle of the slug? Probably because at the time this was added, the CSS text-overflow: ellipsis; wasn't working in all browsers.

However all currently used browsers support that now. The CSS ellipsis are used everywhere, including about 20 other places in the admin, and almost all users are familiar with them. Seems it is time to update the slug truncation too.

@azaozz
5 years ago

#31 @azaozz
5 years ago

In 29573.5.patch: remove the slug truncation from PHP and add ellipsis with CSS.

@azaozz
5 years ago

@azaozz
5 years ago

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


5 years ago

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


5 years ago

#34 @SergeyBiryukov
5 years ago

In 30791:

Revert [28948], which caused a regression.

see #29573 for trunk. see #28350.

#35 @SergeyBiryukov
5 years ago

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

Closing as fixed for both trunk and the 4.0 branch. Follow-up: #30633.

Note: See TracTickets for help on using tickets.