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

Dynamic textdomain will not work #2

Open
schlessera opened this issue Feb 21, 2017 · 7 comments
Open

Dynamic textdomain will not work #2

schlessera opened this issue Feb 21, 2017 · 7 comments

Comments

@schlessera
Copy link
Contributor

The classes have their textdomain for localization injected through the constructor.

This will not work in all instances, as gettext() only works by checking for simple strings, it does not evaluate the PHP code to get runtime information.

Also, this does not really make sense, as multiple plugins using the messages would then provide multiple, partially conflicting translations, and each one needs to localize for all languages on their own.

I suggest providing default messages with a fixed textdomain that will be centrally localized by the community, with a mechanism to override them only when needed.

Out of the gate I can think of two possible ways of having the messages be localizable:

  1. Inject a type of config file into the class that contains identifier => strings combinations.
  2. Have the class be extended to provide localizations through either overrideable properties or methods.
@atimmer
Copy link
Contributor

atimmer commented Feb 23, 2017

It does work this way right now for the license manager: https://github.com/Yoast/License-Manager/blob/master/class-product.php#L72. And the strings are picked up by makepot.php in wordpress-seo so that is the way we have done it previously. Yes gettext only works with a hardcoded textdomain, but makepot.php parses these anyway.

I agree that this is not the most optimal way. How would you design the tooling around your first solution? The current way of translating is a pragmatic choice because it fits in the existing tooling.

@schlessera
Copy link
Contributor Author

Something like this (coded in the GitHub comment box, so not tested or even sane in any way):

<?php
return array(
    // Message identifiers should be defined as interface constants somewhere.
    Whip_Message::MESSAGE_IDENTIFIER_1 => __( 'This is a localized string.', 'whip' ),
    Whip_Message::MESSAGE_IDENTIFIER_2 => __( 'This is another localized string.', 'whip' ),
    // [...]
);

Then, in the file where you want to make use of the messages:

<?php
// We can now inject a config.
$default_config_path = dirname( __FILE__ ) . '/message_strings.php';
$config_path = apply_filters( 'whip_config_path', $default_config_path );

if ( ! is_readable( $config_path ) ) {
    $config_path = $default_config_path;
}

ob_start();
include $config_path;
$config = ob_get_clean();

$php_messages = new PHPMessages( $config );
$message = PHPMessages->get_from( $failed_requirement );

@schlessera
Copy link
Contributor Author

If someone else wants to provide a different set of messages, they can provide an alternate config file (with their own textdomains) and inject it via the filter.

Otherwise, every plugin will use the default messages with your whip textdomain, so that these only need to be translated once, instead of for every single plugin.

@schlessera
Copy link
Contributor Author

You could even improve upon this and load both the default and the custom config and merge both, so you can only override the defaults partially, while still falling back to the defaults where you don't provide a custom config.

@schlessera
Copy link
Contributor Author

To make the code cleaner, you can add a Config class that is responsible for loading (and optionally merging) config files.

@schlessera
Copy link
Contributor Author

schlessera commented Feb 23, 2017

See https://github.com/brightnucleus/config-52 for an extremely basic implementation, as inspiration.
That one is PHP 5.2 compatible.

@w3guy
Copy link

w3guy commented Mar 6, 2017

I was exactly here to report same issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants