Closed janhn closed 3 years ago
Thank you for your contribution! I think the code looks ok but I have worries about changing the text domain. I am guessing anyone that has implemented other languages on the current codebase will lose it and revert back to English?
I suppose that's possible, but it seems unlikely that this change would affect many.
Someone like that would also have to make the changes to run load_textdomain()
etc. and then, why not contribute the fix? Reading #38 I'd expect that for majority of users, if not all, there's no i18n/l10n at all.
@janhn Thanks a lot!
I think the question @Kukks had is what happens to already translated strings via e.g. WPML?
Example, we currently have:
$order->add_order_note(__('BTCPay invoice payment completed. Payment credited to your merchant account.', 'btcpay'));
If somebody used any translation tool like WPML, and translated it already to spanish, will your change revert that change back to english if there is no .po yet?
I see you overwrite also the textdomain from 'btcpay' to 'btcpay-for-woocommerce', not sure but I think this will make all WPML translations not match anymore?
Assume the textdomain does not affect the WPML translations. If existing WPML translations are there AND also a plugin provided .po file, which will take precendence? The WPML translation or will this change force translationes provided by the plugin?
@Kukks @ndeet Thank you for the review.
@janhn Thanks a lot!
I think the question @Kukks had is what happens to already translated strings via e.g. WPML?
Understood, I just didn't assume any translation plugin specifics. I'm not familiar with WPML, so I can't rule out breakage. To make it work, existing localization files (.po, .mo) only need to be renamed to match the textdomain.
Ideally, such custom translations should be shipped along with the plugin, so it would be best to allow users to contribute. I'm hoping that these changes will allow that. Currently, I'm seeing This plugin is not properly prepared for localization.
at https://translate.wordpress.org/projects/wp-plugins/btcpay-for-woocommerce/.
Seems like there's a choice to make: avoid possibly breaking existing custom translations (how many are there?) or fix the i18n of the plugin for any language and get the opportunity to ship the plugin including l10n in the future.
Can the textdomain change be communicated in the ChangeLog of new version?
@Kukks @ndeet Thank you for the review.
@janhn Thanks a lot! I think the question @Kukks had is what happens to already translated strings via e.g. WPML?
Understood, I just didn't assume any translation plugin specifics. I'm not familiar with WPML, so I can't rule out breakage. To make it work, existing localization files (.po, .mo) only need to be renamed to match the textdomain.
Ideally, such custom translations should be shipped along with the plugin, so it would be best to allow users to contribute. I'm hoping that these changes will allow that. Currently, I'm seeing
This plugin is not properly prepared for localization.
at https://translate.wordpress.org/projects/wp-plugins/btcpay-for-woocommerce/.Seems like there's a choice to make: avoid possibly breaking existing custom translations (how many are there?) or fix the i18n of the plugin for any language and get the opportunity to ship the plugin including l10n in the future.
Can the textdomain change be communicated in the ChangeLog of new version?
Sounds good!
Fix the Text Domain https://developer.wordpress.org/plugins/internationalization/how-to-internationalize-your-plugin/
Create localization template under
languages/
subdirectory https://developer.wordpress.org/cli/commands/i18n/make-pot/Add
btcpay_load_textdomain()
to enable localizationShould help with fixing #38
Tested by creating a custom l10n:
btcpay-for-woocommerce-es_ES.po
)$ wp i18n make-mo btcpay-for-woocommerce-es_ES.po
And perhaps the Translation BUI at https://translate.wordpress.org/projects/wp-plugins/btcpay-for-woocommerce/ will become available automatically with this.