WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 7 years ago

#10527 closed enhancement (fixed)

load_textdomain() merges even if not needed.

Reported by: hakre Owned by: nbachiyski
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.8.1
Component: I18N Keywords: has-patch tested 2nd-opinion
Focuses: Cc:
PR Number:

Description

About 4 weeks ago in #10286 with [11680] & [11681] merging of textdomains have been introduced in load_textdomain(). That generally is a good feature, but I would like to see it optional to not merge textdomains over and over again.

for example with one of my co-maintained plugin we load the translation in an important object as well as in the admin panel. doing the merge now in the admin panel would not be necessary.

Therefore I suggest to add an optional parameter that flags if a merge is wanted / needed.

Attachments (4)

10527.patch (1.8 KB) - added by hakre 10 years ago.
10527.2.patch (3.3 KB) - added by hakre 10 years ago.
Updated Patch reflecting defaults and further comment improvements
10527-netbeans.patch (3.5 KB) - added by hakre 10 years ago.
Test of a netbeans diff export on the same changes.
is-textdomain-empty.diff (677 bytes) - added by nbachiyski 10 years ago.

Download all attachments as: .zip

Change History (19)

@hakre
10 years ago

#1 @hakre
10 years ago

  • Keywords has-patch tested added

Uploaded a patch.

It is tested, I applied the changes to the core sourcecode and then used a plugin to load a plugin textdomain. that plugin textdomain is loaded then only once, once it has been loaded it is not automatically merged. Setting the optional $merge parameter to true then gets it loaded & merged then.

#2 @dd32
10 years ago

  • Keywords 2nd-opinion added
  • Milestone changed from 2.8.3 to 2.9
  • Type changed from defect (bug) to enhancement

attachment 10527.patch added

  • Typo in comments: "textdomain in nemory."
  • The commits you refer to, Change nothing other than spacing, and comments, as well as adding a error-condition handler..
  • Merging is the current default, So shouldn't the merge param be true by default? - It was after all, apparently chosen to merge by default.

This is really an enhancement, setting as such.
Milestone: Current trunk (2.9) for enhancements with patch.

#3 @dd32
10 years ago

See for merging: #7376 (closed enhancement: fixed), Opened 12 months ago, Allow merging of similarly named text domains

#4 @hakre
10 years ago

Then it should default true for textdomain and default false for plugin. I must change the patch later on, need to go now.

@hakre
10 years ago

Updated Patch reflecting defaults and further comment improvements

@hakre
10 years ago

Test of a netbeans diff export on the same changes.

#5 @hakre
10 years ago

Updated the patch. Defaults to true now for load_textdomain(). Fixed the typo in comments as well as taking care of line-length in the docblock comments and stricter writing according to the wordpress coding standards.

#6 @hakre
10 years ago

Another Idea is to have an additional function called load_plug_textdomain_once() and load_textdomain_once(). But that would help to keep the existing function signatures untouched but it would mean more code.

#7 @hakre
10 years ago

For those who are looking for a workaround for their plugins, this is a tested piece of code that has a shortcomming in using the global directly but it's working. Can be used in a plugin's function or class:

global $l10n;	
$domain = 'pluginstextdomain';				
if (!isset( $l10n[$domain] ))
{
	load_plugin_textdomain($domain, false, 'path_to/languages');
}	

#8 @hakre
10 years ago

maybe the API should be extended having a is_textdomain_loaded() function? just in case the whole patch is not fitting.

#9 @ryan
10 years ago

  • Milestone changed from 2.9 to Future Release

#11 @nbachiyski
10 years ago

  • Milestone changed from Future Release to 3.0

The case of plugins with so many strings that the merge becomes a problem is extremely rare. I don't think it warrants an extra argument for these functions.

However, it makes sense to have an API call for checking whether a domain is empty. This way if it very important to you, you can check if your translation is already loaded.

#12 @nacin
10 years ago

I'm thinking is_textdomain_loaded() instead, just to be consistent on naming?

#13 @nbachiyski
10 years ago

Agreed, loaded is better.

#14 @automattor
10 years ago

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

(In [13304]) Introduce is_textdomain_loaded(). Fixes #10527 props nbachiyski.

#15 @nacin
7 years ago

Follow-up: #21319

Note: See TracTickets for help on using tickets.