Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#54561 closed enhancement (fixed)

Avoid translating several times widget media default strings

Reported by: chouby's profile Chouby Owned by: audrasjb's profile audrasjb
Milestone: 6.0 Priority: normal
Severity: normal Version: 4.8
Component: Widgets Keywords: has-patch commit assigned-for-commit
Focuses: performance Cc:

Description

The translation being a rather expensive process, we should avoid to translate the same string several times.

The class Widget_Media is a base class for 4 widgets. Its constructor is thus called 4 times, meaning that all the strings included in this constructor are translated 4 times.

Attachments (2)

54561.patch (4.5 KB) - added by Chouby 3 years ago.
Avoid 27 duplicate translations
54561.2.patch (5.4 KB) - added by Chouby 2 years ago.
Refresh patch and reset default labels on change_locale

Download all attachments as: .zip

Change History (21)

@Chouby
3 years ago

Avoid 27 duplicate translations

#1 @Chouby
3 years ago

  • Keywords has-patch added

#2 @costdev
3 years ago

  • Type changed from defect (bug) to enhancement

#4 @audrasjb
3 years ago

  • Milestone changed from Awaiting Review to 6.0
  • Owner set to audrasjb
  • Status changed from new to reviewing

#5 @audrasjb
3 years ago

  • Keywords needs-testing added

The above PR refreshes the patch against trunk and updates @since mentions.
GH Tests are passing.
Adding needs-testing keyword to ensure it doesn't break anything :)

#6 @audrasjb
3 years ago

  • Keywords needs-testing removed

Removing needs-testing keyword: the patch looks good on my side, it doesn't break anything on the Classic Widgets screen and it successfully mutualizes translations.

#7 @peterwilsoncc
2 years ago

Does the static need to be cleared on change_locale to account for language switching plugins or similar? I set my site's language to Français and it all seemed good but I'd rather ask as others on this ticket deal with translations more often than I.

#8 @audrasjb
2 years ago

I don't think any specific action is needed. What do you think @Chouby?

#9 @Chouby
2 years ago

The usages I remember having seen for switch_to_locale() are for sending emails, creating documents such as PDF invoices and displaying the admin bar in a different language than the main language on frontend.

I don't foresee a usage which wpuld require to switch the locale before displaying the backend of widgets.

However, if we want to be on the safe side, then it would indeed be necessary to reset the static variables on change_locale.

#10 @peterwilsoncc
2 years ago

Thanks @Chouby and @audrasjb.

For the sake of a single line in default-filters.php, clearing the static on change_locale seems safest.

I'd be surprised if failing to do so broke a plugin but similar enough surprises have happened in the past.

#11 @audrasjb
2 years ago

Alright @peterwilsoncc @Chouby, but I'm unsure what would be the best function to hook on in default-filters.php. Any thought?

#12 @peterwilsoncc
2 years ago

@audrasjb I think you'd need to add a reset method reset_default_labels() similar to the one in WP_Post_Type and then hook on to that in default-filters.

#13 @audrasjb
2 years ago

I'm sorry, but I'm not sure where to hook/call the reset labels function… (and I'm not sure it's really needed too). Halp :)

#14 @costdev
2 years ago

If the reset_default_labels() method is going to be added, I think default-filters.php would just need this:

add_action( 'change_locale', array( 'WP_Widget_Media', 'reset_default_labels' ) );

As for "where" in default-filters.php, the change_locale hooks for Post and Taxonomy are under their respective comments, so this hook could just go under the // Media. comment at ~L621.

What do you think @audrasjb?

@Chouby
2 years ago

Refresh patch and reset default labels on change_locale

#15 @Chouby
2 years ago

The patch required some refresh due to recent changes in default labels. 54561.2.patch also adds a way to reset the default labels on change_locale.

#16 @audrasjb
2 years ago

  • Keywords commit assigned-for-commit added

Thanks for the patch @Chouby.
Let's roll.

#17 @audrasjb
2 years ago

PR refreshed with the above patch: https://github.com/WordPress/wordpress-develop/pull/2123/files
Waiting for unit tests to pass.

#18 @audrasjb
2 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 53158:

Widgets: Avoid 27 duplicate translations in Media Widgets constructor.

This changeset rationalizes the declaration of translation strings in the Media Widgets constructor.

Props Chouby, audrasjb, peterwilsoncc, costdev.
Fixes #54561.

Note: See TracTickets for help on using tickets.