Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Translatability fixes #1858

Merged
merged 36 commits into from
Apr 28, 2020
Merged

Translatability fixes #1858

merged 36 commits into from
Apr 28, 2020

Conversation

benlk
Copy link
Collaborator

@benlk benlk commented Apr 23, 2020

Changes

This pull request makes the following changes:

  • reduces Largo to a single text domain: largo
  • adds text domains to a whole heap of translation functions
  • adds new translation to a few places
  • removes some variables from translation strings, or removes the translation outright
  • adds translators: notes for strings translated for use with sprintf/printf, based on the output of grunt pot

This PR does not include:

  • an exhaustive search for cases where variables relevant to the translated text are appended to the translated text outside of the translation function, such as this example: <php echo esc_html__( 'My name is', 'largo' ); . ' ' . $name ?>

Why

For #1592 and INN/umbrella-caribbean#96

Testing/Questions

Features that this PR affects:

  • internationalization

Questions that need to be answered before merging:

  • Is this PR targeting the correct branch in this repository?
  • Are there other errors of translation that need to be fixed?

Steps to test this PR:

  1. Run grunt pot. Expect no error messages.
  2. Run the Theme Check plugin against Largo. Expect no translation-related error messages other than those noted below.

WARNING: Found a translation function that is missing a text-domain. Function __, with the arguments 'largo'

I can't find this one. Good luck if you're going hunting.

$this->lib_e(

The theme check plugin appears to think lib_e is a translation function. It's not; these results are a false positive.

Possible variable $s found in translation function in inc/widgets/largo-twitter.php. Translation function calls must NOT contain PHP variables.

Line 34: __( '@%1$s's Likes on Twitter', 'largo' ),

That's the right format for passing a variable to sprintf, right?

benlk added 22 commits April 22, 2020 22:06
- provide `translators: ` notes
- make date formates translatable for archive title
- replace the deprecated urlencode with rawurlencode
- date archives don't return an object for `get_queried_object()`, so
in that case we shouldn't be trying to get the term meta post
- better formatting for the date archive dropdown picker
- replace an endwhile with {}
- remove translation of post_type_archive_title() as translation for
that function's output is handled during post type registration.
- improved escaping of output
- punctuate inline comments
- add a translators: note to an error condition in co-authors
- make the output generator for coauthors links more readable via
sprintf
@benlk benlk requested a review from joshdarby April 23, 2020 23:49
@benlk benlk self-assigned this Apr 23, 2020
@benlk benlk marked this pull request as ready for review April 24, 2020 01:22
@joshdarby
Copy link
Collaborator

Possible variable $s found in translation function in inc/widgets/largo-twitter.php. Translation function calls must NOT contain PHP variables.

Line 34: __( '@%1$s's Likes on Twitter', 'largo' ),

That's the right format for passing a variable to sprintf, right?

Yea. I think we can ignore this one because it thinks $s is a variable in this case even though it's not.

@joshdarby
Copy link
Collaborator

WARNING: Found a translation function that is missing a text-domain. Function __, with the arguments 'largo'

I can't find this one. Good luck if you're going hunting.

@benlk I went hunting and commented out all __ functions in inc/post-social.php and inc/widgets/largo-twitter.php and that removed the warning. Not sure why that warning is being thrown though because all cases have 2 args like they should

Copy link
Collaborator

@joshdarby joshdarby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work as expected. 👍

@benlk
Copy link
Collaborator Author

benlk commented Apr 28, 2020

commented out all __ functions in inc/post-social.php and inc/widgets/largo-twitter.php and that removed the warning

With the following comments, the warning goes away:

diff --git a/inc/post-social.php b/inc/post-social.php
index aad372cd..47c81099 100644
--- a/inc/post-social.php
+++ b/inc/post-social.php
@@ -34,11 +34,13 @@ if ( ! function_exists( 'largo_post_social_links' ) ) {
 
 		if ( isset( $utilities['facebook'] ) && '1' === $utilities['facebook'] ) {
 			$fb_share = '<span class="facebook"><a target="_blank" href="https://www.facebook.com/sharer/sharer.php?u=%1$s"><i class="icon-facebook"></i><span class="hidden-phone">%2$s</span></a></span>';
+			/*
 			$output .= sprintf(
 				$fb_share,
 				rawurlencode( get_permalink() ),
 				__( ucfirst( of_get_option( 'fb_verb' ) ), 'largo' )
 			);
+			*/
 		}
 
 		if ( isset( $utilities['twitter'] ) && '1' === $utilities['twitter'] ) {
diff --git a/inc/widgets/largo-twitter.php b/inc/widgets/largo-twitter.php
index fa41913f..129c815c 100644
--- a/inc/widgets/largo-twitter.php
+++ b/inc/widgets/largo-twitter.php
@@ -31,7 +31,7 @@ class largo_twitter_widget extends WP_Widget {
 				$widget_href = 'https://twitter.com/' . $instance['twitter_username'] . '/likes';
 				$widget_text = sprintf(
 					// translators: @username's Likes on Twitter.
-					__( "@%1$s's Likes on Twitter", 'largo' ),
+					// __( "@%1$s's Likes on Twitter", 'largo' ),
 					esc_html( $instance['twitter_username'] )
 				);
 				break;
@@ -39,7 +39,7 @@ class largo_twitter_widget extends WP_Widget {
 				$widget_href = 'https://twitter.com/' . $instance['twitter_username'] . '/lists/' . $instance['twitter_list_slug'];
 				$widget_text = sprintf(
 					/* translators: A Twitter List by [twitter user name] */
-					__( 'A Twitter List by %1$s', 'largo' ),
+					// __( 'A Twitter List by %1$s', 'largo' ),
 					$instance['twitter_username']
 				);
 				break;
@@ -52,7 +52,7 @@ class largo_twitter_widget extends WP_Widget {
 				/* translators: Tweets by @username */
 				$widget_text = sprintf(
 					// translators: %1$s is the twitter username
-					__( 'Tweets by @%1$s', 'largo' ),
+					// __( 'Tweets by @%1$s', 'largo' ),
 					esc_html( $instance['twitter_username'] )
 				);
 		}

It's tripping over:

  • output of a function
  • %1$s on a sprintf when there's only one argument to be interpolated

@benlk
Copy link
Collaborator Author

benlk commented Apr 28, 2020

With those two latest commits, it now completes without those two error messages.

@benlk benlk merged commit 5901819 into trunk Apr 28, 2020
@benlk benlk deleted the 1592-translatability-fixes branch April 28, 2020 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants