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.

Sign up for GitHub

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

Check for more mistakes in translatable strings #64

Merged
merged 12 commits into from
Aug 6, 2018
Merged

Check for more mistakes in translatable strings #64

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f521f3f
Check for more mistakes in translatable strings
swissspidy Jul 31, 2018
5c5c6a8
unshift -> shift
swissspidy Jul 31, 2018
3c8f91f
update file reference check
swissspidy Jul 31, 2018
c976d23
Remove duplicate parentheses
swissspidy Aug 3, 2018
369dcd9
Add some more tests
swissspidy Aug 3, 2018
4d1da99
Merge branch 'master' into 61-audit
swissspidy Aug 5, 2018
a8830ed
Use pluralize utility function
swissspidy Aug 5, 2018
f417336
PHP compat
swissspidy Aug 5, 2018
cbebdac
run -> try
swissspidy Aug 5, 2018
b58b69e
Fixes after code review
swissspidy Aug 5, 2018
a82ae9b
Merge branch 'master' into 61-audit
swissspidy Aug 6, 2018
03a4872
Flag strings with placeholders that should have translator comments
swissspidy Aug 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 108 additions & 4 deletions features/makepot.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,9 +605,6 @@ Feature: Generate a POT file of a WordPress project
/* translators: Translators 1! */
__( 'Hello World', 'foo-plugin' );

/* translators: Translators 1! */
__( 'Hello World', 'foo-plugin' );

/* Translators: Translators 2! */
__( 'Hello World', 'foo-plugin' );
"""
Expand All @@ -620,7 +617,114 @@ Feature: Generate a POT file of a WordPress project
"""
And STDERR should contain:
"""
Warning: The string "Hello World" has 2 different translator comments.
Warning: The string "Hello World" has 2 different translator comments. (foo-plugin.php:7)
"""

Scenario: Prints a warning for strings without translatable content
Given an empty foo-plugin directory
And a foo-plugin/foo-plugin.php file:
"""
<?php
/**
* Plugin Name: Plugin name
*/

sprintf( __( '"%s"', 'foo-plugin' ) );

"""

When I try `wp i18n make-pot foo-plugin --debug`
Then STDOUT should be:
"""
Plugin file detected.
Success: POT file successfully generated!
"""
And STDERR should contain:
"""
Warning: Found string without translatable content. (foo-plugin.php:6)
"""

Scenario: Prints a warning for missing singular placeholder
Given an empty foo-plugin directory
And a foo-plugin/foo-plugin.php file:
"""
<?php
/**
* Plugin Name: Plugin name
*/

sprintf(
_n( 'One Comment', '%s Comments', $number, 'foo-plugin' ),
$number
);

"""

When I try `wp i18n make-pot foo-plugin --debug`
Then STDOUT should be:
"""
Plugin file detected.
Success: POT file successfully generated!
"""
And STDERR should contain:
"""
Missing singular placeholder, needed for some languages. See https://developer.wordpress.org/plugins/internationalization/how-to-internationalize-your-plugin/#plurals (foo-plugin.php:7)
"""

Scenario: Prints a warning for placeholders in different order
Given an empty foo-plugin directory
And a foo-plugin/foo-plugin.php file:
"""
<?php
/**
* Plugin Name: Plugin name
*/

sprintf(
_n( '%1$s Comment (%2$s)', '%2$$s Comments (%1$s)', $number, 'foo-plugin' ),
Copy link
Member

Choose a reason for hiding this comment

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

double dollar sign here: %2$$s

$number,
$another_variable
);

"""

When I try `wp i18n make-pot foo-plugin --debug`
Then STDOUT should be:
"""
Plugin file detected.
Success: POT file successfully generated!
"""
And STDERR should contain:
"""
Singular and plural placeholder appear in different order. (foo-plugin.php:7)
"""

Scenario: Prints a warning for multiple unordered placeholders
Given an empty foo-plugin directory
And a foo-plugin/foo-plugin.php file:
"""
<?php
/**
* Plugin Name: Plugin name
*/

sprintf(
__( 'Hello %s %s', 'foo-plugin' ),
$a_variable,
$another_variable
);

"""

When I try `wp i18n make-pot foo-plugin --debug`
Then STDOUT should be:
"""
Plugin file detected.
Success: POT file successfully generated!
"""
And STDERR should contain:
"""
Multiple placeholders should be ordered. (foo-plugin.php:7)
"""

