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

Fix translation unloading #2840

Merged
merged 3 commits into from
Jan 9, 2025
Merged

Fix translation unloading #2840

merged 3 commits into from
Jan 9, 2025

Conversation

brianhogg
Copy link
Contributor

@brianhogg brianhogg commented Dec 20, 2024

Description

It was mentioned on a related trac ticket that we should not be unloading the textdomain on init. This is causing related changes in WP 6.7 to not show the translations if anything uses the lifterlms textdomain early.

I believe the unload_textdomain was added to fix a conflict with Loco Translate and saving in a certain location. I'm not able to replicate this, so it may no longer be needed. Restricting to just lifterlms core for now but we could just remove it for all add-ons too.

Refs #2807

How has this been tested?

Manually

Checklist:

  • This PR requires and contains at least one changelog file.
  • My code has been tested.
  • My code passes all existing automated tests.
  • My code follows the LifterLMS Coding & Documentation Standards.

@brianhogg brianhogg requested a review from ideadude as a code owner December 20, 2024 15:57
@brianhogg brianhogg added this to the Next Available Release milestone Dec 20, 2024
@brianhogg
Copy link
Contributor Author

Setting a different language for the User still works.

There's more information on translations with Loco translate and custom file locations. This should be fine without unloading the textdomain. https://localise.biz/wordpress/plugin/faqs/custom-folder

@@ -91,7 +89,9 @@ function llms_load_textdomain( $domain, $plugin_dir = null, $language_dir = null
$plugin_dir = $plugin_dir ? $plugin_dir : LLMS_PLUGIN_DIR;
$language_dir = $language_dir ? $language_dir : 'languages';

unload_textdomain( $domain );
if ( 'lifterlms' !== $domain ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to just remove this completely even for add-ons?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Let's remove it completely. Then test and if we don't have the issue in the latest versions of WP, Loco, and Lifter, merge it.

@brianhogg
Copy link
Contributor Author

Remove completely and do a test with an add-on + loco translate

@brianhogg brianhogg self-assigned this Jan 9, 2025
@brianhogg brianhogg merged commit bc59b6e into dev Jan 9, 2025
24 checks passed
@brianhogg brianhogg deleted the fix/translation-unloading branch January 9, 2025 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants