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

⚠️ Load later for i18n (Enhance i18n of attendees-email...) #752

Closed
wants to merge 16 commits into from

Conversation

carstingaxion
Copy link
Collaborator

@carstingaxion carstingaxion commented Aug 1, 2024

Description of the Change

Originally planned to only close #651, some additional findings are solved with this PR, too:

How to test the Change

Testing this PR might be a little tricky and is not possible with WordPress Playground because of the necessary sending & receiving of emails.

Preparation to test:

  1. Setup at least two different users
  2. One of the users creates an event, the other users attends. (This will make sure, 2 emails are sent for the test)
  3. Assign a non-site-default language to one of the users in their profile.php
  4. Go to Updates, get to the bottom of the page and hit the "Update translations" button.
  5. Make sure that in the following screen, the translation files for the selected non-site-default language gets downloaded!

Testing this PR:

  1. Edit the newly created event
  2. Compose a message via Email
  3. Send email
  4. Check the results, one email should follow the user-defined language, while the other is sent in the default locale

Changelog Entry

Fix sending localised emails

Credits

Props @carstingaxion

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

Copy link

what-the-diff bot commented Aug 1, 2024

PR Summary

  • Adjusted gatherpress.php File
    The unnecessary Domain Path comment has been eliminated for cleaner code.

  • Enhanced class-rest-api.php File
    Eliminated some previously commented code used for generating email subject and body. Introduced new logic to switch languages or 'locales' more efficiently during email dissemination. The system will now restore the original language setting after mailing.

  • Refined class-setup.php File
    Integrated a new hook that will help load the plugin's language package or 'textdomain', increasing flexibility in managing plugin translations.

  • Amended event-email.php File
    We've taken out the HTML 'lang' attribute for greater code conciseness. We also introduced a new variable, $gatherpress_event_image, to store the event image URL for better manageability. Moreover, the event image display in the email template has been improved upon with this update. The language_attributes() function has been added to the HTML tag to incorporate language information, enhancing web accessibility. Finally, we've modernized the way the event title presents itself in the email template for better aesthetics.

…e GatherPress filters for date- and time- format, as well as the users timezone, are recognized by the functions inside render_template().
@carstingaxion carstingaxion changed the title Fix/651 Enhance i18n of attendees-email & minor fixes Aug 1, 2024
@carstingaxion carstingaxion deleted the fix/651 branch August 13, 2024 20:11
@carstingaxion carstingaxion restored the fix/651 branch August 14, 2024 14:34
@carstingaxion carstingaxion reopened this Aug 14, 2024
@carstingaxion

This comment was marked as outdated.

@carstingaxion

This comment was marked as resolved.

@carstingaxion
Copy link
Collaborator Author

Ok, found the problem:

In the gatherpress timeformat settings, \ are ok and allowed.
grafik

While within the profile.php, they are not and get stripped off on save.
grafik

@carstingaxion carstingaxion marked this pull request as ready for review August 25, 2024 21:18
* A call to load_plugin_textdomain() is ABSOLUTELY NEEDED to properly load all translation files
* for users who chosed another than the sites default language in their /profile.php.
*/
public function load_gatherpress_textdomain() {
Copy link
Contributor

Choose a reason for hiding this comment

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

You really don't need this. It's pointless and only has downsides. And saying "A call to load_plugin_textdomain() is ABSOLUTELY NEEDED" is purely WRONG.

The just-in-time translation loading in WordPress handles everything for you and only loads the translations when needed.

Copy link
Collaborator Author

@carstingaxion carstingaxion Aug 28, 2024

Choose a reason for hiding this comment

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

Hmm... a little bit strange.

I absolutely trust you in translation stuff, because I know you have been advocating and coding a lot for that topic.

From my long doc-comment you can imagine that I read & tried a lot to come up with my solution and the UPPERCASE comment. But unfortunately I can not make this work, without that line of code. Or maybe I have the wrong expectations.

How to test

  1. Set site language to English in wp-admin/options-general.php

  2. Set user prefered language to German in wp-admin/profile.php

  3. Comment out load_plugin_textdomain( 'gatherpress', false, false )

  4. Check that the Admin-UI is German now, except everything GatherPress

  5. Re-Enable load_plugin_textdomain( 'gatherpress', false, false )

  6. Check that the Admin-UI is German now, including everything GatherPress

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue is that you are triggering just-in-time translation loading too early.

WordPress tries to load the translations automatically as soon as you call __(). In your case, this happens somewhere before the current user is loaded, thus the admin won't be in German.

With this manual load_plugin_textdomain() call you're just papering over this issue, causing WP to try to load the translations twice.

The proper fix is to initialize your plugin later. This in the main plugin file works:

add_action(
	'plugins_loaded',
	static function () {
		// Initialize setups.
		GatherPress\Core\Setup::get_instance();
	}
);

// Or simply:

add_action(
	'plugins_loaded',
	array( '\GatherPress\Core\Setup', 'get_instance' )
);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, thank you very much Pascal, once again your explanation is better than any docs (I know) about that.

I followed your advise:

  1. Set add_action( 'plugins_loaded', array( '\GatherPress\Core\Setup', 'get_instance' ));
  2. Disabled my attempt // add_action( 'init', array( $this, 'load_gatherpress_textdomain' ), 0 );
  3. And started looking for the anoying bug again
  4. and was able to track it down until Assets->localize(), which calls
    'timezoneChoices' => Utility::timezone_choices(), and seems to be the culprit. Disabling this line brings all translations back into shape; having this enabled messes things up.
  5. Going on ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hello @mauteri ,

do we really need all possible timezones

  1. in the frontend markup?
  2. in the backend markup?
  3. on every page load?

https://github.com/carstingaxion/gatherpress/blob/02358ab60318a7464e31865ffea0f3bb8c961809/includes/core/classes/class-assets.php#L306

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't Assets::localize() only called in Assets::add_global_object() which is hooked to wp_head? So that should be late enough.


Aside: just adding a script tag like this is usually not a good idea. You should at least use wp_print_inline_script_tag. Even better, attach it to a specific script with wp_add_inline_script.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bildschirmaufzeichnung.vom.05.09.2024.16.08.52.mp4

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Found it: 🎉

$timezones_raw   = explode( PHP_EOL, wp_timezone_choice( 'UTC', get_user_locale() ) );  // i18n FIX!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As expected, a change like to the load order will break things. At least the unit tests.

Copy link
Collaborator Author

@carstingaxion carstingaxion Sep 5, 2024

Choose a reason for hiding this comment

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

Thanks for taking the time again @swissspidy !

Isn't Assets::localize() only called in Assets::add_global_object() which is hooked to wp_head? So that should be late enough.

And on admin_print_scripts, which is the problematic trace.

Aside: just adding a script tag like this is usually not a good idea. You should at least use wp_print_inline_script_tag. Even better, attach it to a specific script with wp_add_inline_script.

👍 I wanted to suggest the same, even that the relevant data - the timezones, in this case - are only used for the Event Date Block (#684)

@carstingaxion carstingaxion changed the title Enhance i18n of attendees-email & minor fixes ⚠️ Load later for i18n (Enhance i18n of attendees-email...) Sep 5, 2024
Copy link

github-actions bot commented Sep 5, 2024

Preview changes with Playground

You can preview the least recent changes for PR#752 by following one of the links below:

⚠️ Note: The preview sites are created using WordPress Playground. You can add content, edit settings, and test the themes as you would on a real site, but please note that changes are not saved between sessions.

@carstingaxion
Copy link
Collaborator Author

This PR was split into multiple different PRs to make testing, understanding etc. easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set user-timezone & user-locale in emails
3 participants