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
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
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
41 changes: 28 additions & 13 deletions includes/core/classes/class-rest-api.php
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;
$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
34 changes: 34 additions & 0 deletions includes/core/classes/class-setup.php
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,38 @@ protected function setup_hooks(): void {
);
}

/**
* 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 how 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)

// Keep as a fallback if translation files were not loaded from translate.wordpress.org ??
carstingaxion marked this conversation as resolved.
Show resolved Hide resolved
// load_plugin_textdomain( 'gatherpress', false, GATHERPRESS_DIR_NAME . '/languages' );

// Or use it like that
// and DELETE the /languages folder,
// and "* Domain Path: /languages" as well?
load_plugin_textdomain( 'gatherpress', false, false );
carstingaxion marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Add custom links to the plugin action links in the WordPress plugins list.
*
Expand Down
13 changes: 8 additions & 5 deletions includes/templates/admin/emails/event-email.php
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