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

Plugin i18n check complains about default text domain #713

Closed
felixarntz opened this issue Oct 10, 2024 · 14 comments · Fixed by #722
Closed

Plugin i18n check complains about default text domain #713

felixarntz opened this issue Oct 10, 2024 · 14 comments · Fixed by #722
Labels
Checks Audit/test of the particular part of the plugin [Team] Plugin Review Issues owned by Plugin Review Team [Type] Bug An existing feature is broken

Comments

@felixarntz
Copy link
Member

I just noticed that as of 1.2.0, the Plugin Check complains about usage of the default text domain in plugins. I believe that this is not correct. It is explicitly allowed to use WordPress Core's text domain if it is for strings that are taken directly from Core. Not doing so creates unnecessary work for translators.

It looks like the relevant change was made in #681. I believe we need to update that code so that in addition to the plugin's own text domain we pass default there so that it is allowed too.

Here's the example where I noticed this check suddenly failing: https://github.com/felixarntz/ai-services/actions/runs/11282317040/job/31379501989?pr=11

It is worth noting that this was also automatically flagged during the review of that plugin when I submitted it to the directory, however after clarification it was allowed. So this confirms to me that this is simply a bug in the tooling, and we should allow default.

cc @davidperezgar @ernilambar

@felixarntz felixarntz added [Team] Plugin Review Issues owned by Plugin Review Team [Type] Bug An existing feature is broken Checks Audit/test of the particular part of the plugin labels Oct 10, 2024
@swissspidy
Copy link
Member

swissspidy commented Oct 10, 2024

It is explicitly allowed to use WordPress Core's text domain if it is for strings that are taken directly from Core.

Explicitly allowed, or just tolerated? Where is this written?

If it‘s not written anywhere I‘d suggest the plugins team to do that first.

Not doing so creates unnecessary work for translators.

Nowadays this is not really true anymore. Translators can simply copy those strings with a single click.

FWIW, personally I have never really been a fan of reusing core strings, as it has more downsides than upsides.

@felixarntz
Copy link
Member Author

This is part of WPCS for example: https://github.com/WordPress/WordPress-Coding-Standards/blob/2133137c33fa898df70b5f879a65d83af4dbb97d/WordPress/Sniffs/WP/I18nSniff.php#L479

Nowadays this is not really true anymore. Translators can simply copy those strings with a single click.

That's still more work than doing nothing. Why would we want people to copy translations from somewhere else when we can reuse existing ones that are established?

FWIW, personally I have never really been a fan of reusing core strings, as it has more downsides than upsides.

What are the downsides? 🤔

@swissspidy
Copy link
Member

  • It makes it impossible to filter/modify a plugin‘s translations because the strings are part of a different language pack.
  • The core string can change between releases, breaking your usage.
  • Not possible to do this for JS translations, bit a dev might do it nonetheless without realizing that it doesn‘t work.
  • The core string might have different meaning/context and the dev might use it incorrectly, causing confusion for users.
  • Confusing and limiting for translators as not all strings can be translated by them

I could go on :-)

Point is, using core strings should be a really rare thing that one should only do if they know the consequences. That means PCP should still warn about it. You can ignore the warnings or skip the check in that case.

@felixarntz
Copy link
Member Author

Those are fair points. Though if you know what you're doing, some of the points may not actually be downsides.

Per your last note, we should probably adjust this to be a warning then. Right now, using the default text domain produces an error, and that it's definitely not.

@frantorres
Copy link
Contributor

In this respect there are two casuistries:

  • Plugin authors using the default text domain consciously.
  • Plugin authors using the default text domain accidentally, perhaps because they forgot to set it up or don't know how it works.

One of the solutions might be to check if the string to translate is really part of the core translations. If it is: Warning? , if it isn't: Error.

The existence of the translated string could perhaps be checked directly with a call to WordPress core? In any case since the core strings can be easily exported to a PHP array, that could be another possibility.

@swissspidy
Copy link
Member

Good point, it‘s an important distinction.

The existence of the translated string could perhaps be checked directly with a call to WordPress core? In any case since the core strings can be easily exported to a PHP array, that could be another possibility.

Interesting idea, but that requires extra maintenance of such a (giant!) PHP file somewhere in the plugin. And strings change a lot between releases, so this is infeasible IMO.

I‘d say:

  • No domain = error
  • using "default" = warning (so it doesn‘t block plugin submission)
  • using anything other than the plugin slug = error

@davidperezgar
Copy link
Member

I think is a rare case of using default for i18 textdomain. I haven't seen before even all the time reviewing plugins. If you see @felixarntz is important, the idea to make it as warning could work.

@felixarntz
Copy link
Member Author

To make use of the default text domain a warning, can we do that via some configuration to WPCS, e.g. in our ruleset? Or do we need to use some hacky workaround, e.g. check whether the error message refers to the default text domain and if so downgrade it to a warning?

@swissspidy
Copy link
Member

I don't think there's a configuration for this (and I doubt WPCS would add one), so yeah probably need to parse the error message.

@ironprogrammer
Copy link

ironprogrammer commented Oct 12, 2024

Update: On closer examination, this behavior is due to this plugin being in the plugins/ root, rather than its own folder. My revised assessment is that this is expected for any other plugin.

I'm also seeing an unexpected WordPress.WP.I18n.TextDomainMismatch with different results between the UI and CLI. For Hello Dolly (from the plugin repo, not Core, which doesn't include a text domain), provided:

__( 'Quote from Hello Dolly song, by Jerry Herman:', 'hello-dolly' ),

The admin shows
Mismatched text domain. Expected 'plugins' but got 'hello-dolly'.
whereas the CLI returns
Mismatched text domain. Expected 'hello.php' but got 'hello-dolly'.

Seems related, but if this should be an entirely different ticket, please let me know.

@swissspidy
Copy link
Member

That indeed sounds like it warrants its own ticket, if you don‘t mind :) Thanks for raising!

@ernilambar
Copy link
Member

@ironprogrammer I tried to reproduce the error you mentioned in my local setup. Can you please elaborate the steps to reproduce the issue?

@ironprogrammer
Copy link

@ernilambar, thanks for the follow-up. The issue occurred when the file was in the plugins root (the default Core location for hello.php), which I later realized is not applicable to checks for plugin directory plugins -- they should each be in their own directory.

When the plugin was moved into its own directory, then the text domain checks worked as expected. Unless PCP is intended to be used on plugins installed in the plugins root like that, then my report can be disregarded.

@ernilambar
Copy link
Member

@ironprogrammer Thanks for the update. Although WordPress plugin directory does not accept single file plugin, PCP theoretically supports single file plugin. Mismatched textdomain check is not supposed to be triggered because we have kept that in the conditional checking is_single_file_plugin(). After checking the error message I realized it is coming from WordPress.WP.I18n which run for single file or multiple files plugin. Not critical but it is good to have fixed issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Checks Audit/test of the particular part of the plugin [Team] Plugin Review Issues owned by Plugin Review Team [Type] Bug An existing feature is broken
Projects
None yet
6 participants