Make WordPress Core

Opened 10 years ago

Closed 2 years ago

Last modified 2 years ago

#26746 closed enhancement (fixed)

Avoid translating several times post type and taxonomy default labels

Reported by: chouby's profile Chouby Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.0 Priority: normal
Severity: normal Version: 3.0
Component: Posts, Post Types Keywords: has-patch early commit add-to-field-guide
Focuses: performance Cc:

Description

get_taxonomy_labels and get_post_type_labels translate default labels each time register_post_type and register_taxonomy are called. This is useless and is a rather expensive process resulting in a performance loss.

Attachments (6)

26746.patch (5.6 KB) - added by Chouby 10 years ago.
26746.2.patch (5.9 KB) - added by Chouby 10 years ago.
26746.3.patch (6.9 KB) - added by SergeyBiryukov 10 years ago.
26746.4.patch (17.3 KB) - added by Chouby 2 years ago.
26746.5.patch (17.4 KB) - added by Chouby 2 years ago.
Capture d’écran 2022-03-13 à 13.45.31.png (404.3 KB) - added by audrasjb 2 years ago.
The current patch throws a fatal error

Download all attachments as: .zip

Change History (36)

@Chouby
10 years ago

#1 @Chouby
10 years ago

  • Keywords has-patch added

#2 @nacin
10 years ago

  • Component changed from Performance to Post Types
  • Focuses performance added
  • Milestone changed from Awaiting Review to 3.9

#3 @nacin
10 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 3.9 to Future Release

I'm all for performance improvements here but, as written, this will break translations. Right now core post types and taxonomies are loaded twice, and the first time is before translations are loaded. These would thus always be in English.

A lesser concern could be someone switching between languages (such as each language being a post type), but they're likely going to have their own strings/labels, so it's not as big of a deal and probably an edge case.

@Chouby
10 years ago

#4 @Chouby
10 years ago

Yes you are right. It breaks translations. So we can't do that before the default textdomain is loaded. In the second patch, I propose to wait for the 'init' action. The benefit would not be so high for a default WordPress install, but could still be interesting for an install with lot of custom post types and taxonomies (provided that they are registered in an init action as recommended).

#5 @SergeyBiryukov
10 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 4.0

26746.2.patch works as expected for me. Refreshed, moving for 4.0 consideration.

#6 @Rarst
10 years ago

I think this can be pushed further. The current state without patch is regenerating labels on every call. The current patch will regenerate labels on every call until did_action('init') state change.

Then why don't we generate labels once before and once after state change?

Also timing-wise locale is available tad earlier than init at after_setup_theme, makes sense to check for that one instead in case CPTs are already getting registered in third party code.

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


10 years ago

#8 @SergeyBiryukov
10 years ago

  • Milestone changed from 4.0 to Future Release

#9 @chriscct7
8 years ago

  • Keywords needs-refresh added

#10 @SergeyBiryukov
3 years ago

#54128 was marked as a duplicate.

#11 @SergeyBiryukov
3 years ago

  • Milestone set to 5.9

#12 @SergeyBiryukov
3 years ago

The approach in 26746.3.patch still seems viable, though I'm wondering if it might affect any multilingual plugins. I guess we can try this while still in earlier stages of the release, and wait for community feedback.

Implementing the approach suggested in comment:6 also seems like a good idea.

#13 @ocean90
3 years ago

Sounds related to #38218 which was closed as a duplicate of #41305.

This ticket was mentioned in Slack in #core by sergey. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

#16 @audrasjb
2 years ago

  • Keywords early added
  • Milestone changed from 5.9 to Future Release

As per today's bug scrub and since tomorrow is WP 5.9 feature freeze, I'm moving this ticket to Future release. Also adding early workflow keyword since this enhancement needs to be properly checked to avoid backwards compatibility issues.

@Chouby
2 years ago

#17 @Chouby
2 years ago

The way proposed when the ticket was initiated can't work anymore due to the introduction of switch_to_locale() since that time.

In this new patch, I introduce a new class to store the default labels once they are translated. The reset() method can be called to reset the cache.

NB: I propose to decrease the priority of the functions hooked to change_locale to make sure that the default labels will be resetted before plugins hook to this same action with the default priority (in case they need to register their own post types or taxonomies when the locale is switched).

I tested the patch against WP 5.9-alpha-52272. The number of calls to MO::translate_entry() decreases from 2,296 to 1,207. Comparing cachegrind files, they show a total time divided by 12 for get_post_type_labels() and by 4 for get_taxonomy_labels().

Last edited 2 years ago by Chouby (previous) (diff)

@Chouby
2 years ago

#18 @Chouby
2 years ago

  • Keywords needs-refresh removed

This new patch uses the same approach as the previous one except that it uses the existing classes WP_Post_Type and WP_Taxonomy to store labels, rather than creating a new class.

#19 @SergeyBiryukov
2 years ago

  • Milestone changed from Future Release to 6.0
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#21 @audrasjb
2 years ago

The above PR refreshes the previous patch against trunk.

@audrasjb
2 years ago

The current patch throws a fatal error

#22 @audrasjb
2 years ago

  • Keywords needs-refresh needs-testing added

The current patch throws a fatal error. Investigating…

#23 @audrasjb
2 years ago

  • Keywords needs-refresh needs-testing removed

The above bug was a merge issue on my side, locally.
I can confirm the patch works fine on my side (it doesn't change anything, except it improves translation loading performance).

#24 @peterwilsoncc
2 years ago

I asked a question on the linked pull request about changing the priority of some default hooks. It's a minor back-compat break so it would be good to know why it's needed so the cost/benefit can be evaluated.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


2 years ago

audrasjb commented on PR #2404:


2 years ago
#26

Thanks @costdev @peterwilsoncc for the review!

#27 @peterwilsoncc
2 years ago

  • Keywords commit needs-dev-note added

Linked pull request looks good for commit.

I've added needs-dev-note as I think it would be good to alert multilingual plugins. I don't think it needs to be a full dev note, just a mention in a grouped note for developer or i18n changes.

#28 @peterwilsoncc
2 years ago

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

In 53058:

Posts, Post Types; Taxonomy: Translate default labels once.

Improve the translation of post type and taxonomy labels by caching the translations during runtime. To account for internationalisation plugins, the runtime cache is cleared as the post types/taxonomies are reinitiated on change_local hook.

The same property and methods are added to both WP_Post_Type and WP_Taxonomy:

  • $default_labels: for storing the translated strings at runtime
  • get_default_labels(): for getting the default labels, these are translated on the first run and stored in the new property.
  • reset_default_labels(): to clear the runtime cache and force a re-translation of the default labels

Props Chouby, nacin, SergeyBiryukov, Rarst, chriscct7, ocean90, audrasjb, costdev.
Fixes #26746.

#30 @milana_cap
2 years ago

  • Keywords add-to-field-guide added; needs-dev-note removed

We have new keyword for tickets that don't need dedicated dev note but rather just mentioning in Field guide or misc dev note.

Note: See TracTickets for help on using tickets.