Making WordPress.org

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#3601 closed defect (bug) (maybelater)

Wrong detection of localizable method in case of class

Reported by: mte90's profile Mte90 Owned by:
Milestone: Priority: normal
Component: Translate Site & Plugins Keywords: 2nd-opinion
Cc:

Attachments (2)

patch-proposal.zip (5.1 KB) - added by MattDotNet 6 years ago.
See https://meta.trac.wordpress.org/ticket/3601#comment:5
patch-3601.zip (5.8 KB) - added by MattDotNet 6 years ago.

Download all attachments as: .zip

Change History (14)

#1 @obenland
6 years ago

@ocean90 Do you if it's possible to have parsers ignore a line or skip a section? Is this even something for us to fix?

#2 @dd32
6 years ago

  • Keywords 2nd-opinion close added

As our parsers are regex based (?) I don't think it's really possible for us to add exceptions for 'same method name, different object' right now.

Unless the instances of *::$translation_functions() is high amongst plugins I don't think the benefit of excluding such strings outweigh the time involved with adding and managing exceptions to our parser rules right now unfortunately.

Having an extra string or two end up in translations isn't a huge issue, and no matter what translators translate it to, it won't have any effect anyway. (edit: well in this case it's more like 90+..)

Last edited 6 years ago by dd32 (previous) (diff)

#3 @Mte90
6 years ago

Maybe we can evaluate that if a class has the same methods of gettext is parsed anyway because maybe is a wrapper to avoid to put the textdomain.
Or in case that the textdomain is missing also if it is a class method is not parsed like in this case.

#4 @ocean90
6 years ago

  • Keywords close removed
  • Resolution set to maybelater
  • Status changed from new to closed

The code for the parser is in the WordPress core project. The relevant part of the extractor is here: https://core.trac.wordpress.org/browser/trunk/tools/i18n/extract.php?rev=36781#L145

If someone has patch for this please go ahead and create a new ticket on core.trac.wordpress.org for the patch. Thanks!

We could also think about switching to https://github.com/wp-cli/i18n-command for plugins and themes, assuming that it doesn't have this issue. /cc @swissspidy

#5 @MattDotNet
6 years ago

Hello everybody,

This topic was opened after our company reported the issue to @Mte90 for the translations used by our plugins.
We've followed what @ocean90 advised, and I built a possible patch for the StringExtractor class that resolves this issue. I would like you to review our proposal, hoping this will be helpful.

I'm attaching a ZIP package containing the modified file extract.php (all the modifications can be found by searching for @mod) and a file with the correct results of the test made for our plugin VikBooking (see first message).

My modification allows to ignore the translating functions that belong to classes which are out of context. For example:

<?php
// the function _ is ignored as it's part of the Html class
Html::_('checkbox', 1, false);

Additionally, in case certain functions belonging to specific classes should not be ignored, maybe because they are part of the gettext framework, it is possible to pass them within the class constructor as a second optional argument. For example:

<?php
$extractor = new StringExtractor($rules, ['Gettext', 'Translator']);

...

// this code will be ignored
Route::_($url);
// these lines of code will be added as new translation entries
Gettext::_('This string can be translated');
Translator::_('This string can be translated too');

That's all. Hopefully someone will review our proposal.

Please let us know your thoughts.

#6 @swissspidy
6 years ago

If someone has patch for this please go ahead and create a new ticket on core.trac.wordpress.org for the patch. Thanks!

@MattDotNet Would you mind making a proper patch with your modifications? See https://make.wordpress.org/core/handbook/tutorials/working-with-patches/. Happy to assist with that, too.

Also, I'm curious to know which of your plugins you have experienced this with and whether https://github.com/wp-cli/i18n-command works for you just as well.

#7 @MattDotNet
6 years ago

Would you mind making a proper patch with your modifications?

@swissspidy unfortunately I'm not an expert with the creation of patches via SVN. To get started, I downloaded from http://develop.svn.wordpress.org/trunk/ the whole project through SmartSVN (for Mac), then I edited the file /tools/i18n/extract.php and I created the patch file ticket3601.diff.
What am I supposed to do with this file?

I followed the instructions listed at https://make.wordpress.org/core/handbook/tutorials/working-with-patches/ but we cannot use that software.

Any advice would be appreciated.

Also, I'm curious to know which of your plugins you have experienced this with and whether https://github.com/wp-cli/i18n-command works for you just as well.

The plugin that is facing this issue is VikBooking: https://wordpress.org/plugins/vikbooking/.
Unfortunately we could not test it with wp-cli commands.

Thank you.

#8 @swissspidy
6 years ago

What am I supposed to do with this file?

You can either create a new ticket on core.trac.wordpress.org for updating the i18n tools, or just upload it here and I can do that for you.

Feel free to ping me on Slack if you have any questions.

By the way, if you're more comfortable in using Git, using that instead of SVN works just as fine. See https://make.wordpress.org/core/handbook/contribute/git/ for some examples.

The plugin that is facing this issue is VikBooking

Thanks, I'll test it against the WP-CLI command to see if it extracts all strings correctly. If so, we can focus on getting that onto wordpress.org instead of trying to fix the i18n-tools.

@MattDotNet
6 years ago

#9 @MattDotNet
6 years ago

You can either create a new ticket on core.trac.wordpress.org for updating the i18n tools, or just upload it here and I can do that for you.

@swissspidy Given the situation, I would rather have you do it for me (thanks). Next time I will definitely try using Git.

As you can see, I've just uploaded a ZIP file containing the modified file extract.php and the patch file related to this ticket (3601).

Thanks again.

#10 @swissspidy
6 years ago

For future reference, I just ran the VikBooking plugin through the WP-CLI wp i18n make-pot command.

  • The POT file from WordPress.org contains 2955 strings
  • The one from WP-CLI contains only 1652 strings, all of which are also part of the former
  • The strings mentioned in this ticket aren't being extracted
  • Also, The one from WP-CLI skips any strings without the vikbooking text domain (e.g. mod_vikbooking_horizontalsearch)

#11 @MattDotNet
6 years ago

Thanks for your tests.

It's sounds like the WP-CLI command works fine. This would solve the issue too, and if you will switch to this method there is no need I guess to apply the patch we made for the extractor class.
Also, we are going to replace the text domain of the widget (mod_vikbooking_horizontalsearch ) with the text domain of the plugin (vikbooking) as they share the same PO/MO file.

Do you know when and whether something will be changed? Because for the moment it is not possible to reach the 100% of the translation. Also, I guess once this method will be switched, the current translation strings will be updated for the plugin.

#12 @swissspidy
6 years ago

Do you know when and whether something will be changed?

No. There is no timeline.

I suggested using WP-CLI on WordPress.org now. You can follow the progress at #3748.

Note: See TracTickets for help on using tickets.