about 10576 (style language stuff) and 10586 (extension front)

gistfile1.weechatlog

[20:23] == imkingdavid [ad4242aa@phpbb/developer/imkingdavid] has joined #phpBB-dev
[20:23] == mode/#phpBB-dev [+o imkingdavid] by ChanServ
[20:25] <@imkingdavid> can another developer review https://github.com/phpbb/phpbb3/pull/545 ? it's pretty much ready (I tested it some time ago and it worked) but I'd like a second opinion before I merge it
[20:30] == nw- [~pie@72.245.42.19] has joined #phpBB-dev
[20:48] <nw-> imkingdavid: rfc for http://tracker.phpbb.com/browse/PHPBB3-10576 ?
[20:51] <nw-> we can have a prosilver/lang/en.php.sample possibly regarding comments in https://github.com/phpbb/phpbb3/pull/530
[20:53] == Marshalrusty [~Marshalru@phpbb/manager/pdpc.21for7.marshalrusty] has quit [Disconnected by services]
[20:53] == YuriyRusko [~Marshalru@phpbb/manager/pdpc.21for7.marshalrusty] has joined #phpBB-dev
[20:53] == mode/#phpBB-dev [+o YuriyRusko] by ChanServ
[20:54] <nw-> &$lang = array() - that looks very suspicious
[20:54] <nw-> have you run that code with all php complaining on?
[20:56] <nw-> from what i understand if you don't pass a $lang the function will be a no-op as it will modify that empty array and then throw it away, provided it even works
[20:56] <nw-> (20:58:09) nw-: have you run that code with all php complaining on? - ideally on php 5.4
[20:56] <nw-> so my guess would be the defaulting should be taken out
[20:57] <nw-> wait
[20:57] <nw-> you don't even use it
[20:57] <@imkingdavid> it isn't used explicitly in the function, but if we don't pass it, then the $lang in the included files don't get added to it
[20:58] <@imkingdavid> alternatively i could do global $lang if you like that better
[20:58] <nw-> why is phpbb_add_style_lang global?
[20:58] <nw-> session has an add_lang
[20:58] <@imkingdavid> want me to move it into session -> add_style_lang() ?
[20:58] <@imkingdavid> and remove the second paramter (using $this->lang)
[20:59] <nw-> i am leaning in that general direction
[20:59] <@imkingdavid> okay
[20:59] <nw-> it seems to be consistent with add_lang
[20:59] <@imkingdavid> and for your initial question, the rfc is the first link in the ticket
[20:59] <@imkingdavid> s/first/second
[20:59] <nw-> indeed
[21:00] <nw-> i'm still reading add_lang
[21:00] <@imkingdavid> okay
[21:00] <nw-> $lang is global for language files, correct?
[21:01] <A_Jelly_Donut> yes
[21:02] <A_Jelly_Donut> imkingdavid, was the PR for php version update a WIP?
[21:02] -NickServ- imkingdavid_!~imkingdav@pool-173-66-66-170.washdc.fios.verizon.net has just authenticated as you (imkingdavid)
[21:02] <nw-> this looks like a bit of a mess
[21:03] <nw-> $lang is global but there is also a language array in session class?
[21:03] <@imkingdavid> A_Jelly_Donut : https://github.com/phpbb/phpbb3/pull/615 <--- this ? that is pretty much done
[21:03] <@imkingdavid> nw- : yep
[21:04] <A_Jelly_Donut> imkingdavid, no changes to includes/acp/acp_main.php
[21:04] <nw-> no, $this->lang isa function
[21:04] <A_Jelly_Donut> $user->lang is also an array
[21:04] <@imkingdavid> ahh I haven't made a PR for that. I'm waiting for the php 5.3.2 announcment to be posted
[21:04] <nw-> the heck
[21:04] <@imkingdavid> $this->lang is both an array and a function
[21:04] <A_Jelly_Donut> ok, got it
[21:04] <@imkingdavid> depending on if you use () or []
[21:05] <@imkingdavid> the function iirc just grabs the array element
[21:05] <@imkingdavid> and does some sprintf() for any further arguements
[21:05] <A_Jelly_Donut> the function also handles pluralisation
[21:05] <nw-> the function should be doing plurals, yes
[21:05] <@imkingdavid> ah right that too
[21:06] == ikd|linux [~imkingdav@phpbb/developer/imkingdavid] has joined #phpBB-dev
[21:06] == mode/#phpBB-dev [+o ikd|linux] by ChanServ
[21:06] <nw-> where is $this->lang initialized?
[21:06] <A_Jelly_Donut> user::setup() which I believe calls user::add_lang
[21:07] <nw-> i mean the property is never initialized
[21:07] <nw-> i can only find array accesses on it
[21:08] <nw-> phpbb_get_first_existing_path - why defaulting there?
[21:09] <nw-> it does not make any sense
[21:10] <@imkingdavid> what, the parameter default for $paths? idk...
[21:10] <nw-> yes
[21:10] <@imkingdavid> i'll remove it
[21:10] <@imkingdavid> we can type hint right since we're using 5.3.2?
[21:10] <@imkingdavid> type hint arrays, that is
[21:10] <nw-> yes
[21:10] <A_Jelly_Donut> nw-, session.php:1628?
[21:10] <@imkingdavid> k
[21:10] <nw-> umm
[21:11] <nw-> don't ask me though i don't use that stuff
[21:11] <nw-> but we do typehint elsewhere
[21:11] <nw-> git grep '(array ' should find some hits
[21:12] <nw-> phpbb_get_first_style_lang_path - in what situation would someone pass an empty style to it?
[21:13] <nw-> my feeling is it should actively fail when given an empty style
[21:13] <nw-> but this is php so maybe the way it's written is the best we can do
[21:13] <@imkingdavid> would you rather I use a trigger_error()?
[21:13] <nw-> that seems too drastic
[21:14] <A_Jelly_Donut> yeah, pretty sure $user->lang is really only a reference to the global $lang o_0
[21:14] <nw-> do we use trigger_error in similar situations elsewhere?
[21:14] <nw-> A_Jelly_Donut: $lang = &$this->lang;
[21:14] <nw->  is the other direction
[21:15] <nw-> there is no assignment to $this->lang
[21:15] <@imkingdavid> nw- : i don't know, but i wouldn't feel comfortable doing so here. this is supposed to be able to not find any language files in the style directory and still keep moving on without errors
[21:15] <nw-> ok leave it
[21:16] <nw-> is that logic copy pasted from somewhere?
[21:16] <nw-> phpbb_get_first_style_lang_path
[21:16] <nw-> list of language files i mean
[21:16] <A_Jelly_Donut> yes, I see that.  but obviously $user->lang contains some values
[21:17] <nw-> A_Jelly_Donut: from where?
[21:17] <nw-> that is the question
[21:17] <@imkingdavid> nw- : no, that is just a list of files to check on. first it looks for the user's specified language, then it looks for the board default, then it falls back on english (which in many cases all three will be the same thing)
[21:18] <nw-> yes
[21:18] <nw-> but we should already be doing that
[21:18] <nw-> for "normal" language entries, no?
[21:18] <@imkingdavid> the language files in a style language directory should be named with their iso code
[21:18] <@imkingdavid> these aren't files in the language directory
[21:18] <nw-> session.php line 2078
[21:18] <@imkingdavid> so we don't look for them elsewhere
[21:18] <nw-> take a look at that
[21:19] <@imkingdavid> okay
[21:19] <nw-> also lise 2106
[21:19] <nw-> line*
[21:19] <nw-> for error suppression
[21:20] <A_Jelly_Donut> references are two way in php
[21:20] <nw-> how about using a function for including language files
[21:20] <A_Jelly_Donut> so the "order" of the assignment is irrelevant
[21:21] <nw-> you also don't do the trigger error that line 2111 does
[21:21] <nw-> A_Jelly_Donut: there is no way that is true
[21:21] <A_Jelly_Donut> although that does raise some syntactical questions ...
[21:21] <nw-> i think you just made that up :)
[21:21] <A_Jelly_Donut> I've got nothing better :P
[21:22] <nw-> it reminds me of the come from statement
[21:22] <nw-> see "intercal come from"
[21:23] <@imkingdavid> nw- : so you want me to do the same thing for styles as is done for extension paths in the set_lang() method?
[21:23] <A_Jelly_Donut> no, that's how it works.  first, you point $lang and $user->lang at the same data.  then you add data to $lang
[21:23] <nw-> A_Jelly_Donut: sure, where is the pointing part?
[21:24] <A_Jelly_Donut> that's this line: $lang = &$this->lang;
[21:24] <nw-> and $this->lang has what at that point and how that got there?
[21:25] <A_Jelly_Donut> $this->lang is array()
[21:25] <@imkingdavid> where?
[21:25] <nw-> defined where?
[21:25] <A_Jelly_Donut> var $lang = array()
[21:25] <@imkingdavid> oh right... in the user class, not the session class
[21:25] <nw-> that's in user
[21:26] <nw-> great
[21:26] <nw-> ok
[21:29] <nw-> (21:26:54) imkingdavid: nw- : so you want me to do the same thing for styles as is done for extension paths in the set_lang() method? - a little confused what you mean there
[21:30] <nw-> basically code duplication should be taken out
[21:30] <nw-> how the result should look i can't presently say
[21:31] <nw-> but any differences in behavior between add_lang and add_style_lang should probably be documented and have a reason
[21:32] <nw-> phpbb_get_first_existing_path is fine in global scope
[21:32] <nw-> phpbb_get_first_style_lang_path i'm not yet sure where it should be
[21:32] <nw-> phpbb_add_style_lang should probably be on session
[21:33] <nw-> phpbb_get_first_style_lang_path and add_lang/set_lang should have common logic refactored out
[21:34] <@imkingdavid> hmm okay
[21:38] <nw-> skimmed the rfcs
[21:38] <nw-> Second, why include two files? Second file will override first file, making first include pointless. I think translations should include all text, not only modified lines. - do we have  a precedent for that?
[21:39] <nw-> i'm guessing we don't
[21:39] <nw-> styles are meant to translate everything, right?
[21:39] <nw-> i mean language packs
[21:39] <nw-> i'm guessing we don't approve language packs that don't translate everything
[21:40] <nw-> therefore there is no need to fall back
[21:40] <@imkingdavid> the two includes are for template inheritance
[21:40] <@imkingdavid> if we are inheriting from a parent, we want to make sure we get its language stuff
[21:40] <nw-> yes that probably should stay
[21:40] <@imkingdavid> style language files are only meant to have things that are specific to that style
[21:40] <nw-> what fallbacks do exist?
[21:41] == rxu [~Miranda@phpbb/developer/rxu] has joined #phpBB-dev
[21:41] == mode/#phpBB-dev [+o rxu] by ChanServ
[21:41] <nw-> basically i think we should assume that all *translations* are complete
[21:42] <nw-> i.e. if there is a language file for e.g. ru we should not fall back to en at all
[21:42] <nw-> we only fall back to en if there is no ru
[21:42] <@imkingdavid> right the only fallback occurs when the language file as a whole is missing
[21:42] <nw-> yes/no/maybe?
[21:42] <nw-> ok
[21:42] <@imkingdavid> we don't fallback for individual language entires
[21:42] <@imkingdavid> entries
[21:42] <nw-> so then the last point is extensions
[21:42] <@imkingdavid> just like in the rest of the language system
[21:42] <nw-> ok, great
[21:42] <nw-> extensions
[21:43] <nw-> say there is a style adding language entries
[21:43] <nw-> or having its own language entries
[21:43] <nw-> how is it meant to be translated? what is the file structure of the translation?
[21:43] <@imkingdavid> i don't understand what you're asking
[21:43] <nw-> ok, start over
[21:43] <nw-> I have a board
[21:44] <@imkingdavid> okay
[21:44] <nw-> i install style greensilver which has styles/greensilver/language/en.php
[21:44] <nw-> i also have a russian board therefore i install official russian language pack
[21:44] <nw-> how do i get greensilver russian translation? what files does it provide?
[21:46] <@imkingdavid> the style language files are packaged with the style, not the global language file, so if we want a translation, the style author needs to provide that language in the styles folder
[21:46] <@imkingdavid> styles language folder*
[21:46] <nw-> so translation will provide styles/greensilver/language/ru.php
[21:46] <@imkingdavid> right
[21:46] <@imkingdavid> the language file is named with its language iso code
[21:46] <@imkingdavid> and put into the styles/<style>/language
[21:47] <nw-> if that works it looks ok to me
[21:47] <@imkingdavid> or lang/ rather
[21:47] <@imkingdavid> that's how it works as currently implemented in the pr
[21:47] <nw-> alright
[21:48] <nw-> then it just needs to be DRYed up
[21:48] <nw-> what is the status of your front controller pr?
[21:48] <@imkingdavid> what's "DRYed"? lol
[21:48] <nw-> don't repeat yourself
[21:48] <@imkingdavid> ahh lol nice
[21:48] <@imkingdavid> okay
[21:49] <@imkingdavid> umm the front controller works
[21:49] <nw-> as popularized by the ruby crowd
[21:49] <@imkingdavid> just needs tests
[21:49] <@imkingdavid> functional tests
[21:49] <@imkingdavid> I wrote the tests but need to make a way for it to copy fixtures into the testing board
[21:49] <@imkingdavid> for the duration of the tests and then delete them
[21:49] <@imkingdavid> that's currently where I'm stuck
[21:50] <nw-> my general feeling is deleting is tough
[21:50] <nw-> i would concentrate on copying
[21:50] <@imkingdavid> okay
[21:50] <nw-> and then if you have to truncate the entire database before each test
[21:50] <nw-> we need to have that stuff working before worrying about how long it takes to run
[21:51] <nw-> php fixtures, hmm
[21:51] <@imkingdavid> right. btw are there functions in the test framework that run before a group of tests instead of before each test? because otherwise it's going to be copying the files each time the a test is run
[21:52] <@imkingdavid> afaik setUp and bootstrap are run for each test
[21:52] <@imkingdavid> but I may but misunderstanding them
[21:52] <nw-> setup should be run for each test
[21:52] <nw-> see setupbeforeclass
[21:53] <@imkingdavid> okay
[21:53] <nw-> hm, do dbunit fixtures work for functional tests?
[21:53] <@imkingdavid> i'm not sure. i'm really new to the testing framework as a whole
[21:53] <nw-> for front controller you should not need that much data
[21:54] <nw-> Ok ok - we are all PHP evangelists - but still - not everything in Ruby is bad.
[21:54] <@imkingdavid> lol
[21:55] <nw-> i need to build out a fixture solution for my python app still
[21:56] <nw-> the fanciest fixture solution i have built so far involves sequencing tests correctly and having earlier tests create data needed by later tests
[21:56] <nw-> works very well
[21:56] <A_Jelly_Donut> I'm pretty sure phpunit fixtures work for all types of tests
[21:57] <nw-> http://www.frontalaufprall.com/2008/05/05/php-unit-database-fixtures-the-ruby-way/
[21:57] <nw-> now i'm going to lose wifi
[21:59] <@imkingdavid> well I'm going to get some sleep. i'll look into this more tomorrow... good night all.
[22:00] <@rxu> while official langpack has a maintainer/translator, styles don't have any similar. So unless a user translates it himself, he can stay with untranslated style. Do we need a fallback for styles language if so?
[22:01] <@imkingdavid> rxu : imo we should have it fall back for all language entries to the board default
[22:01] <@imkingdavid> anyway, it's up to the style author to translate or get his users to translate his style language, imo