WordPress.org

Make WordPress Core

Opened 7 years ago

Last modified 12 days ago

#14981 assigned defect (bug)

Provide a context for post statuses

Reported by: xibe Owned by: rockwell15
Milestone: Future Release Priority: normal
Severity: normal Version: 3.1
Component: I18N Keywords: good-first-bug has-patch dev-feedback needs-testing
Focuses: Cc:

Description

1) /wp-admin/nav-menus.php: the "Most Recent" string (/wp-admin/includes/nav-menu.php:645) should be separated between the Pages context and the Articles context, since they can take different forms according to the language (i.e.: in French, "Articles" is masculine, "Pages" is feminine.

2) /wp-admin/edit.php: Same contextual need for "All", "Published", "Scheduled" and the rest of the per-status selector, for posts and articles (and others...).

Attachments (6)

14981.patch (9.5 KB) - added by rockwell15 17 months ago.
Patch for #14981
14981.1.patch (12.2 KB) - added by rockwell15 17 months ago.
Added post statuses, nav menu filters, & table view labels to get_post_type_labels()
14981.2.patch (12.7 KB) - added by rockwell15 17 months ago.
I think this patch addresses everything!
14981.3.patch (12.8 KB) - added by rockwell15 16 months ago.
Fixed patch index
14981.diff (13.2 KB) - added by swissspidy 15 months ago.
14981.4.patch (14.9 KB) - added by rockwell15 6 months ago.
Refreshed patch & added the logic to 'post-state'

Download all attachments as: .zip

Change History (51)

#1 @RanYanivHartstein
6 years ago

  • Cc ran@… added

+1 for this, the need exists for the Hebrew translation as well.

#2 @westi
6 years ago

  • Keywords 3.2-early added
  • Milestone changed from Awaiting Review to Future Release

Both of these improvement require a fair few changes.

The strings for the Post Statuses have context of "Post" but get used for all Post Types.

Marking as a 3.2 candidate

#3 @ocean90
6 years ago

  • Keywords needs-patch added

#4 @xibe
5 years ago

  • Keywords 3.2-early removed
  • Version changed from 3.0.1 to 3.3.1

Still an issue in 3.4-alpha.

#5 @chriscct7
3 years ago

Still a valid issue in 4.0

#6 @cfoellmann
3 years ago

A list of affected strings in context must be compiled by multiple people speaking different languages presenting these "irregularities"

#8 @cfoellmann
3 years ago

  • Version set to trunk

#9 @johnbillion
2 years ago

  • Version changed from trunk to 3.1

The version field indicates the first version in which the issue was present.

#10 @swissspidy
18 months ago

  • Keywords good-first-bug added

#11 follow-up: @iworks
18 months ago

It should be like this:

switch( $post_type_name ) {
   case 'post':
       _ex( 'Most Recent', 'post' );
       break;
   case 'page':
       _ex( 'Most Recent', 'page' );
       break;
   default:
       _e( 'Most Recent' );
       break;
}

or can be like this:

_ex( 'Most Recent', $post_type_name );

#12 in reply to: ↑ 11 ; follow-up: @ocean90
18 months ago

Replying to iworks:

or can be like this:

_ex( 'Most Recent', $post_type_name );

No, variables are not supported as a context parameter.

#13 in reply to: ↑ 12 @iworks
18 months ago

Replying to ocean90:

Replying to iworks:

or can be like this:

_ex( 'Most Recent', $post_type_name );

No, variables are not supported as a context parameter.

I expected this answer ;-)

#14 @swissspidy
18 months ago

Since variables are not supported for context and the labels are indeed different for different post types (as noted by OP), what about adding this to get_post_type_labels()?

This ticket was mentioned in Slack in #polyglots by ocean90. View the logs.


17 months ago

#16 @fxbenard
17 months ago

Bump it up. Time to solve it. A 5 year old ticket ;)

#17 follow-up: @ocean90
17 months ago

  • Summary changed from Two i18n misses to Provide a context for post statuses

#18 in reply to: ↑ 17 @xibe
17 months ago

Replying to ocean90: Thank you, it is indeed much clearer this way :)

@rockwell15
17 months ago

Patch for #14981

#19 @rockwell15
17 months ago

  • Keywords has-patch added; needs-patch removed

#20 follow-up: @swissspidy
17 months ago

Thanks for the patch, @rockwell15!

Just note that not only the post statuses should be post type labels, but also the other mentioned strings like __( 'Search' ), __( 'View All' ) and __( 'Most Recent' ).

Also, we usually use yoda conditions, i.e. if( 'private' === $post->post_status ) {...} instead of if( $post->post_status == 'private' ) {...}

#21 in reply to: ↑ 20 ; follow-up: @rockwell15
17 months ago

Replying to swissspidy:

Thanks for the patch, @rockwell15!

Just note that not only the post statuses should be post type labels, but also the other mentioned strings like __( 'Search' ), __( 'View All' ) and __( 'Most Recent' ).

Also, we usually use yoda conditions, i.e. if( 'private' === $post->post_status ) {...} instead of if( $post->post_status == 'private' ) {...}

Ok, I'll fix the conditions format.

I didn't add those 3 strings you mentioned to get_post_type_labels() since those only appear once regarding post type (wp-admin/includes/nav-menu.php ~ line 382), so I thought it unnecessary for that function to fetch them every time it is called. You think I should add them to get_post_type_labels() though?

