As per the lengthy discussion in #1216094: We call too many things 'language', clean that up calling language objects and language codes as well as other things language in the API/schema is very misleading. The process we choose there is to convert all language code APIs and schema elements to langcode for consistency. So this issue would involve changing the language.language column to be language.langcode. Should be implemented as a followup to #1301040: Move language listing functionality from locale.module to a new language.module. Marking as postponed for that.
Other objects like nodes, fields and path aliases are converted in related issues. See a full issue list for this conversion at http://drupal.org/project/issues/search/drupal?issue_tags=langcode
Comment | File | Size | Author |
---|---|---|---|
#84 | 1357918-82.patch | 226.4 KB | no_commit_credit |
#84 | interdiff.txt | 2.04 KB | no_commit_credit |
#78 | 1357918-78.patch | 226.81 KB | Gábor Hojtsy |
#75 | 1357918-75.patch | 226.82 KB | Gábor Hojtsy |
#72 | 1357918-72.patch | 226.22 KB | Gábor Hojtsy |
Comments
Comment #1
sunCleaning up bogus/duplicate "API cleanup" term.
Comment #2
Gábor Hojtsy#1301040: Move language listing functionality from locale.module to a new language.module landed, so opening this one up.
Comment #3
Gábor HojtsyHere is a quick initial patch. Will probably come with massive fails but its a start. Its pretty delicate to avoid changing user, node and comment related code while we do this to constrain the scope of this patch
Comment #5
Gábor HojtsyFixing several other ->language instances. Will no doubt turn up more through the tests.
Comment #7
Gábor HojtsyEven more places converted.
Comment #8
Gábor HojtsyAbove patch was for #1357912: Convert path language code schema to langcode, please disregard. Here is the right one.
Comment #10
Gábor HojtsyFound some code to be affected in user.module, filter.module, etc.
Comment #12
Gábor HojtsyEven more places where langcode was missed. This is turning to be a huge megapatch. Hope to have it landed soon since managing it will be a nightmare :/ The language negotiation related fixes now included should fix many of the notices around not finding $langcode on objects not properly returned before by the negotiation system.
Comment #14
Gábor HojtsyDrupal web test case had language initialization which in fact broke most of the tests. Changing that leads to much better results.
Comment #16
Gábor HojtsyComment token and node.module still had lingering language key based code.
Comment #18
Gábor HojtsyFound two places where I was going over converting node related ->language properties which should be left alone for the scope of this patch. That resulted in many node language related fails. (This actually reduces the patch size a bit).
Comment #19
Gábor HojtsyTagging up so we can find the langcode API changes later.
Comment #20
sunStale debugging code.
btw, debug() automatically performs a var_export() already, no need to do that manually.
We need a update function for this schema change, no?
Lastly, could we enhance the issue summary to clarify what is going to happen with the other language schema columns, such as node.language and field data tables?
Comment #21
Gábor HojtsyThanks for the review!
- Great find on that debug hunk, removed.
- No, an update function is not needed since languages are needed in the early bootstrap for update, so the patch includes changes in the update.inc code to do the update in this hunk:
- Finally, added this sentence to the issue summary: "Other objects like nodes, fields and path aliases are converted in related issues. See a full issue list for this conversion at http://drupal.org/project/issues/search/drupal?issue_tags=langcode"
Comment #21.0
Gábor HojtsyAdd issue list link for related issues.
Comment #22
Gábor Hojtsy#21: language-to-langcode-21.patch queued for re-testing.
Comment #24
Gábor HojtsyRerolled patch to fix the apply problems.
Comment #25
Gábor Hojtsy#24: language-to-langcode-24.patch queued for re-testing.
Comment #26
sunI guess this is RTBC then.
Comment #27
Gábor Hojtsy#24: language-to-langcode-24.patch queued for re-testing.
Comment #28
Gábor Hojtsy#24: language-to-langcode-24.patch queued for re-testing.
Comment #29
catchOK this looks fine in general but there's at least one occurrence missed:
From node.install:
Also from system.install
I'm fine if we don't change these schemas in this patch, that could happen in separate follow-ups, but the docs should be updated at least.
Comment #30
Gábor Hojtsy@catch: its pretty impossible to change all schemas to use langcode instead of language. There is at least one more patch queued up already at #1357912: Convert path language code schema to langcode and the langcode tag is used to track future issues that are going to change all those. (http://drupal.org/project/issues/search/drupal?issue_tags=langcode).
You are right though that the comments for the schema are not properly updated. I found four occurrences (comment.install and locale.install were also affected on top of your two finds above) and all are fixed in the attached patch. Otherwise unchanged.
Comment #31
catchThanks for the quick follow-up. Committed/pushed to 8.x.
This will need a change notification.
Comment #32
catchComment #33
Gábor Hojtsyhttp://drupal.org/node/1399806 added with both a developer example and a themer example (with possible D8 theme changes in the future in mind).
Comment #34
Gábor HojtsyBTW next one up in this conversion is #1357912: Convert path language code schema to langcode which has a patch that was previously passing tests (but was made not possible to apply by this patch, so just updated there). Reviews welcome there! Thanks a lot!
Comment #35
sunA proper D8->D8 update would have revealed a critical upgrade path bug.
Comment #36
Gábor HojtsyThe primary key change is definitely not going to work this way in the D7-> D8 upgrade path as far as I see since there is no language table in D7.
Comment #37
sunWell, then rename it first. ;)
I don't really see why this should not work. Let's see what the testbot thinks.
Comment #39
Gábor HojtsyLooks like you are now not actually calling language_update_8000() vs. the previous patch, so it breaks differently now. You let the bootstrap through before the update with the wrong schema, so there you get those test fails AFAIS.
Comment #40
Gábor Hojtsy@sun: so here is the patch that reflects your intentions (AFAIS).
Comment #42
Gábor HojtsyWith that the problem is that the language update will run in itself and by that time the language column is no more. By adding appropriate conditions in the update function, we can avoid running parts of it as needed. Adding a db_field_exists('language', 'language') to the DB change and an isset($language_default->language) to the variable update. Should pass now.
I still don't agree that this is any more elegant compared to having it all in the bootstrap prepare code. And we could easily make it work for D8 -> D8 updates in the bootstrap prepare code too by using the current conditionals there. Anyway, I hope this should pass tests now and if this is deemed better, then so be it. The real point is fixing the remaining issue with not updating language_default in the original patch and it is done either way.
Comment #43
Gábor HojtsyAlso elevating. This is at least a major upgrade path bug.
Comment #44
sunExcellent, thanks!
Comment #45
catchThere's no tests added here, and the existing ones didn't catch this, so can we add some?
Comment #46
Gábor HojtsyHere is a suggested text expansion to add language_count and language_default to the filled database. English and Catalan were already present in the DB, so I decided to make Catalan the default here. I think this should cover the data requirement we need. I've also added test coverage to test language_default() and language_list() output on the D8 end, so that Catalan is returned to be the default and that is the only one returned to be the default.
Posting a diff including the above patch and without so that we can see if it properly fails / passes.
Comment #48
Gábor HojtsyUps, fixing PHP error in the test. Same patches otherwise.
Comment #50
Gábor HojtsyOk, well, the serialization of language_default was not proper either (due to UTF8 byte differences). Fixing that. Let's see if this correctly passes and fails now :)
Comment #52
Gábor HojtsyAll right, one more curly brace was missing in the serialized language_default. I did run this test now locally as well. It looks like the test system *itself* will produce notices until the update.php code actually runs that updates the language_default variable early in the first bootstrap. I reproduced those notices on my machine. Not sure how could we fix that.
Comment #54
Gábor HojtsyAll right, so as can be seen the "pass" patch actually passes with notices, the fail patch produced the two test failures expected. However, since all those notices are added, it is obviously not feasible to commit these to core. Now because the notices come from the test system running on a not yet updated D7 database, how could we make sure that the test system is not affected? We could extend the setUp() in UpgradePathTestCase to run update_prepare_d8_bootstrap() let's say, not sure how pristine the update testing would be at that point. Any good ideas? @sun? @catch?
Comment #55
Gábor HojtsyOk, well up on more testing it turns out that the bootstrap prepare code does run in time BUT the test system has outdated cached values in place. like $conf['language_default'], the internal cache of language_list() and the global $language values. If we clear / reinitialize those from the database / backend data, the test system becomes perfectly happy. Putting this right before it causes a problem. Now this needs serious feedback :)
Comment #57
Gábor HojtsyAll right, I must have had a test glitch where it actually passed on my machine. I cannot reproduce it passing anymore. Doh. My best patch is in #52 and I request that someone please take this over from me because I wasted way too much time on trying to work around this and apparently failed. Please help.
Comment #58
Gábor HojtsyComment #59
Gábor HojtsyMarking needs focus.
Comment #60
Gábor HojtsyUploading 'pass' test with fixed PHP syntax. It will still not pass. Still needs serious help to figure out clearing the test environment properly to be able to pass through this. Please!
Comment #62
swentel CreditAttribution: swentel commentedWow, this is indeed a hard one to crack. Attaching a patch which reduces the notices to 46, also letting the 'Bare upgrade tests' pass. I've removed drupal_language_initialize() in the test method and did the reset in language_update_8000() as it's (imo) a much cleaner solution to fix notices outside the testing system.
However, still not fully passing, I'll check this further out tonight.
Comment #63
swentel CreditAttribution: swentel commentedSetting to review to make sure the testbot also has reduced notices compared to my local install.
Comment #65
swentel CreditAttribution: swentel commentedActually, the reset in language_update_8000() is not even needed, Removing the resets in performUpgrade() makes the notices drop already. Digging further.
Comment #66
swentel CreditAttribution: swentel commentedOk, turns out this was easy in the end, but it took me a couple of iterations to find out what went wrong. Turns out that when language_update_8000(); is called in update.inc the variables weren't loaded yet and thus variable_get('language_default'); returns zero and doesn't set the language_default with the new langcode key. Simply changing the boostrap constant in update_prepare_d8_bootstrap to DRUPAL_BOOTSTRAP_VARIABLES fixes all notices without any ugly hacks.
Comment #67
plachBrilliant! However there is this critical issue that is messing with more or less the same portion of code and also adding the variable bootstrap: #1331370: [Upgrade path broken] Stored include paths need to be updated to /core before running the upgrade path. Perhaps it would be wiser to get that in before, and then reroll the patch here, because the bootstrap preparation code is being refactored quite a bit over there. Moreover also that one just needs a RTBC sign off and I have the feeling that the code here would be simpler if the other patch were committed before.
Comment #68
Gábor Hojtsy#1331370: [Upgrade path broken] Stored include paths need to be updated to /core before running the upgrade path is now committed so this will not apply anymore I guess. Also that does the bootstrap already, so this patch can be simpler. @swentel: can you help move this forward?
Comment #69
swentel CreditAttribution: swentel commentedRerolled, language_default wasn't in the gz file, so I hope I did it ok. Patch locally passes, so fingers crossed :)
Comment #70
Gábor Hojtsy@swentel: thanks!
In general we are having issues with keeping the dump up to date and looks like it would be better to move to a componentized model for the upgrare test. Would you be interested in looking into that with this patch? That would allow us to move forward much quicker in other patches! I've looked into how D7 componentized the update tests, summarized my findings a bit in http://drupal.org/node/1410096#comment-5558268
Comment #71
clemens.tolboomI just added a comment to #1182296: Add tests for 7.0->7.x upgrade path (random test failures) as I don't understand the gz dumps for db files.
Comment #72
Gábor HojtsyHere is a rework of the above patch that would not make @chx cry (I'd imagine :). The updgrade test system is built with componentization in mind (at least where possible), and now that we start to add language asserts in the test, we ought to set up our own .test file for it. We can/should also introduce a db-addon file for this which contains the language data. We can take advantage of the system's support for non-gz dump files, so since the language add-on would be simple, we can keep it diffable and make people's work and patch rerolls in related issues *way* easier. Rerolling patches against the gz dumps is a huge pain.
So the attached patch takes the cues from the componentized D7 dumps/tests (http://drupalcode.org/project/drupal.git/tree/refs/heads/7.x:/modules/si...) not yet introduced in D8 yet. The language related dump data is moved out to a drupal-7.language.database.php (not locale, because we want to work with node, comment, field language, etc. type of data here too). It also introduces an upgrade.language.test file which builds on the core filled test file and adds on this dump. Then runs its own asserts.
So we leave the generic filled dump and test pristine and can work in our own addon dump/test to prove language updates are good. The filled dump got all the data removed that you can see added in the drupal-7.language.database.php, all others are left intact.
This passes on my local machine, hope it passes on the testbot.
@swentel, all, please review, because this blocks changes like #1410096: Convert comment language code schema to langcode and friends, basically any language related schema change, of which we are working on / planning a lot.
Comment #73
Gábor HojtsyBTW wrote some upgrade test docs based on all this work at http://drupal.org/node/1429136. IMHO let's get this refactoring/fix in, then we can move on with a simpler workflow for writing the language update tests.
Reviews please!
Comment #74
clemens.tolboomGreat Upgrade tests ... tnx Gabor. I could not have written that. Componentization is a great improvement. Some remarks as I'm still not able to generate the full patch (sorry)
Why are these records inserted and table created? There is a relation to the tests. So this imho could use some more comments. Maybe just something like
"
Inserting blocks for 'reason'
Installing table locale_source, locale_target, language for 'reason'
"
The use of block ids is probably a little brittle as adding new blocks to the global db set would collide with these. Block schema uses a serial so we can drop these values all together right?
Comment #75
Gábor Hojtsy@clemens: thanks for the review. Here is an updated patch with plenty more comments in the dump.
Keep the reviews coming!
Comment #76
clemens.tolboomI expected 'the why' add two rows.
This probably help people in the future not to add too much rows?!?
I am now puzzled about for what test part we need these blocks. Or are these just the defaults of local.install?
The bid is removed from values but not from field list. So test should fail.
Same for documenting languages. Why these data?
Maybe it is just enough to just add @see 'unit test a' (allowing IDEs to jump to tests)?
I try to chew on this for http://drupal.org/node/1429136 too.
One last question: Is there an issue about 'making chx happy' concerning test data sets?
Comment #77
clemens.tolboomTest failed as expected.
catch answered 'make chx happy' referring to #1364798: Impossible to generate meaningful diffs of upgrade db dumps
Comment #78
Gábor HojtsyUpdated patch with only the
missing bid fixedextra bid removed. I don't think we should document the test data in the detail you are looking for. That would require us to update the comments in the dataset every time we update the tests, which is likely going to fall unmaintained. Also, this dump already has plenty more comments compared to the dumps in Drupal 7.On the blocks, well, we don't currently need them. We can remove them, but they were in the filled dump, so then we'd lose those blocks and would require extra work when we come to moving those blocks from locale module to language modul and I don't want to inflict that extra work on anybody.
Comment #79
swentel CreditAttribution: swentel commentedDid a really quick review on the new stuff, and I like this idea a lot. Besides finally becoming readable, It gives us a more flexible way to test different Drupal 7 situations. I haven't found any practical use cases yet, but I can imagine an upgrade could maybe fail depending on configuration of your site (n* languages, n* node types where a couple are translatable, some not etc).
Marking RTBC, let's get Catch's opinion. Great work Gabor!
Comment #80
xjmAwesome!
Couple points on the doxygen:
These should all start with third-person verbs.
Also, several of the new files are missing
@file
docblocks.Comment #81
catchAdding the extra data in a separate file uncompressed is fine with me, I thought we were doing that elsewhere already.
If we end up doing sun's idea, we'll still need to get the data into the db, so there's a chance we'll still need these partial dumps anyway for the actual data.
Didn't do a full review or anything yet.
Comment #82
Gábor Hojtsy@catch: thanks for the quick feedback! We don't do the componentized db dumps in D8 yet, it is only done in D7 (and D7 does not even gz the base filled database either). I think going this way will make writing upgrade path tests much easier for language stuff (of which we need to do a lot these days).
Comment #83
catchAhh, in this case I think it's a case of the D8 upgrade tests lagging behind the D7 ones (largely because we had zero upgrade path coverage for 7-8 for about 3-4 months last year when the 7.x upgrade and tests were removed) - but either way we should definitely do that. For several 6-7 upgrade path tests I manually added lines to insert queries for dumps to get the right data in, it's much easier to do that with an approach like this.
Comment #84
no_commit_credit CreditAttribution: no_commit_credit commentedAttached adds
@file
docblocs to the two new files (which in the case of the dump is just a matter of adding@file
) and adds verbs. Note that I also reverted an out-of-scope cleanup in the main upgrade path test file (that removed a double period in a summary); that might collide with other cleanups.Comment #85
Gábor HojtsyLooks good, I think this is still RTBC if it passes tests, which is more than likely :)
Comment #86
Gábor HojtsyFix tags :)
Comment #87
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #88
Gábor HojtsyYay, thanks. This just unblocked #540294: Move node language settings from Locale to Node module and #1410096: Convert comment language code schema to langcode which are going to extend the tests/dumps added here :)
Comment #89
Gábor HojtsyAlso remove sprint tag.
Comment #90
Gábor HojtsyOk, @Dries, you forgot to commit the new files from the patch. (http://drupalcode.org/project/drupal.git/commit/fdca0419a524917d0d8b9126...)
Comment #91
xjmLooks like this was fixed in:
http://drupalcode.org/project/drupal.git/commit/72a6343
Comment #92
Gábor HojtsyOff-sprint then :)
Comment #94.0
(not verified) CreditAttribution: commentedUpdate related issue text slightly.