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

Don't use Constants for Text Domains #17

Merged
merged 2 commits into from Mar 24, 2020
Merged

Don't use Constants for Text Domains #17

merged 2 commits into from Mar 24, 2020

Conversation

@mitogh
Copy link
Contributor

mitogh commented Mar 21, 2020

Following the WordPress plugin developer handbook, removes the usage of a variable or constant for the text domain.

From the Handbook:

Do not use variable names or constants for the text domain portion of a gettext function. Do not do this as a shortcut: __( ‘Translate me.’ , $text_domain );

Reference: https://developer.wordpress.org/plugins/internationalization/how-to-internationalize-your-plugin/#text-domains

Following the WordPress plugin developer handbook, removes the usage of a variable or constant for the text domain.

From the Handbook:

> Do not use variable names or constants for the text domain portion of a gettext function. Do not do this as a shortcut: __( ‘Translate me.’ , $text_domain );

Reference: https://developer.wordpress.org/plugins/internationalization/how-to-internationalize-your-plugin/
@mitogh mitogh self-assigned this Mar 21, 2020
@mitogh mitogh requested review from bordoni and cliffordp Mar 21, 2020
@cliffordp

This comment has been minimized.

Copy link
Contributor

cliffordp commented Mar 21, 2020

@mitogh The WordPress.org system cannot process them so as long as we aren't publishing these to .org, we can use a variable. You can use the https://wordpress.org/plugins/piglatin/ plugin to see that it works, such as if someone wants to do their own customizations via PHP filter. I haven't tried with creating custom .po file but generating a .pot works with variables.

And, just FYI, @andrasguseo said reusing phrases from our main plugins, such as a string with 'tribe-common' text domain, does work for consistent translations when reusing. You can also see the discussion about variables for text domains here: DevinVinson/WordPress-Plugin-Boilerplate#59

@mitogh

This comment has been minimized.

Copy link
Contributor Author

mitogh commented Mar 21, 2020

Thanks for your feedback @cliffordp a couple of follow up questions / comments.

The WordPress.org system cannot process them so as long as we aren't publishing these to .org, we can use a variable

We definitely can, as this is not create a parsing error neither a warning inside of WordPress.

There are a couple of reasons why we should keep it as a plain strings.

  1. This is not a parsing issue in terms of WordPress processing.
  2. Using a constant for the text domain here does not provide much value as if you add a new string you still have to add the constant name anyways, which is the same cost.
  3. I don't think we are publishing the extensions to *.org, however I think we still should enforcing the usage of WP Standards in order to allow for consistency (IMHO) same as other code styles and such.
  4. Using the Standards here and everywhere makes things easier to follow are basically have the same convention across the ecosystem.
  5. Using a constant as a value for the text domain is mainly discouraged because the text is not extracted when using tools to generate the *.pot file using a tool like wp i18n make-pot
  6. Using a variable allow to use the translations from other Plugins Like TEC or ET for free as we can use text that has been already translated in other plugins, as this strings will be already translated and available to be used. The point mentioned from @andrasguseo
  7. WP CLI tool is used to generate files for transaltions in core https://meta.trac.wordpress.org/ticket/3748

Overall I would love to go with Constant as I agree it feels like the "right" approach to keep things DRY however I think in this particular case I would prefer to stick with WP Standards for the reasons mentioned above.

@cliffordp

This comment has been minimized.

Copy link
Contributor

cliffordp commented Mar 21, 2020

I disagree with (2) because it's very difficult to identify copy/pasta issues with text domain strings, especially when copying a snippet from the template then a different snippet from another extension and they all start with 'tribe-ext-...'

I tested (5) and Poedit works with the variable but WP-CLI's make-pot did not. I think this is the only really compelling reason.

(6) This is what I was trying to say Andras' testing showed... you can use the variable for your own extension's strings but if you want to borrow/replicate existing strings from core plugins, translating will work in both scenarios. So if you are replicating a string, don't use the variable; instead, use the same text domain as copied the string from in the core plugin.

As a template, I think there's a case for leaving it as-is, and the author of the extension can easily search-replace if they decide they need to run the WP CLI command to generate the .pot file. So maybe the README.md steps suggest using a string but don't require it. You could also provide the full CLI command for them, saying it only works if you set string text domains but say that Poedit works with variables for text domains:

wp i18n make-pot . languages/tribe-ext-extension-template.pot --headers='{"Report-Msgid-Bugs-To":"Modern Tribe, Inc. <https://support.theeventscalendar.com/>"}'

The point is that the end user won't be affected either way, as long as we correctly generate the .pot file.

@cliffordp

This comment has been minimized.

Copy link
Contributor

cliffordp commented Mar 21, 2020

@mitogh MattB and I DM'd in Slack and agrees we should use parseable text domains.

However, why fully remove the PLUGIN_TEXT_DOMAIN constant?

It can be used for load_plugin_textdomain() and tribe_notice(), for example.
(Maybe most appropriate to rename to something like PLUGIN_SLUG?)

@mitogh

This comment has been minimized.

Copy link
Contributor Author

mitogh commented Mar 23, 2020

Great.

  1. Prevent the usage of the constant as part of gettext functions in future strings, as this is mainly a template we need to provide a foundation of what the next values are.
  2. DRY is good but is not always ideal, there might be cases where repetition is fine, in this particular case I don't see there's a big problem due all the instances where this is used is in a single location.
@mitogh mitogh merged commit 5c3c2e7 into master Mar 24, 2020
@mitogh mitogh deleted the fix/text-domains branch Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.