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

t10ns #2

Closed
richardsweeney opened this Issue Oct 11, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@richardsweeney
Copy link

richardsweeney commented Oct 11, 2016

Hi, class looks great and works well, but I haven't managed to get the t10ns working.

I added the this via composer to my theme (if i require it to my vendor dir, the t10ns won't be found). I could translate all the strings ok, but the t10ns don't actually work.

The issue is unfortunately with using a variable for the text-domain for t10ns. Otto sums it up nicely in the following article (it's old but still relevant).

http://ottopress.com/2012/internationalization-youre-probably-doing-it-wrong/

Basically, the get_text function suite doesn't parse PHP. It only searches through the code for __() _e() etc. The textdomain must therefore be a string.

Did you ever get this working with a provided textdomain for the object instance?

<?php
    $case = new PostType('case');
    $case->translation('shipyard');
?>

As i said, I can grab the t10ns via poedit, but the translations are not show in the backend.

@jjgrainger

This comment has been minimized.

Copy link
Owner

jjgrainger commented Oct 11, 2016

@richardsweeney I believe the translation (when passing the textdomain for the object instance) works with a few translations plugins.

But, as for working with poedit, I think the only way it can work with the class is supplying the translation strings in the arguments.

So for example, to set up the post type labels for translation with poedit, you would do the following:

$args = [
  'labels' => [
    'name' => __('Cases', 'shipyard'),
    'singular_name' => __('Case', 'shipyard'),
    'add_new_item' => __('Add New Case', 'shipyard')
    // ... additional labels here
  ]
];

$case = new PostType('case', $args);

I know its additional work on the developers part, but seeing how get_text doesn't parse PHP I don't see any other way around it, unfortunately :(

Thanks for the article 👍 . There are definitely parts of the class that can be fixed following the articles advice.

@jorisvm

This comment has been minimized.

Copy link
Contributor

jorisvm commented May 29, 2017

The translation is still an issue. Not the gettext compatibility, but simply getting a valid .mo file to work with this class.

I ran into it, and it comes down execution order. The name and label strings are initialized on class construction. In other words you can call the PostType->translation() method but it is always to late. I made some modifications to the code to make it work by calling PostType->names() and PostType->labels() on init instead of on construct and it does the trick. I suppose I could send it as a pull request if you want me to.

@jjgrainger

This comment has been minimized.

Copy link
Owner

jjgrainger commented Oct 11, 2017

I've been researching into this more while working towards releasing v2.0 and I've decided to remove anything to do with translations from the project.

With the current setup, I seem to be encouraging bad practices around translations such as using a variable for the text domain and more. Likewise, PostTypes isn't a plugin but a developer utility. Therefore, I believe it should not have its own text domain or translate strings itself.

For v2.0, I plan to remove anything to do with translations and instead provide simple methods to set labels and other strings that need translations. This way the developer will have more control and be able to implement translations properly.

@jorisvm

This comment has been minimized.

Copy link
Contributor

jorisvm commented Oct 12, 2017

Thanks for the reply,
I understand that using a variable for translations may not be best practice, but it would be real nice if you could somehow tell the class to use the textdomain of the theme or plugin for which the posttype class is used. For plugin or theme development centralizing all translations in 1 po file and one text domain is crucial. Maybe create a way to set all translation defaults from outside the class?

$books = new PostType(array(
            'name'  => 'book',
            'singular' => __('Book', 'my-plugin'),
            'plural' => __('Books', 'my-plugin'),
            'slug' => 'book'
));

$books->setTranslation(array(
            'add_new' => __('Add New', 'my-plugin'),
            'add_new_item' => sprintf(__('Add New %s', 'my-plugin'), $books->singular),
            'edit_item' => sprintf(__('Edit %s', 'my-plugin'), $books->singular),
            'new_item' => sprintf(__('New %s', 'my-plugin'), $books->singular),
            'view_item' => sprintf(__('View %s', 'my-plugin'), $books->singular),
            'search_items' => sprintf(__('Search %s', 'my-plugin', $books->plural),
            'not_found' => sprintf(__('No %s found', 'my-plugin'), $books->plural),
            'not_found_in_trash' => sprintf(__('No %s found in Trash', 'my-plugin'), $books->plural),
            'parent_item_colon' => sprintf(__('Parent %s:', 'my-plugin'), $books->singular)
));

Just a thought. Not exactly fast code to write, but it would prevent using a variable textdomain.

Looking forward how this wil workout in the 2.0 version. Keep up the good work.

@jjgrainger jjgrainger referenced this issue Oct 24, 2017

Merged

v2.0 #18

@jorisvm

This comment has been minimized.

Copy link
Contributor

jorisvm commented Nov 13, 2017

Nicely solved in 2.0!

@jjgrainger

This comment has been minimized.

Copy link
Owner

jjgrainger commented Nov 15, 2017

@jorisvm Thanks!

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