Remove t() from _set_option_defaults()

add tagj

Simple and correct. It works.

Just some discussion in irc:

<dawehner> merlinofchaos: is set_options_default still needed with unpack_options? see http://drupal.org/node/1376348
<Druplicon> http://drupal.org/node/1376348 => Remove t() from _set_option_defaults() => Views, Code, normal, reviewed & tested by the community, 2 comments, 4 IRC mentions
<dawehner> it's documented there that it can disappear, so i guess it's safe to remove
<dawehner> or at least clear the content of it
<dawehner> so calls don't fail
<merlinofchaos> dawehner: No, unpack_options does not appear to actually set the default.
<dawehner> oh yess ... but the t() should at least be removed
<merlinofchaos> dawehner: unpack_options should handle the default so we don' thave to recurse into the array twice. :P
<merlinofchaos> dawehner: That's...interesting. I never noticed it doing that.
<-- wroos (~wroos@h-176-10-212-44.na.cust.bahnhof.se) has quit (Quit: Lämnar)
<merlinofchaos> dawehner: I wish we'd noticed this a year ago, we could've removed that in D7. Now? I'm not so sure.
<dawehner> oh that's probably the same as $storage[$key] = t($value); in unpack_options
<dawehner> it's about code-base strings
<merlinofchaos> dawehner: Yeah but it's inefficient in any case; we're setting the default regardless of whether or not we have an overridden value. That's...kind of crummy.
<-> timplunkettAFK is now known as timplunkett
<merlinofchaos> dawehner: That unpack code has become quite the horror. :(
<dawehner> indeed, i would actually like to test it probably so it can be refactored with less worries
<dawehner> especially this condition :(
<dawehner> merlinofchaos: hey http://drupal.org/files/views-unpack_options-402944-43-6.x-2.x.patch was an actualy patch in the issue queue
<merlinofchaos> dawehner: It is a good candidate for testing because it's actually quite isolated.
--> ezra-g (~ezra-g@drupal.org/user/69959/view) has joined #drupal-views
<merlinofchaos> dawehner: Oh. Yes I am totally against that patch.
<merlinofchaos> dawehner: I simply refuse to place an artificial depth limit AND make the code worse to save a few recursive function calls.
<dawehner> it was more or less a sign of desperation :) the caching though did the job
<-- jenlampton (~jenlampto@adsl-75-21-50-145.dsl.ksc2mo.sbcglobal.net) has quit (Quit: jenlampton)
<dawehner> merlinofchaos: the good thing about unpack_options is that it only runs on changed values from the default
<dawehner> which is actually at the same time a problem, as it can not be changed that easy from my perspective (0.5 am) to support the default case
<dawehner> well iterate above the option_definition could be done but this would increase the amount of in the cache
<merlinofchaos> dawehner: Right. But really it should go through the options array. "Do I ahve a value? Yes? Use it. No? Use default." Lather, rinse repeat.
<dawehner> not sure whether this is actually bad
<merlinofchaos> And then export basically unwinds that.
<merlinofchaos> Which is the whole point.
<dawehner> it's quite amazing that the benchmarking/xhprof didn't showed set_default_options that prominent

So the roadmap is actually

* Test unpack_options and set_options_defaults
* Refactor unpack_options to support setting the default options

Related issue #1388078: Optimize unpack_options by not calling init_localization all the time.

Another related issue #1388092: Test set_options_defaults and unpack_options

If someone is using views in another language then english, the expectation might be to have all configuration translated on the initial adding, which would be removed with this patch, so i'm not sure whether it still makes sense to do it.

This is still a problem, and it's causing a mess for my team and me. The t() call on line 90 is causing double translation in many cases for us, which leads to tons of bogus strings in the strings table (i.e. Chinese translation text being inserted as English source text.).

There were additional double translation issues, but those were resolved by updating to 7.x-3.13+28-dev; removing this t() call--actually, dumping the entire elseif block (lines 89:91) would be fine--fixes the last of those double translation issues for us.

@dawehner, considering #8, maybe best to remove this and deal with the unpacking in a separate issue?

The other option is to static cache existing translations because most of them are like 'more', but from translation point of view this should be done at the time of creation with t() on the string so that the potx and other tools can statically analyze them.

So.. is this "RTBC" or "won't fix"? I'm confused.

I think it may need @dawehner to make that call but rtbc from my point of view. t($var) is not useful for the extraction of strings and discouraged.

We've applied this change. Thanks

We used this patch and found an issue with it.
when using this patch, our "Show more" pager that we use with views infinite scroll failed to translate to our other language.

Do not use this patch if you plan on supporting more than one language.

backing off the patch restored our pager label translation.

Going to mark this won't fix:
Please consider this instead:
#2760405: Reduce the calls to the same t() in views_fetch_fields()

For improved views performance, use these instead.
#2760405: Reduce the calls to the same t() in views_fetch_fields()
#2653214: Replace views_include() with include_once
#2653266: Avoid excess t() calls in field_views_field_default_views_data()
#2760419: get_option() micro optimization

Thanks for listing all my performance issues:)

@joelpittit , your performance fixes are excellent, really appreciate the profiling screenshots too. I'm a few months away from setting up xhprof myself, got some other things brewing first but xhprof is definately on my TODO list. In the middle of building a super infrastructure for myself, openvz , redundancy , PHP 7 , new servers, rsynced and offsite warm spare, all disks raid1 , spent about 5000$ so far.

That sounds like it's going to be screaming fast!

Most of the xhprof were done with drupalvm. Nicely installs it for you(just remember to turn off xdebug because it can change the results). It can be a pain to setup, may switch to local tideways soon(also on Drupalvm)