Make WordPress Core

Opened 18 months ago

Closed 14 months ago

Last modified 14 months 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 18 months ago.
Avoid 27 duplicate translations
54561.2.patch (5.4 KB) - added by Chouby 14 months ago.
Refresh patch and reset default labels on change_locale

Download all attachments as: .zip

Change History (21)

@Chouby
18 months ago

Avoid 27 duplicate translations

#1 @Chouby
18 months ago

  • Keywords has-patch added

#2 @costdev
18 months ago

  • Type changed from defect (bug) to enhancement

#4 @audrasjb
17 months ago

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

#5 @audrasjb
17 months 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
17 months 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
15 months 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
14 months ago

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

#9 @Chouby
14 months 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
14 months 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
14 months 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
14 months 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
14 months 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
14 months 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
14 months ago

Refresh patch and reset default labels on change_locale

#15 @Chouby
14 months 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
14 months ago

  • Keywords commit assigned-for-commit added

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

#17 @audrasjb
14 months ago

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

#18 @audrasjb
14 months 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.