Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#30883 closed defect (bug) (fixed)

Duplicate post_class() for different categories

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 4.2 Priority: normal
Severity: normal Version:
Component: Posts, Post Types Keywords: has-patch commit
Focuses: Cc:

Description

  1. Create two categories, "Первая рубрика" and "Вторая рубрика". They will have the following slugs:
    %d0%b2%d1%82%d0%be%d1%80%d0%b0%d1%8f-%d1%80%d1%83%d0%b1%d1%80%d0%b8%d0%ba%d0%b0
    %d0%bf%d0%b5%d1%80%d0%b2%d0%b0%d1%8f-%d1%80%d1%83%d0%b1%d1%80%d0%b8%d0%ba%d0%b0
    
  2. Create a post in each category.
  3. Both posts will have a category-- class.

sanitize_html_class() uses a fallback value (term ID) if the resulting string is empty. It should do the same if the resulting string only contains hyphens.

Attachments (5)

patch.diff (1.6 KB) - added by sgrant 9 years ago.
30883.diff (512 bytes) - added by davideugenepratt 9 years ago.
patch.2.diff (2.0 KB) - added by sgrant 9 years ago.
If only non-letter characters remain, use fallback. New tests.
patch.3.diff (3.0 KB) - added by A5hleyRich 9 years ago.
Refresh of patch 2 to work with taxonomy slug
30883.2.diff (8.2 KB) - added by SergeyBiryukov 9 years ago.

Download all attachments as: .zip

Change History (20)

#1 @sgrant
9 years ago

Hiya! I took a crack at this. Added a new conditional in sanitize_html_class, and added three unit tests relating to the fallback in a new file (seems to fit the formatting unit test structure). This should swap any remaining sanitized string to the fallback if it only consists of hyphens. Patch attached. Thanks!

@sgrant
9 years ago

#2 @TempAcc
9 years ago

Also big problem is coming when you try create category with numbers-chars slug. For example "25кадр". In output we get category-25, but if category with ID=25 exists and has own css we have a new problem.

Maybe transliteration will help?

#3 follow-up: @davideugenepratt
9 years ago

I installed sgrant's patch and it works for me. The unit test ran without any failures.

#4 in reply to: ↑ 3 @TempAcc
9 years ago

Replying to davideugenepratt:

I installed sgrant's patch and it works for me. The unit test ran without any failures.

The patch not covering all possible situations. One of these described above by me. Also, as minimum, patch not fixes underscore symbol _ (try instead of hyphen)

I think the best way use $fallback variable when $sanitized != $class or use term ID always.

#5 @davideugenepratt
9 years ago

Or maybe one last conditional that checks for a numerical value and if it's numerical will use the ID. I don't know if the underscore needs to be taken into consideration as it's the hyphen that's used to replace a space.

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

#6 follow-up: @sgrant
9 years ago

If the issue is the non-letter characters, should the single additional check be whether or not the remaining characters after replacement are exclusively in [0-9_-] ? It seems redundant to have multiple checks; rather, maybe it's best to find the line in the sand and do the correct test.

@sgrant
9 years ago

If only non-letter characters remain, use fallback. New tests.

#7 in reply to: ↑ 6 @davideugenepratt
9 years ago

I applied patch.2.diff and it works for me for categories "Первая рубрика", "Вторая рубрика", and "25кадр".

#8 @rachelbaker
9 years ago

  • Keywords has-patch added; good-first-bug needs-patch needs-unit-tests removed

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


9 years ago

#10 @DrewAPicture
9 years ago

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

We're going to need a new patch here. patch.2.diff doesn't account for the "category" part of the class name (therefore removing the hyphens via the preg_replace() returns "category" not empty), and for that matter, we should probably handle it for any prospective taxonomy slug.

@A5hleyRich
9 years ago

Refresh of patch 2 to work with taxonomy slug

#11 @A5hleyRich
9 years ago

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

Refreshed to work with any taxonomy. The taxonomy name should only contain lowercase letters and underscores, as per the codex (http://codex.wordpress.org/Function_Reference/register_taxonomy). Therefore, do we need to sanitize the taxonomy name?

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


9 years ago

#13 follow-up: @DrewAPicture
9 years ago

  • Keywords commit added; needs-testing removed
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

@SergeyBiryukov: patch.3.diff fixes the problem for me. I wonder what you think about this bit, however: $taxonomy . '-' . sanitize_html_class( $term->slug, $term->term_id ). What are your thoughts on essentially keeping sanitize_html_class() separate from the term slug logic as .3 does?

#14 in reply to: ↑ 13 @SergeyBiryukov
9 years ago

Replying to DrewAPicture:

What are your thoughts on essentially keeping sanitize_html_class() separate from the term slug logic as .3 does?

Actually, I don't think we should touch sanitize_html_class() at all, it works as expected.

This logic should be in post_class() and body_class().

30883.2.diff is a new patch with new tests.

#15 @SergeyBiryukov
9 years ago

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

In 31979:

Avoid duplicate classes for different terms with UTF-8 slugs in post_class() and body_class().

Fall back to term ID if the sanitized slug is numeric or only contains hyphens.

props SergeyBiryukov, A5hleyRich, sgrant, davideugenepratt.
fixes #30883.

Note: See TracTickets for help on using tickets.