#22 in reply to: ↑ 21 @swissspidy
17 months ago

  • Keywords needs-refresh added

Replying to rockwell15:

I didn't add those 3 strings you mentioned to get_post_type_labels() since those only appear once regarding post type (wp-admin/includes/nav-menu.php ~ line 382), so I thought it unnecessary for that function to fetch them every time it is called. You think I should add them to get_post_type_labels() though?

Yes!

Again, that's what has been reported originally. These strings can be translated differently per post type. Just separating posts and pages is not enough. There are other post types as well.

#24 in reply to: ↑ 23 @rockwell15
17 months ago

Ok, thanks for clarification. I'll get on that.

Replying to ocean90:

The patch needs to cover the table views trunk/src/wp-admin/includes/class-wp-posts-list-table.php@35901#L264 too.

What table views are you referring to exactly, the 'Mine', 'All', and 'Sticky'?

Last edited 17 months ago by rockwell15 (previous) (diff)

@rockwell15
17 months ago

Added post statuses, nav menu filters, & table view labels to get_post_type_labels()

#25 in reply to: ↑ 23 ; follow-up: @fxbenard
17 months ago

Replying to ocean90:

The patch needs to cover the table views trunk/src/wp-admin/includes/class-wp-posts-list-table.php@35901#L264 too.

In French like edit/ quick edit/ trash/ view are verbs, no need to translate them differently

#26 in reply to: ↑ 25 ; follow-up: @ocean90
17 months ago

Replying to fxbenard:

In French like edit/ quick edit/ trash/ view are verbs, no need to translate them differently

I'm talking about "All", "Pending" or "Mine".

#27 in reply to: ↑ 26 @fxbenard
17 months ago

Replying to ocean90:

I'm talking about "All", "Pending" or "Mine".

All > Tous or Toutes
Pending > En attente
Mine > Le mien or La mienne

Yes it's needed

@rockwell15
17 months ago

I think this patch addresses everything!

#28 @rockwell15
17 months ago

  • Keywords needs-refresh removed

#29 @rockwell15
16 months ago

  • Keywords dev-feedback added

#30 @SergeyBiryukov
16 months ago

#35056 was marked as a duplicate.

@rockwell15
16 months ago

Fixed patch index

#31 @fxbenard
15 months ago

It's still time to have a last look here no? it will improve i18n for several languages

#32 @xibe
15 months ago

Indeed!

Testing @rockwell15's patch would mean generating the POT file and checking that the contexts are indeed separated, right? I'll try and do that.

#35 @wolforg
15 months ago

I agree with this, the women in the french community aren't happy when they read "Les miens" for the pages.

#36 follow-up: @ocean90
15 months ago

Please avoid any further +1 and use the star beside the title instead.

It would be more helpful if you could test the latest patch and provide feedback.

14981.3.patch: I don't think we should split the string All <span class="count">(%s)</span>, see https://core.trac.wordpress.org/ticket/31859 and https://core.trac.wordpress.org/ticket/31251#comment:17.

Last edited 15 months ago by ocean90 (previous) (diff)

@swissspidy
15 months ago

#37 in reply to: ↑ 36 @swissspidy
15 months ago

Replying to ocean90:

14981.3.patch: I don't think we should split the string All <span class="count">(%s)</span>, see https://core.trac.wordpress.org/ticket/31859 and https://core.trac.wordpress.org/ticket/31251#comment:17.

Agree, though it makes things a bit inconsistent. For example, the string Published is needed both with the <span> and without it. What should we do about that?

See: #31859, 31251#comment:17


14981.diff is an updated patch without the splitted All <span class="count">(%s)</span> string. I also removed the auto-draft string as it's not needed (maps to draft) and fixed some wording (Sticky is still Sticky in plural, not Stickies).

As for testing the changes, I ran php tools/i18n/makepot.php wp-frontend src test.pot and the strings get correctly added to the POT file with the different contexts.

#38 follow-up: @fxbenard
15 months ago

Can you send the pot over, I'll be happy to give it a try and check everything is working fine.

#39 in reply to: ↑ 38 @swissspidy
15 months ago

Replying to fxbenard:

Can you send the pot over, I'll be happy to give it a try and check everything is working fine.

Sure, I uploaded the wp-admin and wp-frontend pot files here: https://cloudup.com/caCUNvosi9C
The relevant entries are only in wp-frontend.pot, they shouldn't be in wp-admin.pot anymore.

#40 @xibe
13 months ago

I'm not sure of the proper way of testing the POT.

Is it to check that the All and Pending strings are separated (because contextualized)? So far (within poEdit), doesn't seem to have changed anything. Am I missing something?

Thank you!

#41 @DrewAPicture
12 months ago

  • Owner set to rockwell15
  • Status changed from new to assigned

Assigning to mark the good-first-bug as "claimed".

This ticket was mentioned in Slack in #core-i18n by rockwell15. View the logs.


6 months ago

#43 @swissspidy
6 months ago

  • Keywords needs-refresh needs-testing added

@rockwell15
6 months ago

Refreshed patch & added the logic to 'post-state'

#44 @rockwell15
6 months ago

  • Keywords needs-refresh removed

#45 @swissspidy
6 months ago

  • Milestone changed from Future Release to 4.8

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


12 days ago

#47 @flixos90
12 days ago

  • Milestone changed from 4.8 to Future Release
Note: See TracTickets for help on using tickets.