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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 16 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
67c5ff7
Make sure translation files are loaded properly.
carstingaxion Aug 1, 2024
ba03e38
Load users locale before sending emails & reset afterwards
carstingaxion Aug 1, 2024
4fec728
Set html language attributes correctly for the email
carstingaxion Aug 1, 2024
da57071
Conditionally render the featured image in the email, only if it is set.
carstingaxion Aug 1, 2024
eb20906
Set the current user to the actual member to mail to, to make sure th…
carstingaxion Aug 1, 2024
6ffc797
Fix CS
carstingaxion Aug 1, 2024
4d012cf
Do not rely on /languages folder in favor of translate.wordpress.org
carstingaxion Aug 1, 2024
65905e8
Fix whitespaces for CS
carstingaxion Aug 1, 2024
8867db1
Add doc-comment desc
carstingaxion Aug 1, 2024
bd0fdb7
fulfilling 1001 wishes for the CS check action
carstingaxion Aug 1, 2024
02358ab
Keep slashes in user-defined date- and time-format strings
carstingaxion Aug 25, 2024
7e5aae7
Initialize all setups later, hook on 'plugins_loaded'
carstingaxion Sep 5, 2024
38c9f07
Remove superflous load_plugin_textdomain() call
carstingaxion Sep 5, 2024
3b9a063
Fix i18n-loading problem, by providing a locale to wp_timezone_choice()
carstingaxion Sep 5, 2024
3c2230a
Require main-plugin-file to test on priority 9, because the plugin it…
carstingaxion Sep 5, 2024
41681fe
Move de-activation-hooks out of 'plugins_loaded'
carstingaxion Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion gatherpress.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
* Requires PHP: 7.4
* Requires at least: 6.4
* Text Domain: gatherpress
* Domain Path: /languages
* License: GNU General Public License v2.0 or later
* License URI: https://www.gnu.org/licenses/gpl-2.0.html
*
Expand Down
43 changes: 29 additions & 14 deletions includes/core/classes/class-rest-api.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,28 +448,43 @@ public function send_emails( int $post_id, array $send, string $message ): bool
return false;
}

$members = $this->get_members( $send, $post_id );
/* translators: %s: event title. */
$subject = sprintf( __( '📅 %s', 'gatherpress' ), get_the_title( $post_id ) );
$body = Utility::render_template(
sprintf( '%s/includes/templates/admin/emails/event-email.php', GATHERPRESS_CORE_PATH ),
array(
'event_id' => $post_id,
'message' => $message,
),
);
$headers = array( 'Content-Type: text/html; charset=UTF-8' );
$subject = stripslashes_deep( html_entity_decode( $subject, ENT_QUOTES, 'UTF-8' ) );
// Keep the currently logged-in user.
$current_user = wp_get_current_user();

$members = $this->get_members( $send, $post_id );
foreach ( $members as $member ) {
if ( '0' === get_user_meta( $member->ID, 'gatherpress_event_updates_opt_in', true ) ) {
continue;
}

if ( $member->user_email ) {
$to = $member->user_email;
$to = $member->user_email;
$switched_locale = switch_to_user_locale( $member->ID );

// Set the current user to the actual member to mail to,
// to make sure the GatherPress filters for date- and time- format, as well as the users timezone,
// are recognized by the functions inside render_template().
wp_set_current_user( $member->ID );

/* translators: %s: event title. */
$subject = sprintf( __( '📅 %s', 'gatherpress' ), get_the_title( $post_id ) );
carstingaxion marked this conversation as resolved.
Show resolved Hide resolved
$body = Utility::render_template(
sprintf( '%s/includes/templates/admin/emails/event-email.php', GATHERPRESS_CORE_PATH ),
array(
'event_id' => $post_id,
'message' => $message,
),
);
$headers = array( 'Content-Type: text/html; charset=UTF-8' );
$subject = stripslashes_deep( html_entity_decode( $subject, ENT_QUOTES, 'UTF-8' ) );

// Reset the current user to the editor sending the email.
wp_set_current_user( $current_user->ID );

wp_mail( $to, $subject, $body, $headers );

if ( $switched_locale ) {
restore_previous_locale();
}
}
}

