As per the discussion in #1293304: Break up locale.module, but how? and now that we have the negotiation functions cleaned up in #1222106: Unify language negotiation APIs, declutter terminology and friends, we can move the negotiation functionality to language module I think. There will need to be some upgrade path and cleanup on the way as well due to the required renames. Or if this is deemed too much to do at once, we can do the renames first maybe. Let's see what would be required anyway.
In this patch I did the following (items marked with (UP) will need an upgrade path, not included):
- moved the admin pages from locale.admin.inc to language.admin.inc with respective menu items and help text too.
- moved the LANGUAGE_NEGOTIATION_* constants to language.module and renamed them to language- from locale (UP)
- moved the locale_language_* negotiation functions from locale.inc to modules/language/language.negotiation.inc (UP due to negotiation assignments?)
- var locale_language_negotiation_session_param => language_negotiation_session_param (UP)
- var locale_language_negotiation_url_part => language_negotiation_url_part (UP)
- removed the special language switcher colors from locale.css, so that did not move to language.css; I think any designer would applaud this :)
- var locale_language_negotiation_url_prefixes => language_negotiation_url_prefixes (UP)
- var locale_language_negotiation_url_domains => language_negotiation_url_domains (UP)
- vars locale_language_negotiation_methods_weight_$type => language_negotiation_methods_weight_$type (UP)
- moved all negotiation info / type info hooks to language.module
- moved all language change hooks to language.module to change prefix/domain appropriately
- moved blocks to language.module (UP for the delta changes)
This will definitely not pass the upgrade tests. Also looking at how much do we need to include the modules/languages/language.negotiation.inc backend functions (that got moved from locale.inc). The less the better, however some hooks on language save for example use them. That appears to be an interesting area to get right/nice.
Comments
Comment #1
Gábor HojtsyComment #2
Gábor HojtsyRealized I did not include anything of the base install/uninstall routines. The language install needs to do the preconfiguration of negotiation and it needs to uninstall stuff. The hook_enable() implementation is not needed anymore since we move in negotiation with languages, so data syncing is ensured explicitly.
Looking at the variables removed, I'm not sure what is language_content_type_negotiation and language_content_type_default, but I left there for now. I did not find them in any other code, but they might be dynamic based on something else?
Comment #4
Gábor HojtsyUpdated several uses of the API as well as use of negotiation names without their constants and block names. Likely more to go, but interested in what the test bot would say to this.
Comment #6
Gábor HojtsyFound a couple missing includes so installation works great. Also ran a few tests and fixed wider spread failures. It is still going to fail at places.
Moved the constants to language.negotiation.inc too since they seemed to be needed more with the negotiation functions themselves then without them. (They were together before the patch in locale.inc too).
But more feedback would be great before I move on much farther. :)
Comment #8
Gábor HojtsyIt would help if I actually include the language.negotiation.inc file, right? :)
Comment #10
dawehnerOne reason for the remaining test issues is that locale.inc isn't included by the locale module if it's not needed.
So just adding it all the time, only if locale.module is enabled.
Just some general work to get down the amount of broken tests
Comment #12
dawehnerGet up a patch which applies, though there are still some broken tests.
Comment #14
dawehnerHere are some observations about the failing tests:
* Language list configuration: Simpletests uses for whatever reason 'en/...' as path, so disabling english let 404 all next request on the test
Comment #15
dawehnerSo some progress: the update tests are running now fine.
Therefore some new update functions had to be written.
Comment #17
dawehnerNow just language list configuration is left. The rest which has to be done is about getting the redirecting in the admin ui working as expected.
Even there are such many test fails it's probably just a single one left
Comment #19
dawehnerSo here is what happens.
On the current version of 8.x the languages.test doesn't enable locale.module, so locale_url_outbund_alter
isn't fired and based on that nothing is redirected.
With this patch the language negotiation is done by language.module so language_url_output_alter is runned, which rewrites the urls.
Now the question is why is the redirecting now working as expected, sadly i currently don't have an idea
why, because language_url_rewrite_session checks for active languages.
Comment #20
dawehnerSo lets see what the testbot is thinking about that.
Comment #22
dawehnerOkay this time everything should work.
Comment #23
dawehnerThe drupal community is too fast
Comment #24
das-peter CreditAttribution: das-peter commentedHere's the result of my visual review (supported by drupalcs ;) ).
Next step from my side will be a test of the upgrade path.
Comment looks broken here.
A comma should follow the last multiline array item.
Line exceeds 80 characters; contains 122 characters
Use "elseif" in place of "else if"
Line exceeds 80 characters; contains 81 characters
Line exceeds 80 characters; contains 122 characters
Use "elseif" in place of "else if"
Line exceeds 80 characters; contains 81 characters
There should be no white space after an opening "(" : array( '%language' => ...
Line exceeds 80 characters; contains 83 characters.
Inline comments must end in full-stops, exclamation marks, or question marks.
Leftover debug statements
If the line declaring an array spans longer than 80 characters, each element should be broken into its own line.
No space found after comma in function call
Leftover debug statements
Comment #25
fubhy CreditAttribution: fubhy commentedI found quite a few code style issues in the stuff that we are moving around here. So let's clean that up in the same step if you guys are ok with that.
Comment #26
fubhy CreditAttribution: fubhy commentedPlease note that there are a few slightly unrelated code style (basically only formatting) fixes but since we are moving this stuff over anyways I think this is ok.
Comment #27
dawehnerI just reviewed the interdiff. They are looking fine. Some are fixing documentation/80 chars, obvious some of the leftover debug() statements.
So from my perspective this looks RTBC, though das-peter said he wants to test the upgrade path a bit better.
Comment #29
fubhy CreditAttribution: fubhy commentedComment #30
fubhy CreditAttribution: fubhy commentedComment #31
das-peter CreditAttribution: das-peter commentedI found some flaws in the upgrade path. I'll provide a new patch asap.
Comment #32
dawehnerOkay the changes for language.negotiation.inc also looks fine. Here is a interdiff between #23 and #29
Let's see what peter is doing.
Comment #33
das-peter CreditAttribution: das-peter commentedHere we go.
Changes:
locale_language_negotiation_methods_weight_
)Comment #35
das-peter CreditAttribution: das-peter commented*grml* I'm still trying to get used to phpStorm :) Next try...
Comment #37
das-peter CreditAttribution: das-peter commentedForgot to adjust the tests to check for the proper values.
Adjusting that revealed another issue with the upgrade path - the order of some variable values wasn't maintained. This is now fixed as well.
Comment #38
fubhy CreditAttribution: fubhy commentedThanks @dereine and @das-peter! Looking good now (at least to me).
Comment #39
sunThis is a huge patch. I wasn't able to review all changes in detail, and actually I hope that we're really just moving most functions around without any other changes.
Due to its size, I more or less randomly jumped around within this patch, and identified the following issues:
None of these form constructor functions should include support files directly. Form functions always have to use form_load_include().
This bug existed before already, so no need to fix it in this monster. However, please create a follow-up issue for that. (this fix can/should be backported)
Why isn't this using module_load_include()?
Speaking of, you could add an optional
&$form_state = NULL
parameter to this helper function, so you can also call this function from form constructors. However, we can do that in a follow-up issue (as mentioned above).The manual include statements look a bit odd, given that there is a language_negotiation_include() helper function, which is used in ~70% of all other cases.
All of these comment changes/fixes are totally outside of the scope for this issue/patch. The comments are actually not changed, only re-formatted and adjusted to fit within 80 chars. Don't do this for monster patches like this.
For some reason, it looks like the file path in the old language negotiation info contained a leading slash, whereas the new one does not. No leading slash is definitely correct and consistent with the rest of Drupal code, but the update function checks for the exact path without leading slash.
We probably want to use strpos() !== FALSE here (type-agnostic comparison).
"from_brow" should be "from_browser"?
This unconditional loading of locale.inc in locale.module's global scope could use a comment that explains why that is needed. (It looks bogus to me.)
(and elsewhere) Why isn't this using module_load_include()?
Comment #40
Gábor Hojtsy@sun: yes, the patch mostly only moves stuff around verbatim. It is unfortunate that we don't have a good diff tool to be able to see that :/ I think unless there are fundamental changes with the patch, it would be great to get in and then continue refine the code at its "final" place. As-is now this 140k patch needs a reroll anytime even simple patches like #1156576: Language negotiation is undocumented get committed. So from a pure patch/resource management perspective, getting in the moving-around soon would be great.
All: can we work on resolving @sun's findings so we can get this in soon? Thanks for all your hard work!
Comment #41
dawehnerOh that's definitive a good point: #1506868: Rewrite language_negotiation_include to support form_state and use it in language.admin.inc
* Rechanged the comment in locale.install
* Used strpos() !== FALSE to detect the updated variable
*
"from_brow" should be "from_browser"?
ups, right.* +include_once DRUPAL_ROOT . '/core/includes/locale.inc'; I actually don't see a reason to do that, because it's included in locale_init anyway.
*
Let's use here language_nego...include as well.
Comment #42
Gábor HojtsyUploading for retest, since we seems like never got an answer from testbot.
Comment #44
Gábor HojtsySo looks like removing the locale.inc inclusions was not harmless after all. What is a great achievement of this patch is the almost entire removal of locale.inc. After this patch, only the following things remain in locale.inc:
There are a few bad things about locale.inc. First it contains all kinds of stuff, like as you can see, interface translation sanitization to JS parsing to providing a country list (which otherwise locale module does not manage itself). It is also included right away from locale_init() so in effect, all page loads, that have locale.module loaded will have these utility functions loaded too. This was added in D7 with/for date format localization I think (IMHO mistakenly). This resulted in massive problems like #1219236: Locale module includes 1350+ lines of unneeded code on all page loads which lead us to componentize the things from locale.inc and move stuff out gradually. Since only the above functions will be left there, I think we can easily move those to either "their right place" or en-masse to the locale.module file with which they are loaded at all times anyway. We can refactor them from there afterwards. That would help us get rid of these uninspiring includes splintered all around.
There is very minimal functionality left there after this patch, and I hope we can get rid of that file sooner than later anyway, so we don't need to debate when to include it and how :)
Comment #45
Gábor HojtsyAttached patch moves country_get_list() to system.module (it is the only place it is being used and locale.inc is included merely for this). Moved the rest to locale.module. With this, all references to locale.inc could be removed (except two places in the upgrade path that changes callback locations formerly in locale.inc).
RIP to locale.inc (finally). Now this needs review. It would be great to fast-track the inclusion of this, since its a huge code move and has the danger of getting outdated very fast. Also very logical :)
Comment #46
Gábor Hojtsyhttp://qa.drupal.org/pifr/test/251424 says it passed but no feedback here. Uploading for retest.
Comment #47
Gábor HojtsyAll of sun's concerns are now resolved, so back to RTBC as it was 2 weeks ago. It would be important to get this in sooner than later because it moves around huge chunks of code (with minimal change), and is easily going outdated :/
Comment #48
dawehnerI'm sorry for not being able to do some work in the last days.
Here is an interdiff, which looks fine, so it's RTBC from my side as well.
Comment #50
sunI didn't re-review, but if anything is left, we can fix that after committing #46
Comment #51
plachI love this patch! I found some minor issue which we could neglect, but I thought the following one might deserve a further reroll:
Now we can rely on language negotiation hooks, so we can use the regular API functions.
The language type name is supposed to be passed through t(), hence we need to avoid double-escaping it.
We can use
language_types_get_default()
here.Missing space before the opening brace.
-5 days to next Drupal core point release.
Comment #52
Gábor Hojtsyt() does not escape any strings, so it should still be used with @. Also, update functions should not use API functions like language_types_get_default() or their results would be unpredictable (if the return value of e function ever changes). Therefore I think these two changes should not have happened. I agree with the rest.
Comment #53
plachYou are right, but reason for the problem you can see below is that block's info is supposed not to be sanitized (see block.admin.inc:128). Hence I guess my motivation was wrong but the fix was right.
I reverted the locale.install hunk because I don't want a discussion to hold up this moster patch. That said we are talking about an install function not an update one, hence IMO we can assume a predictable state. Also an API function implies a contract, if the the contract changes things normally break, not only in the update (install!) functions and they must be fixed anyway. I think using API functions (which we are doing anyway, however) is only decreasing the risk of doing something wrong.
Comment #54
plachRemoved also the wrong comment in the block info hunk.
Comment #55
plachSorry for being a PITA but if we keep this code we should call
language_language_negotiation_info()
here.-5 days to next Drupal core point release.
Comment #56
Gábor Hojtsy@plach: look, I debated the use of API functions in the update function, not in the install function. You reverted the install function, which I did not debate and left the API use in the update function which I did. Just look in locale_update_8006().
Also, looks like it does indeed make good sense to use ! in the block function. So what about documenting it there that the block title is escaped later? (In place of the wrong comment you added and removed).
Comment #57
plachI'm terribly sorry, I don't know how I could misunderstand your comment. Probably I was so focused on the language.install hunk that I totally forget about the minor change in locale.install :)
Rerolled the patch as per #56. The interdiff is against #51.
A minor concern I have with this patch is it introduces a dependency from include-level code to module-level code when including
language.negotiation.inc
in some places. However this should go away when we migrate to PSR-0 classes.Comment #58
Gábor HojtsyAgreed. Looks good now, should get in as soon as possible :)
Comment #59
Dries CreditAttribution: Dries commentedPatch needs re-roll.
Comment #60
plachRerolled.
Comment #61
plachSorry, wrong issue.
Comment #62
Gábor HojtsyShould be still good to go! Let's get this in! :)
Comment #63
Dries CreditAttribution: Dries commentedReviewed. Looks good and applies now. Committed to 8.x. Thanks.
Comment #64
plachWoo-hoo!
Comment #65
Gábor HojtsySuperb, thank you! :) Added a change notification at http://drupal.org/node/1528520 - it is a logical reorganization, so I don't think it needs a CHANGELOG.txt entry. Moving off-sprint.