WordPress / theme-check

Theme Check plugin
https://wordpress.org/plugins/theme-check/
341 stars 114 forks source link

Update TextDomain_Check #389

Closed carolinan closed 3 years ago

carolinan commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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:

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

kafleg commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

carolinan commented 3 years ago

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:.
carolinan commented 3 years ago

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 commented 3 years ago

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',