Expand Down
33 changes: 33 additions & 0 deletions includes/core/classes/class-setup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ protected function instantiate_classes(): void {
* @return void
*/
protected function setup_hooks(): void {
add_action( 'init', array( $this, 'load_gatherpress_textdomain' ), 0 );
carstingaxion marked this conversation as resolved.
Show resolved Hide resolved

register_activation_hook( GATHERPRESS_CORE_FILE, array( $this, 'activate_gatherpress_plugin' ) );
register_deactivation_hook( GATHERPRESS_CORE_FILE, array( $this, 'deactivate_gatherpress_plugin' ) );

Expand Down Expand Up @@ -111,6 +113,37 @@ protected function setup_hooks(): void {
);
}

/**
* Load plugin textdomain.
*
* Make sure to load all translation fileseven for users
* who chosed another than the sites default language.
*
* @see https://developer.wordpress.org/reference/functions/load_plugin_textdomain
*
* Calling load_plugin_textdomain() should be delayed until init action.
* @see https://developer.wordpress.org/reference/functions/load_plugin_textdomain/#comment-1568
*
* load_plugin_textdomain() will try to load the mo file firstly from:
* WP_LANG_DIR . ‘/plugins/’ . $mofile
* Only if it is not able to it will load it from GATHERPRESS_DIR_NAME folder
* @see https://developer.wordpress.org/reference/functions/load_plugin_textdomain/#comment-3521
*
* Since WordPress 4.6 translations now take translate.wordpress.org as priority and so
* plugins that are translated via translate.wordpress.org DO NOT necessary require load_plugin_textdomain() anymore.
* If you don’t want to add a load_plugin_textdomain() call to your plugin
* you have to set the Requires at least: field in your readme.txt to 4.6 or more.
* @see https://developer.wordpress.org/plugins/internationalization/how-to-internationalize-your-plugin/#plugins-on-wordpress-org
*
* BUT, ...!!!
*
* 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 •
edited
Loading

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 •
edited
Loading

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)

load_plugin_textdomain( 'gatherpress', false, false );
}

/**
* Add custom links to the plugin action links in the WordPress plugins list.
*
Expand Down
4 changes: 2 additions & 2 deletions includes/core/classes/class-user.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,8 @@ public function save_profile_fields( int $user_id ): void {
}

update_user_meta( $user_id, 'gatherpress_event_updates_opt_in', intval( filter_input( INPUT_POST, 'gatherpress_event_updates_opt_in' ) ) );
update_user_meta( $user_id, 'gatherpress_date_format', sanitize_text_field( filter_input( INPUT_POST, 'gatherpress_date_format' ) ) );
update_user_meta( $user_id, 'gatherpress_time_format', sanitize_text_field( filter_input( INPUT_POST, 'gatherpress_time_format' ) ) );
update_user_meta( $user_id, 'gatherpress_date_format', sanitize_text_field( filter_input( INPUT_POST, 'gatherpress_date_format', FILTER_SANITIZE_ADD_SLASHES ) ) );
update_user_meta( $user_id, 'gatherpress_time_format', sanitize_text_field( filter_input( INPUT_POST, 'gatherpress_time_format', FILTER_SANITIZE_ADD_SLASHES ) ) );
update_user_meta( $user_id, 'gatherpress_timezone', sanitize_text_field( filter_input( INPUT_POST, 'gatherpress_timezone' ) ) );
}
}
13 changes: 8 additions & 5 deletions includes/templates/admin/emails/event-email.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,14 @@
return;
}

$gatherpress_event = new Event( $event_id );
$gatherpress_venue = $gatherpress_event->get_venue_information()['name'];
$gatherpress_event = new Event( $event_id );
$gatherpress_event_image = get_the_post_thumbnail_url( $event_id, 'full' );
$gatherpress_venue = $gatherpress_event->get_venue_information()['name'];

?>

<!DOCTYPE html>
<html lang="en-US">
<html <?php language_attributes(); ?>>
carstingaxion marked this conversation as resolved.
Show resolved Hide resolved
<head>
<title><?php echo wp_kses_post( get_the_title( $event_id ) ); ?></title>
</head>
Expand All @@ -36,8 +37,10 @@
<?php echo wp_kses( nl2br( $message ), array( 'br' => array() ) ); ?>
</p>
<?php endif; ?>
<!-- Feature Image -->
<img src="<?php echo esc_url( get_the_post_thumbnail_url( $event_id, 'full' ) ); ?>" alt="<?php esc_attr_e( 'Event Image', 'gatherpress' ); ?>" style="max-width: 100%;">
<?php if ( $gatherpress_event_image ) : ?>
<!-- Feature Image -->
<img src="<?php echo esc_url( $gatherpress_event_image ); ?>" alt="<?php esc_attr_e( 'Event Image', 'gatherpress' ); ?>" style="max-width: 100%;">
<?php endif; ?>

<!-- Event Title -->
<h1 style="text-align: center;"><?php echo wp_kses_post( get_the_title( $event_id ) ); ?></h1>
Expand Down
Loading