Scenario: Skips excluded folders
Expand Down
145 changes: 128 additions & 17 deletions src/MakePotCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,52 @@ class MakePotCommand extends WP_CLI_Command {
*/
protected $domain;

/**
* These Regexes copied from http://php.net/manual/en/function.sprintf.php#93552
* and adjusted for better precision and updated specs.
*/
const SPRINTF_PLACEHOLDER_REGEX = '/(?:
(?<!%) # Don\'t match a literal % (%%).
(
% # Start of placeholder.
(?:[0-9]+\$)? # Optional ordering of the placeholders.
[+-]? # Optional sign specifier.
(?:
(?:0|\'.)? # Optional padding specifier - excluding the space.
-? # Optional alignment specifier.
[0-9]* # Optional width specifier.
(?:\.(?:[ 0]|\'.)?[0-9]+)? # Optional precision specifier with optional padding character.
| # Only recognize the space as padding in combination with a width specifier.
(?:[ ])? # Optional space padding specifier.
-? # Optional alignment specifier.
[0-9]+ # Width specifier.
(?:\.(?:[ 0]|\'.)?[0-9]+)? # Optional precision specifier with optional padding character.
)
[bcdeEfFgGosuxX] # Type specifier.
)
)/x';

/**
* "Unordered" means there's no position specifier: '%s', not '%2$s'.
*/
const UNORDERED_SPRINTF_PLACEHOLDER_REGEX = '/(?:
(?<!%) # Don\'t match a literal % (%%).
% # Start of placeholder.
[+-]? # Optional sign specifier.
(?:
(?:0|\'.)? # Optional padding specifier - excluding the space.
-? # Optional alignment specifier.
[0-9]* # Optional width specifier.
(?:\.(?:[ 0]|\'.)?[0-9]+)? # Optional precision specifier with optional padding character.
| # Only recognize the space as padding in combination with a width specifier.
(?:[ ])? # Optional space padding specifier.
-? # Optional alignment specifier.
[0-9]+ # Width specifier.
(?:\.(?:[ 0]|\'.)?[0-9]+)? # Optional precision specifier with optional padding character.
)
[bcdeEfFgGosuxX] # Type specifier.
)/x';

/**
* Create a POT file for a WordPress plugin or theme.
*
Expand Down Expand Up @@ -381,34 +427,99 @@ protected function makepot() {
WP_CLI::error( $e->getMessage() );
}

$this->audit_strings();

$result = PotGenerator::toFile( $this->translations, $this->destination );

$translations_count = count( $this->translations );

WP_CLI::debug(
sprintf(
'Extracted %d %s',
$translations_count,
WP_CLI\Utils\pluralize( 'string', $translations_count )
),
'make-pot'
);

return $result;
}

/**
* Audits strings.
*
* Goes through all extracted strings to find possible mistakes.
*/
protected function audit_strings() {
foreach( $this->translations as $translation ) {
if ( ! $translation->hasExtractedComments() ) {
continue;
/** @var Translation $translation */

$references = $translation->getReferences();

// File headers don't have any file references.
$location = $translation->hasReferences() ? '(' . implode( ':', array_shift( $references ) ) . ')' : '';

if ( $translation->hasExtractedComments() ) {
$comments = $translation->getExtractedComments();
$comments_count = count( $comments );

if ( $comments_count > 1 ) {
WP_CLI::warning( sprintf(
'The string "%1$s" has %2$d different translator comments. %3$s',
$translation->getOriginal(),
$comments_count,
$location
) );
}
}

$comments = $translation->getExtractedComments();
$comments_count = count( $comments );
$non_placeholder_content = trim( preg_replace( '`^([\'"])(.*)\1$`Ds', '$2', $translation->getOriginal() ) );
$non_placeholder_content = preg_replace( self::SPRINTF_PLACEHOLDER_REGEX, '', $non_placeholder_content );

if ( $comments_count > 1 ) {
if ( '' === $non_placeholder_content ) {
WP_CLI::warning( sprintf(
'The string "%1$s" has %2$d different translator comments.',
$translation->getOriginal(),
$comments_count
'Found string without translatable content. %s',
$location
) );
}
}

$result = PotGenerator::toFile( $this->translations, $this->destination );
// UnorderedPlaceholders: Check for multiple unordered placeholders.
$unordered_matches_count = preg_match_all( self::UNORDERED_SPRINTF_PLACEHOLDER_REGEX, $translation->getOriginal(), $unordered_matches );
$unordered_matches = $unordered_matches[0];

$translations_count = count( $this->translations );
if ( $unordered_matches_count >= 2 ) {
WP_CLI::warning( sprintf(
'Multiple placeholders should be ordered. %s',
$location
) );
}

if ( 1 === $translations_count ) {
WP_CLI::debug( sprintf( 'Extracted %d string', $translations_count ), 'make-pot' );
} else {
WP_CLI::debug( sprintf( 'Extracted %d strings', $translations_count ), 'make-pot' );
if ( $translation->hasPlural() ) {
preg_match_all( self::SPRINTF_PLACEHOLDER_REGEX, $translation->getOriginal(), $single_placeholders );
$single_placeholders = $single_placeholders[0];

preg_match_all( self::SPRINTF_PLACEHOLDER_REGEX, $translation->getPlural(), $plural_placeholders );
$plural_placeholders = $plural_placeholders[0];

// see https://developer.wordpress.org/plugins/internationalization/how-to-internationalize-your-plugin/#plurals
if ( count( $single_placeholders ) < \count( $plural_placeholders ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Two different forms of count() used, one relative and one absolute.

WP_CLI::warning( sprintf(
'Missing singular placeholder, needed for some languages. See https://developer.wordpress.org/plugins/internationalization/how-to-internationalize-your-plugin/#plurals %s',
$location
) );
} else {
sort( $single_placeholders );
Copy link
Member

Choose a reason for hiding this comment

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

I'm probably missing something here, but I think it warrants a comment. The goal here is to verify the ordering of the placeholders. However, before comparing them, you sort them here. Would that put them into the same order no matter what?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know what I was thinking here. I copied this from WPCS, but somehow messed it up. The order should be irrelevant, but different placeholders are usually not ok.

sort( $plural_placeholders );

if ( $single_placeholders !== $plural_placeholders ) {
WP_CLI::warning( sprintf(
'Singular and plural placeholder appear in different order. %s',
$location
) );
}
}
}
}

return $result;
}

/**
Expand Down