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

[Implement Sniff] Check that only one - or at most two - text-domains are used in the theme #35

Open
2 tasks
jrfnl opened this issue Jul 18, 2016 · 22 comments

Comments

@jrfnl
Copy link
Contributor

jrfnl commented Jul 18, 2016

Rule type:

Error

Rule:

From what I can see, there are actually 5 distinct i18n related rules which may need sniffs - this issue covers the third item on the list.

  1. All theme text strings are to be translatable. (Check that all text strings are translated and in the same language #33)
  2. Include a text domain in style.css ([New Sniff] Check that style.css contain a text-domain entry. #34)
  3. Use a single unique theme slug – as the theme slug appears in style.css. If it uses a framework then no more than 2 unique slugs.
  4. Can use any language for text, but only use the same one for all text. (Check that all text strings are translated and in the same language #33)
  5. Correct usage of the WP i18n functions. ([Implement sniff] Verify that the i18n functions are used properly #31)

Refs:
https://make.wordpress.org/themes/handbook/review/required/#language
https://make.wordpress.org/themes/handbook/review/required/theme-check-plugin/#additional-checks

This requires a slightly different implementation, the existing WordPress.WP.I18n sniff can check whether all i18n functions use the same text domain. For that check, the text domain - at this moment - will have to be provided via the phpcs config file or programmatically. I suggest this text domain injection into the PHPCS configuration will be implemented in the Theme Check wrapper which will call PHPCS in order to enable this check.

Note: it is being investigate upstream whether the text-domain can be determined from the files. If that would be implemented, no further action would be needed for the Theme Check plugin.

Currently the WordPress.WP.I18n sniff only checks against the one text domain and does not keep track of the found text domains (other than reporting them in the error message)

Theme check file covering this rule:

https://github.com/Otto42/theme-check/blob/master/checks/textdomain.php

Decision Needed:

Request for decision: How should we deal with extra (allowed) text-domains ? Is there a white-list of extra text-domains related to frameworks which can be checked against ?

To do:

  • Potentially implement passing the text domain to PHPCS in Theme Check.
  • Potentially extend the existing WordPress.WP.I18n sniff to keep a count of all encountered text-domains and report if the count is more than two.
@justintadlock
Copy link

Request for decision: How should we deal with extra (allowed) text-domains ? Is there a white-list of extra text-domains related to frameworks which can be checked against ?

I'm in favor of creating a whitelist of allowed extra textdomains (or textdomains to ignore). I'd be happy to gather that list if the team agrees.

I think that's a far better approach. And, we can always add new textdomains to the list if/when another framework pops up that needs to be accounted for.

@justintadlock
Copy link

Here's the current list of exceptions (Twenty* textdomains copied from TC). I've sent out a few DMs to known framework authors to get their feedback.

var $exceptions = array( 

    // Core themes
    'twentyten', 
    'twentyeleven',  
    'twentytwelve',  
    'twentythirteen',  
    'twentyfourteen',  
    'twentyfifteen',  
    'twentysixteen',  
    'twentyseventeen',  
    'twentyeighteen', 
    'twentynineteen',  
    'twentytwenty',

    // Frameworks
    'hybrid-core'
);

@grappler
Copy link
Member

@justintadlock Did you get any feedback from others?

@justintadlock
Copy link

Here's a possible list that aristath and dovy help put together. I do not know whether these frameworks properly handle their own textdomains and/or language files when included in a theme though. That'd be the only reason we'd want to whitelist them.

redux-framework
tgmpa
nhp
smof
optiontree (option-tree?)
vp_textdomain
ACF
titan-framework
kirki
upthemes
options-framework

@CryoutCreations
Copy link

We'd also like to have our framework added to this list: "cryout" that we use in all our currently live themes.

Seeing the list Justin submitted I'd also have a far-fetched proposal, at least for the theme specific frameworks (tgmpa is not really a theme framework, is it?). They should all have a standard naming format (something like "name-framework") so it would be easier to identify and recognize when editing or reviewing themes.

Also, something like "options-framework" seems far too generic, like having a "theme" text domain. And what if somebody uses one of these "whitelisted" textdomains in their themes for other purposes, unrelated to the frameworks. How will Theme Check sniff this out?

And on the "no more than 2 theme domains" rule. What if you're using a theme framework and tgmpa for suggesting plugins? You'll be forced to either let one go or rename it, kind of missing the whole point of using frameworks and/or WordPress translations.

@grappler
Copy link
Member

  • We do not need to whitelist tgmpa as the generator automatically changes it to the themes slug.
  • smof has been deprecated and is not supported any more so I would against whitelisting it.
  • I would be against the vafpress-framework as it is a Theme Options Builder, Metaboxes Builder and Shortcode Generator Builder
  • option-tree, Options Framework and upthemesdo not support the customizer so they are out of the question
  • I do not know what nhp is.
  • If we allow ACF then we would need to allow cmb2 too.

I do not know whether these frameworks properly handle their own textdomains and/or language files when included in a theme though. That'd be the only reason we'd want to whitelist them.

We must have a requirement that the framework needs to fulfill to be whitelisted. e.g. "The framework must handle the text domain correctly and include translation files."

And on the "no more than 2 theme domains" rule. What if you're using a theme framework and tgmpa for suggesting plugins?

With a whitelist I expect we can will not need a limit on the number of text domains.

@cristianraiber
Copy link

Here's another one: mte - it's our own framework, not published yet, but we've been silently working on a it for our own themes (will be using the Customizer API)

@justintadlock
Copy link

justintadlock commented Oct 4, 2016

We must have a requirement that the framework needs to fulfill to be whitelisted. e.g. "The framework must handle the text domain correctly and include translation files."

I wanted to touch on this because there are 2 separate methods for making this work.

  1. The framework must load its own textdomain with load_textdomain() and include the translation files within it.

  2. The framework must load its own textdomain with load_textdomain() but merge the strings with the themes. Method for doing this: https://gist.github.com/justintadlock/7a605c29ae26c80878d0

If the framework is not employing one of these two methods (can't think of any others), translations will not work.

Edit: So, basically, if you want the framework whitelisted, you need to show that it's handling translations.

@rinkuyadav999
Copy link

Here's another one: meta-box

@grappler
Copy link
Member

grappler commented Oct 5, 2016

@cristianraiber mte seems a bit short. You need to show that it is handling translations is included.

@rinkublog Please could you give a bit more information like where the framework can be found and show that it is handling translations.

@rinkuyadav999
Copy link

rinkuyadav999 commented Oct 5, 2016

@grappler here is plugin https://wordpress.org/plugins/meta-box/ and i think here it is handling translation https://plugins.svn.wordpress.org/meta-box/trunk/inc/core.php

But theme check: WARNING: The theme uses the add_shortcode() function. Custom post-content shortcodes are plugin-territory functionality. for this plugin.

@cristianraiber
Copy link

cristianraiber commented Oct 5, 2016

@grappler - you're right; how about epsilon-framework ? Would that be considered ok? I'll take care of the translations part as per @justintadlock suggestions above.

@grappler
Copy link
Member

grappler commented Oct 5, 2016

Yes, that makes it clear what it is.

@cristianraiber
Copy link

@grappler - great, thanks!

@grappler grappler self-assigned this Oct 19, 2016
@templatemonster
Copy link

We'd like to have added to white list our Cherry Framework with cherry-framework text-domain.

@cristianraiber
Copy link

Any news about this guys? Theme upload faiks because of duplicate text-domains

@grappler
Copy link
Member

@cristianraiber If you are running your own checks you can define the text domains. This is explained here: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#internationalization-setting-your-text-domain

If you are talking about the new Theme Sniffer plugin, then you can contribute a file that will always exists in your framework and that we can check exists.
https://github.com/WPTRT/theme-sniffer/blob/master/inc/admin.php#L220-L228

@cristianraiber
Copy link

cristianraiber commented Jun 30, 2017

@grappler - makes sense re. the new Theme Sniffer plugin, but I was under the impression that both theme check plugins will exist side by side... How do I get our text-domain approved in both?

@grappler
Copy link
Member

@cristianraiber I don't think Theme Check has a text-domain whitelist. For Theme Sniffer you just make a pull request.

@justintadlock
Copy link

Just noting that I've confirmed epsilon-framework will work and can be white-listed.

@dingo-d
Copy link
Member

dingo-d commented May 18, 2019

I think that prefix all globals sniff covers the use-case of multiple text domains.

@jrfnl @justintadlock can you confirm this? If it's the case, we can close this issue 🙂

@jrfnl
Copy link
Contributor Author

jrfnl commented May 19, 2019

I think that prefix all globals sniff covers the use-case of multiple text domains.

Nope, different sniff, not the same thing.

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

8 participants