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

Update TextDomain_Check #389

Merged
merged 6 commits into from
Sep 15, 2021
Merged

Update TextDomain_Check #389

merged 6 commits into from
Sep 15, 2021

Conversation

carolinan
Copy link
Collaborator

@carolinan carolinan commented Aug 18, 2021

This PR:

  1. Renames the file for the TextDomain_Check to follow the same format as the other files.
  2. Updates the error messages to show the line number where the error occurred. This is something that have been requested by theme authors repeatedly.

How to test:
In your test theme, include any or all of the translation functions in the rules array,
but without text domain or with an invalid amount of parameters.
For example, '_n' expects 3 or 4 parameters, and to test it, you would add only one or two.

Known issues:
The check is very slow.
This PR should not be merged until it's fast enough and any timeout issues have been solved.

@TeBenachi
Copy link
Contributor

Sorry if I’m missing some steps but the check did not indicate the line number nor the file name.
I tested with Twentytwenty-one theme, replacing twentytwentone to hello on 404.php and footer.php.

screenshot1

Also, this is very unlikely case, but I replaced all text domains on all files, except the one on the style.css. The test not only returned no warning but also confirmed the text domain was hello which was not correct.

screenshot2

@SergeyBiryukov
Copy link
Member

Thanks for the PR! It looks like it mentions showing the line number where the error occurred, but so far only shows the file name, which is a great start. Other changes and documentation improvements also look good to me.

Displaying the line number in other classes appears to be done via a call tc_grep() or tc_preg(). It's not immediately clear to me how to apply them here, so this could be a future enhancement.

I could not reproduce performance issues with this check yet, I guess I need a theme with more PHP files in it :) But it looks like as of commit 5eed747 for #370, each call to tc_filename() during the checks results in scanning the current theme for all PHP files, over and over again. This could probably be optimized with a static variable or in some other way.

Sorry if I’m missing some steps but the check did not indicate the line number nor the file name.
I tested with Twentytwenty-one theme, replacing twentytwentone to hello on 404.php and footer.php.

Thanks for testing! While these messages are related, there are more general and indeed don't include a file name or line number, as they can apply to multiple files at once.

This PR is about the other two, more specific messages in the same class:

  • Found a translation function that has an incorrect number of arguments in the file %1$s. Function %2$s, with the arguments %3$s
  • Found a translation function that is missing a text-domain in the file %1$s. Function %2$s, with the arguments %3$s

You should be able to see them if you pass only one argument to the __() function, or more than two arguments.

@kafleg
Copy link
Member

kafleg commented Aug 24, 2021

I see a similar message while checking with non-twenty themes. I haven't checked it with twenty- themes.

WARNING More than one text domain is being used in this theme. This means the theme will not be compatible with WordPress.org language packs. The domains found are master, kafleg, apple, new.

@carolinan
Copy link
Collaborator Author

carolinan commented Aug 24, 2021

@kafleg Please provide the code that is causing the error.
I can't see what you are testing against, and it is not clear if you are saying that the check works or that it does not work.
Please use the brackets to insert the code and not plain text or screenshot, thank you.

@carolinan
Copy link
Collaborator Author

I must have left out the line numbers while I was troubleshooting, I am re-adding them in a bit.

@carolinan
Copy link
Collaborator Author

carolinan commented Aug 24, 2021

I have re-added the line numbers, but the check is not accurate enough.
It shows every line that includes the "arguments" ( as in, the arguments/text inside the translation function) instead of only the line with the errors.

So the issue is with the string that is fed into tc_grep() on lines 121 and 161.


Test theme with known issues: https://themes.trac.wordpress.org/ticket/104185

Result:

WARNING: Found a translation function that is missing a text-domain in the file inc/admin/zoom-about-page/class-zoom-about-page.php. Function esc_attr_e, with the arguments 'plugin_slug'.
Line 558: if ( ! empty( $action_value['plugin_slug'] ) ) {
Line 560: $active = $this->check_if_plugin_active( $action_value['plugin_slug'], $action_value['file_name'] );
Line 563: $url    = ( $external_link ? $external_link : $this->create_action_link( $active['needs'], $action_value['plugin_slug'], $action_value['file_name'] ) );
Line 589: <p class='plugin-card-<?php esc_attr_e( $action_value['plugin_slug'] ) ?> action_button <?php echo ( $active['needs'] !== 'instal
Line 590: <a data-slug='<?php esc_attr_e( $action_value['plugin_slug'] ) ?>'

WARNING: Found a translation function that has an incorrect number of arguments in the file inc/admin/tgm-plugin/class-tgm-plugin-activation.php. Function esc_html__, with the arguments 'The following plugin was activated successfully:', The following plugins were activated successfully:, 'zoom-lite'.
Line 387: 'activated_successfully'          => __( 'The following plugin was activated successfully:', 'zoom-lite' ),
Line 2948: esc_html__( 'The following plugin was activated successfully:', 'The following plugins 

WARNING More than one text-domain is being used in this theme. This means the theme will not be compatible with WordPress.org language packs. The domains found are zoom-lite, The following plugins were activated successfully:.

Improve accuracy of the report by checking if the line includes both the translation function and the argument before displaying the report.
@carolinan
Copy link
Collaborator Author

carolinan commented Sep 13, 2021

Before
Multiple lines are showing incorrectly in the report:
Theme Check warning with incorrect code examples for multiple lines

After ad27e9c, only the line with the problem is showing:
Theme Check warning with the correct line

@kafleg
Copy link
Member

kafleg commented Sep 14, 2021

Worked for me!

Message I got while testing on different scenario.

WARNING: More than one text-domain is being used in this theme. This means the theme will not be compatible with WordPress.org language packs. The domains found are master, kafleg, apple, new.

WARNING: Found a translation function that is missing a text-domain in the file template-parts/content.php. Function esc_html__, with the arguments 'Pages:'. 
Line  40: 'before' => '<div class='page-links'>' . esc_html__( 'Pages:' ),

WARNING: Found a translation function that is missing a text-domain in the file template-parts/content-single.php. Function __, with the arguments 'testing'. L
ine  46: echo __('testing');

WARNING: Found a translation function that has an incorrect number of arguments in the file template-parts/content-single.php. Function __, with the arguments 'Continue reading"%s"', master, 'new'.  \
Line 29: __( 'Continue reading<span class='screen-reader-text'> '%s'</span>', 'master', 

@carolinan carolinan merged commit 667637e into master Sep 15, 2021
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.

4 participants