As per #1216094: We call too many things 'language', clean that up we are converting schemas and parallel APIs to use 'langcode' instead of 'language' where a language code is involved. The use of 'language' inconsistently for language objects and language codes in Drupal 7 keeps throwing people off the cliff and makes understanding the API much harder.
This issue is about node.module needing the same conversion to move from $node->language to $node->langcode. Will need updates and upgrade tests but first of all it would be a well focused search and replace wherever node language is involved. The tricky part is that field language is a different story, this should focus on node language to keep scope limited.
Comments
Comment #1
tstoecklerTaking this one for now.
Comment #2
tstoecklerAll right.
This is the first try.
It passes node tests and at time of creation half of comment tests locally.
So I want to see what other parts might fail. Translation still fails I think (also still running), but I think that should be self-contained enough for a first round of a complete run-through.
I have not started on upgrade tests.
Comment #3
tstoecklerSo here's what I think the upgrade tests should do:
1. Create nodes with LANGUAGE_NONE
2. Create nodes with something other than that
3. Create a translation set with translation.module
And then in D8 assert that each of those still work like in D7.
Anything else?
Comment #4
tstoecklerThe latest patch in #1410096: Convert comment language code schema to langcode reminded that there is this little thing called node revisions...
We should test those as well.
Comment #6
tstoecklerAha! For whichever reason node_revision doesn't have a language column. That is weird, but less work for me :)
Comment #7
tstoecklerAll right, this should be almost finished.
- On my system AjaxMultiFormTestCase fails, and I didn't figure out why. If the bot has the problem someone would need to investigate that.
- I did the bulk of the work with the upgrade tests, but I need to run now, so I couldn't finish them. If you apply the patch and run the 'Language upgrade tests' you will see that I drupalGet() node/40/translate which is a translation overview table. I don't assert anythingon that yet, but we probably want to assert that the Correct languages are listed and that the links to the translations are correct.
Comment #8
tstoecklerUnassigning for now, although if someone reviews this, I will probably pick it up in the next few days.
Comment #9
fubhy CreditAttribution: fubhy commentedWhy does it use double quotes here? (Partly before the patch) - Shouldn't be covered by this patch, I was just curious :).
Other than that it looks okay from a reviewing persepective.
Comment #10
tstoeckler@fubhy There really is no reason. Somewhere around this code, there was one array key which used double-quotes to embed a variable, so I guess this was some sort of "consistency", but I really hate stuff like that.
I hope no kittens will be harmed if I change that in the next reroll.
Comment #11
Gábor HojtsyThis all looks very nice, and it even has extensive upgrade tests! Superb! Some likely minor notes:
$previous_language should be $previous_langcode
Does this not have implications on the rendering/field system, and if so are those taken care of?
Yay, this looks great. Languages are not $entity :) :)
Comment #12
tstoecklerFixes the above.
Well this is definitely part of the API change for implementors of HOOK_preprocess_node or people you call node_view() directly. There are a bunch of those in core but the only one who accesses
$variables['language']
is locale_preprocess_node(), which is covered in the patch, as you can see.As I had noted in #7 I added some more assertions for the upgrade test. For the "tests-only" patch, what I did this time is only remove the update function from the patch. I think that should be more precise proof that the update function is correct.
Either way, though I think this patch is good to go. I also tried this manually created a bunch of languages and translation etc. and everything works. So I would say, if there are no code style issues, this should be RTBC.
Comment #13
tstoecklerOops, forgot to set needs review.
Comment #14
tstoecklerAlso, re-assigning. Now that it is basically finished, if something comes up in reviews I guess I'll drive this home so @GáborHojtsy can mark this RTBC.
EDIT: Or anyone else, of course, didn't want to exclude you from the party @fubhy :)
(PS: Sorry for the noise, it's late...)
Comment #15
fubhy CreditAttribution: fubhy commentedI would prefer using single quotes here. But that is really a personal thing as I am very strict about the whole single- / double quote argument.
Just a minor thing: Using
serialize(2)
might increase readability here. Not sure how we handle that elsewhere though.Other than that it looks really nice and it also looks like it covers everything.
Feel free to directly set it to RTBC (my notes are rather proposals than real issues).
Comment #16
Gábor Hojtsy@fubhy: on the serialize() stuff, the dumps are meant to be generatable by a script from directly from the database, so they usually contain much more convoluted structures. I think we need to accept that as a matter of fact.
Comment #17
tstoecklerOk, fixed #15.1
Didn't upload a tests-only patch again, I think the point has been proven.
RTBC per #15.
Comment #18
tstoecklerAhhh, sorry!
Comment #19
Dries CreditAttribution: Dries commentedI reviewed this patch and it looks good. However, it needs a re-roll because of either #1416392: Clean up language (types) bootstrap function naming and documentation or #1410096: Convert comment language code schema to langcode going in. Let's ask for a re-test to make sure.
Comment #20
Dries CreditAttribution: Dries commented#18: 1439686-17-node-language-langcode.patch queued for re-testing.
Comment #22
tstoecklerWell, here's a reroll.
Because the language-comment and language-node upgrade tests are now integrated with one another, I changed some comments to make it more clear, what node is used for what. I guess that could use another look-over so moving to 'needs review' and not straight back to RTBC. Basically this is still the same as #18, though, so...
Comment #23
Gábor HojtsyLooks good. Has a very comprehensive test suite which verifies not only the langcode rename but that it still works with translation module. Good stuff.
Comment #24
Dries CreditAttribution: Dries commentedLooks great. Committed to 8.x. Thanks!
Comment #25
Gábor HojtsyChange notice posted at http://drupal.org/node/1451962 - removing from sprint.
Comment #26
tstoecklerAwesome stuff!
@GáborHojtsy: I added some more examples to the change notice with the theming changes that you noted in #11.2
Comment #27
Gábor Hojtsy@tstoeckler: Superb, thanks!