Hebrew post slug is not properly cut on Edit Post screen

[KingYes]

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:

Thanks :)

[KingYes]

Fixes

[ariel.k]

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

[tenpura]

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​

[bainternet]

This is actually the case for most none English based languages,
for example Arabic:

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

[miqrogroove]

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.

[miqrogroove]

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

[KingYes]

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

[SergeyBiryukov]

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.

[KingYes]

Replying to SergeyBiryukov:

You're right.

[SergeyBiryukov]

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.

[SergeyBiryukov]

29573.2.patch​ includes a unit test.

[SergeyBiryukov]

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 even if mbstring is enabled. This part is a regression.

29573.2.patch​ fixes both of these issues.

[miqrogroove]

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].

[tenpura]

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.

[SergeyBiryukov]

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.

[nacin]

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

[miqrogroove]

#29800 was marked as a duplicate.

[nacin]

In 30410:

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

see #29573 for 4.0. see #28350.

[SergeyBiryukov]

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​.

[ocean90]

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().

[SergeyBiryukov]

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

[azaozz]

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

[DrewAPicture]

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

[azaozz]

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).

[azaozz]

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]

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

[SergeyBiryukov]

In 30791:

Revert [28948], which caused a regression.

see #29573 for trunk. see #28350.

[SergeyBiryukov]

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