qtranslate / qtranslate-xt

qTranslate-XT (eXTended) - reviving qTranslate-X multilingual plugin for WordPress. A new community-driven plugin soon. Built-in modules for WooCommerce, ACF, slugs and others.
GNU General Public License v2.0
546 stars 103 forks source link

Can no longer translate text in the admin - apply_filters( 'translate_text' ... no longer works with 3.15.1 #1337

Closed yoancutillas closed 1 year ago

yoancutillas commented 1 year ago

Describe the bug Since 3.15, the function apply_filters( 'translate_text', ... no longer works on the admin side, so the texts are no longer translated. It is working fine with 3.14.2.

To Reproduce

herrvigg commented 1 year ago

Thanks for reporting. I went a bit too fast when I deleted QTX_Translator_Admin (#1321), I didn't think some would use the base translator in the admin section.

I pushed a very simple fix to master, can you try it? It's a one-line code change, that you may also apply to your setup while waiting for the next release.

In fact I don't like how the translate_text filtering is used, it's a very old legacy design, possibly coming from the original qTranslate even before qTranslate-X. Plugins and themes should call the qTranslate functions directly, not through apply filters. Filters are meant to be used for integration to allow other plugins to connect their own functions, but here it's done the other way around. I can't deprecate this filter in the usual way, so I intend to add a new hook that will inform users how to use the functions.

yoancutillas commented 1 year ago

The fix is working thank you :)

Yes we need to use the translator in the admin (for example, for WYSIWYG editors, previewing the results, or sometimes translated strings are simply aimed to be used in the admin, etc.)

About the use of hooks, I think they describe their intention here. They use apply_filters instead of a function so that third-party developers can use it in their plugin without breaking the code if qTranslate is deactivated, and without having to check if a translation function exists before calling it. WPML is using its own apply_filters( 'wpml_translate_single_string', ... too.

I think it is smart because it is simple to use and without risks. It is not actually an issue, right?

Since qTranslate, WPML, and maybe other translation plugins have always encouraged their users to use that hook for their every strings, it is most possible that, as a result, most users currently have hundreds of occurences of this hook in their code.

I wouldn't recommend changing this, for you and your users ;)

herrvigg commented 1 year ago

About the use of hooks, I think they describe their intention here. They use apply_filters instead of a function so that third-party developers can use it in their plugin without breaking the code if qTranslate is deactivated, and without having to check if a translation function exists before calling it. WPML is using its own apply_filters( 'wpml_translate_single_string', ... too.

I know this interface but I question it. To me this is legacy for which I don't understand the logic. A filter makes sense if you'd like to override some behavior offered by a plugin, when the call comes from the plugin itself. If the translation plugin like qTranslate or WPML performs some functionality internally, then third-party plugins can alter that by adding some hooks. This is the core logic of WordPress and what makes it successful.

On the other hand, if the third-party plugin or theme calls apply_filter, that's the other way around. You act as if you were the provider of a functionality that you open up to others. Here it is expected that qTranslate creates a hook to perform that translation. Now "if qTranslate is deactived" it won't work anymore. The filter has anyway been designed for qTranslate. It should be named qtranslate_text (I started renaming them... then realized I would break the functionality so I reverted). But what is the goal? What is the use case?

If your plugin creates a hook with a lower priority than qTranslate, the raw values are already lost. If your plugin creates a hook with a higher priority than qTranslate, you would do the translation yourself. So why would need qTranslate? So there's only one use case for such translation: expecting the original plugin to do a fixed behavior. That is called a function API, by opposition to public "events" (action/filter) to be hooked.

These filters are a bad design because they are proprietary. The third-party users should call qTranslate functions if they use qTranslate. They should call WPML functions if they use WPML. They should not call a filter expecting such plugins create hooks on them. The filters would make sense if they were part of a WordPress translation standard, but even there, the filter should be called by the internal code in WordPress.

Also, using the filter mechanism is less efficient than calling the desired functions directly. An API is about functions, the filter/actions give ways to override an internal behavior.

yoancutillas commented 1 year ago

Indeed, the hook should at least have been named qtranslate_text in the first place. But, as you mentioned it, is too late to rename the hook, or it will break everything for everyone for almost no benefits.

Indeed, using a function instead would be more appropriate. But I believe that the point is not about code structure, but ease of use and compatibility. If you replace it with a function, it will break everything for everyone, and everyone will need to change their simple code to a more complex one. For example echo apply_filters( 'translate_text', $the_text ); to echo function_exists( 'qtranslate_text_function' ) ? qtranslate_text_function( $the_text ) : $the_text;.

Also, using the filter mechanism is less efficient than calling the desired functions directly.

That's the big question. Why would you make such a big change? Is it significantly more performant? I don't know, it would be necessary to benchmark the two codes above.

You are a skilled and perfectionnist developer, and you want your code to be the best as possible. Who are your users and what do they want?

herrvigg commented 1 year ago

Thank you. I have a long background with real time critical systems in the industry, automotive and aerospace, so I'm naturally quite exigent on the code quality and I'm much aware of integration issues. WordPress is just for hobby and it's quite a frustrating framework with many flaws but it's also part of the game. Good design not only gives satisfaction but it is required to maintain and extend the software without doing spaghetti code.

I see these filters as an aberration of design, they would be meaningful if we would open the runtime internal translation to other plugins, or define some kind of standard for WordPress translation, but that's not really what qTranslate is doing. It was surely an attempt to offer some integration but this doesn't look like the right approach. The public API is yet not defined in qTranslate. Requiring to call apply_filters is going against a conventional public API. Take the core WordPress interface for example, would you ever call apply_filters on one of their own filters? That would be much convoluted.

I will not do breaking changes immediately, but there should be a transition. Looking forward I still plan to move to a new plugin and this could be the opportunity to phase the legacy out.

yoancutillas commented 1 year ago

would you ever call apply_filters on one of their own filters?

Theoretically, no. Or actually, just for conversation, only if it is the only way to access the final value of a variable, I found a case where I needed to do that: If you want to load your plugin translation file (.mo) from another location than your own plugin directory or WP_LANG_DIR . '/plugins (to allow customers to create their own custom translations without losing them with each update, very useful! (the first one is deleted with the plugin updates, the second one is deleted with the wp.org translations updates)).

Wordpress has a filter to change the locale before loading the plugin textdomain "wp-includes\l10n.php l.921": $locale = apply_filters( 'plugin_locale', determine_locale(), $domain );

So if you want to get the same value for the $locale variable before loading hte custom .mo files, you need to call that hook.

Looking forward I still plan to move to a new plugin and this could be the opportunity to phase the legacy out.

This is great news, looking forward to it.

herrvigg commented 1 year ago

Released in 3.15.2.