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
554 stars 104 forks source link

Cron job not automatically deleted - "cron" option should not be translated #1188

Closed yoancutillas closed 2 years ago

yoancutillas commented 2 years ago

Hello,

Thank you so much for integrating qtranslate-slug and your continued support!

Here is an issue I ran into (tested with latest 3.12.0):

If you set up a one-time cron job with wp_schedule_single_event and if one of its $args has translatable text, then the cron job is not deleted after it is triggered, and it is endlessly triggered. E.g.:

wp_schedule_single_event( time() + 3, 'my_test_hook', array( array( 'text' => '[:fr]Mon texte[:en]My text[:]' ) ) );
add_action( 'my_test_hook', function() { error_log( 'This will trigger over and over again.' ); } );

The issue doesn't occur if you don't translate the "cron" option:

// TEMP FIX - Do not translate cron option, as it may prevent cron jobs from being deleted
function my_tempfix_do_not_translate_options() {
    remove_filter( 'option_cron', 'qtranxf_translate_option', 5 );
}
add_action( 'plugins_loaded', 'my_tempfix_do_not_translate_options', 10 );
herrvigg commented 2 years ago

Mmm good catch. I don't know well why the cron would loop like that. But here is the code setting these filters, it doesn't look very safe to translate all options (set by default in the "Translation of options" settings) :

https://github.com/qtranslate/qtranslate-xt/blob/139986fd966134cea53b03f737b827f79f0aa731/qtranslate_frontend.php#L439

If we can understand better why the cron behaves like this we can have an exception there.

herrvigg commented 2 years ago

In fact if you leave the default settings "Filter all WP options..." there's a WHERE clause like this

            $where = ' WHERE autoload=\'yes\' AND (option_value LIKE \'%![:__!]%\' ESCAPE \'!\' OR option_value LIKE \'%{:__}%\' OR option_value LIKE \'%<!--:__-->%\')';

which means the option value must contain some ML tags. In that case it looks logical to translate the option, but I don't know if this is the right way to do it.

yoancutillas commented 2 years ago

The reason is because

  1. First, cron jobs are retrieved with wp_get_ready_cron_jobs(), which is calling _get_cron_array(), which is calling get_option( 'cron' ). So the $args are translated at this point.

https://build.trac.wordpress.org/browser/trunk/wp-cron.php#L79

  1. Then, the cron jobs are deleted with wp_unschedule_event() and that function takes for parameters the - now translated - $args.

However, as the wp_unschedule_event() documentation says:

these arguments are used to uniquely identify the event, so they should be the same as those used when originally scheduling the event.

The $args passed to the function are translated. The $args stored in database are not translated. No match, no deletetion, and the cron job runs in an infinite loop.

https://build.trac.wordpress.org/browser/trunk/wp-cron.php#L127


Thanks for pointing out the "Translation of options" option!

I believe the "Translate only options listed below" should be the default value. As qTranslate mentions:

Normally, there are very few options, which actually need a translation

I would even say that there is a finite and known list of options to translate in WordPress, in WooCommerce, etc. For the other plugins / theme, it is up to them to translate their options, or if they don't support qTranslate-XT, it is up to the administrator to add the desired option in the list.

But I also understand why you set it to "Filter all WordPress options for translation" by default: less effort for the user, less support requests for the developer.

In anycase, the "cron" option is a special case and should not be allowed to be translated.

herrvigg commented 2 years ago

It could be possible to handle the cron key differently, but that would add more complications than simply skipping the whole translation for the cron items. The question is if there's any good reason to translate it. I don't use this, but do we have an example where ML tags in the cron would make sense to translate? Perhaps they should be translated later when the cron job is effectively running? If yes, or if there's no good reason to translate, then we would make a special case for the cron entries and skip the automatic translation.

yoancutillas commented 2 years ago

they should be translated later when the cron job is effectively running

Yes :)

A WP cron job simply triggers the desired hook at the desired time. The developer must create a function and use the $args in it anyway. So the developer can perfectly translate the $args there. E.g.:

wp_schedule_single_event( time() + 3, 'my_test_hook', array( '[:fr]Mon texte[:en]My text[:]' ) );
add_action( 'my_test_hook', function( $text ) { $translated_text = apply_filters( 'translate_text', $text ); } );

do we have an example where ML tags in the cron would make sense to translate?

Not in WP and WooCommerce at least, you can verify it here, the $args don't contain ML tags: https://github.com/WordPress/WordPress/search?q=wp_schedule_single_event https://github.com/WordPress/WordPress/search?q=wp_schedule_event https://github.com/woocommerce/woocommerce/search?q=wp_schedule_single_event https://github.com/woocommerce/woocommerce/search?q=wp_schedule_event

Morevover, the mere existence of ML tags in the cron makes that problem happen (cron job not deleted). My guess is that if there was an example where ML tags were in cron (that would make sense to translate or not), then you would have had a lot of bug reports.

I conclude that if there is ML tags in the cron, then they are explicitly set by the developer, and the developer knows that he will need to translate them when the cron job is running (it's also my case).

herrvigg commented 2 years ago

Pushed a fix to exclude cron from translation of all options. Let me know if this works as expected. It's still allowed when the options are given manually, at the responsibility of the admin. There could be some situations where the admin has very custom needs.

yoancutillas commented 2 years ago

Tested, it fixes the issue for me, thank you!

herrvigg commented 2 years ago

Fix released in 3.12.1.