Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
The attached patch moves all Node-related code from Locale to Node, because it belongs there. It is split off from #375397: Make Node module optional.
Comment | File | Size | Author |
---|---|---|---|
#70 | decouple-locale-from-node-540294-70.patch | 17.48 KB | clemens.tolboom |
#66 | decouple-locale-from-node-540294-66.patch | 17.47 KB | Gábor Hojtsy |
#61 | decouple-locale-from-node-540294-61.patch | 21.15 KB | clemens.tolboom |
#59 | decouple-locale-from-node-540294-59.patch | 20.46 KB | clemens.tolboom |
#58 | upgrade-workflow.png | 6.22 KB | clemens.tolboom |
Comments
Comment #2
XanoSchema doesn't yet need to be (un)installed in this patch.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedI guess that's debugging stuff?
Comment #5
XanoGood one. That was what made at least some of the tests fail.
Comment #7
XanoLocally all tests pass, except for "Path alias functionality', which causes a 500 error and all I can find is this crash message.
Comment #9
XanoHmm, weird. The language element is #edit-language-1 instead of #edit-language in the node add form...
By the way, some of the tests that fail according to t.d.o work fine when doing locally (Path aliases with translated nodes and Translation functionality.
Comment #11
catchA lot of this code should stay in locale.module - locale is already optional, so it's fine for it to form_alter() node where that makes sense - and additionally locale is de-facto installed on less sites than node.
Comment #12
XanoThe reason for migrating this code is that the storage of a node's language already happens in Node.module. With this patch all code related to a node's language has moved to Node. Putting it in Locale was an option as well, but that would have required a migration path.
Comment #13
Gábor HojtsyInteresting. I agree in Drupal 8 modules will need to contain language support themselves if we consider language support fundamental enough. Does the goal here still apply to Drupal 8? (It will certainly not happen anymore in Drupal 7).
Comment #14
catchThe general goal of making node module optional (which is more about removing circular dependencies than the .info flag) is a good one.
Looking at the patch here, I disagree with my 2009 self - we should move the code from locale to node.module as it does here.
On top of that, we should look at whether some of these features would be better maintained at the entity level (and/or shifted further towards field-level translation).
However having it located in one place in the first place would be a good basis for that.
Comment #15
Gábor HojtsyYes, we should look at generalizing the node translation set concept to entities overall, that should be a different issue. I have not opened one for that before, now opened #1190454: Abtract translation set concept to work with entities in general. I think we should move to node module as a start, and look at generalizing it as a bigger task.
Comment #16
Gábor Hojtsy#158803: Hide "Language" column in content overview table if only one language is enabled is related.
Comment #17
Gábor HojtsyLet's make this happen in Drupal 8.
Comment #18
Gábor HojtsyComment #19
sunComment #20
Gábor HojtsyUpdated patch. This includes all changes but the test moves from Xano's patch. Based on #9. Not tested much. In summary the patch:
- moves content type language config from locale to node module
- moves node language selector from locale to node (and attempts to add a fieldset summary, not sure that works)
- renames the content type language variables to be in the node_* namespace
What's missing?
- upgrade path for the variable rename
- moving tests around
Locale took on field translation functionality as well in the meantime, which I kept there for now, but it has some code which are at the crossroads of node, field and locale module. Ha!
Interested in human and automated test review, so marking needs review (despite some stuff missing).
Comment #21
Gábor HojtsyAlso opened #1236680: Move path language settings from Locale to Path module.
Comment #22
catchShould be db_delete().
Are the variable checks enough for this as opposed to module_exists('locale') - can they get set otherwise?
Variable deletion could happen in hook_modules_uninstalled() as for when node stays enabled but locale doesn't.
Comment #23
Gábor HojtsyThey can get set and then locale disabled, which would whitescreen on locale_language_list('name'). If we modify that to a module_invoke(), that sounds like would present a separate language list if locale is not there, and could result in validation failures (the possible options would not have the selected language, but you cannot change it)?
Comment #24
Gábor HojtsyHere is a patch rerolled for current /core setup (but the previous patch applied with minor offsets there). Also updated the db_delete() as requested. Looking at the code again, the places where it checks for the variable are either in locale module directly or in translation.module (dependent on locale module). The only place it is checked direct in node.module it is coupled with module_exists('locale'), so this should be complete IMHO. I'm not sure the variables should be deleted if locale module is uninstalled but node is not. The settings are for node module, not for locale module, and when/if locale module is installed again they should be there IMHO.
The upgrade path for the variable rename and moving tests around is still missing, otherwise still needs review.
Comment #26
Gábor HojtsyInstead of trying to rework the UX of specifying languages in node forms, let's postpone that for later. It caused too many problems and would probably cause issues with reviews and getting this in faster anyway. Also fixed the translation module form alter to properly change the field type too. This should pass a lot more tests (if not all).
Comment #28
Gábor HojtsyThe remaining test fail seems to be due to LANGUAGE_NONE not used properly for no language assignment for nodes. Fixed that.
Edit: also removed checking for the removal of the node setting, that is now not removed on locale uninstall since its governed by node as discussed above.
Comment #30
Gábor Hojtsy#28: decouple-locale-from-node-28.patch queued for re-testing.
Comment #31
Gábor HojtsyOk, now that this passes, @catch any updated feedback that we should be looking at (minus the missing update function). On possibly moving tests, since the tests cover the combination of modules, it can live at either place and can just be with locale module IMHO.
Comment #32
claudiu.cristea#1074672: Allow language select to be rearranged inside node form is blocked by this and will be rerolled after committing this to 8.x.
Comment #33
Gábor HojtsyQuick reroll for the introduction of language.module and some changes in core. Let's see if this still passes tests. Also with this and #1414314: Make node and path depend on language module only for language support, get rid of locale_language_name, locale module is finally out of the game for language assignment and support for nodes. Let's move this forward.
Comment #35
Gábor HojtsyFound one more instance of old settings form item ID used to set articles to translatable. Also, some leftover code removed from locale module. Passes at least the locale tests on my computer, let's see the rest.
Comment #36
Gábor HojtsyTagging as current focus.
Comment #37
clemens.tolboomI cannot apply patch #35
Comment #38
Gábor HojtsyThanks Clemens. Indeed, the code in question moved to comment.module since the last patch was rolled above. Any other feedback?
Comment #39
clemens.tolboomThere are two competing 'workflow' the alter the node_type forms. module_exist versus node_type_form_alter
This description is competing with translation_form_node_type_form_alter
Should both the #description be the same? I guess not but not sure.
It is weird translation is using a node_type_alter ... shouldn't that code be in the node.module?
The links mentioned above should point to admin/config/regional/language.
Attached patch fixes only the link.
Comment #40
Gábor Hojtsy@clemens.tolboom: Yes, the translation module alters the language assignment option, because if you have node + language module, you get the language assignment feature (as explained in node module). If you also enable the translation module, you get language assignment + translation as well, so both options need to be explained. The translation module description has this explanation in the middle to cover that but otherwise looks identical to me to the node module one(?):
The translation module can use the node_type_form_alter() without checking for language module, because it will depend on language module (currently depends on locale module which depends on language module). So it is already ensured that language module is on, no need to check for it. (Which also means this form element is there and can be modified).
Sidenote: a big drawback of the translation module is that it only supports nodes and that it only works in a node relational way. Our D8 plans include dropping this module altogether and move in the entity_translation contrib module instead (likely under a name something like content_translation). So we don't have any big plans for the translation.module as-is, more like plans to clean up the field translation API and provide a UI for that for all entity types as well as provide a core built-in migration path from the current translation.module. So we should keep translation module intact until that is available, but otherwise not planning any changes there.
Moving to needs review based on that. Any other comments?
Comment #42
clemens.tolboomThe code looks good to me for a RTBC but I'm only scratching D8MI now :(
Comment #43
Gábor HojtsyOk, well, the above patch did not apply anymore either due to other changes in core. Rerolled. No changes.
Comment #44
tstoecklerThis needs an update path I think.
Comment #45
Gábor Hojtsy@tstoeckler: right. Can you or Clemens help with that? It would be superb! Thanks!
Comment #46
clemens.tolboomAdded node_update_8001 ... does the testbot test upgrades too?
Comment #47
clemens.tolboomComment #48
Gábor Hojtsy@clemens: Thanks! The update looks like what we need.
Testbot would not do update testing in itself. There is a good system for testing these updates though. Simpletest has a tests/upgrade/ directory, which does have an empty and a filled database dump (in PHP form) and respective tests. We started adding some language tests to the filled.test, which we can either keep expanding or refactor to more tests extending each other like Drupal 7 did. Anyway, its normal tests which import a dump and then run the test assertions on the updated dump.
Comment #49
Gábor Hojtsy@clemens: I wrote this doc page based on your excellent questions: http://drupal.org/node/1429136. Also introduced easier to diff componentized language test files in #1357918: Missing update for language_default in language langcode update, so I think we should postpone this on that change, so we can introduce the upgrade path test seamlessly. I hope it will be in quick, so we can move on with this.
Again, excellent upgrade path, thanks.
Comment #50
Gábor HojtsyTagging for the content handling leg of D8MI.
Comment #51
Gábor HojtsyOk #1357918: Missing update for language_default in language langcode update landed, so now we can continue adding tests. IMHO the following should do it:
1. Add database dump data for a node type with language support in drupal-7.language.database.php.
2. Add the respective variable with its D7 name there too.
2. Extend the upgrade.language.test to (a) check for the new variable name (b) check the admin page for the content type that the config is still set (c) check the node/add/$type page for the type that it includes a language dropdown.
I think that should be well enough coverage :) With the dump now in its own PHP, it should be easy to diff and submit a patch. @clemens: do you have time to work on this?
Comment #52
Gábor HojtsyNot being worked on by Xano.
Comment #53
Gábor HojtsyNeeds tests.
Comment #54
clemens.tolboom(just some notes to accomplish the generation of test data)
I'm running the following code as per http://drupal.org/node/1429136 script fit to my dev env
Why aren't the scripts located in D7 branch?
Which fails as we need to add a language?
Comment #55
clemens.tolboomThe script ../d8/www/core/modules/simpletest/tests/upgrade/drupal-7.language.database.php is assuming there are no blocks yet. But these are added due to the "Enable all core modules step" some question are:
1. What is our Drupal-7 base line?
- just a site-install?
2. Why should we enable _all_ core modules?
- we should only enable our targeted modules part of our particular test.
3. Adding the 'block' records it the result of enabling the local module so shouldn't we insert a record into the system table too?
I continue with the documentation page now.
Comment #56
Gábor HojtsyLocale module is added to the system table but not installed in the filled db dump (gz). The language dump updates the sytem table entry and adds the schema elements from the module, so in effect installs it. I think it is likely going to be very hard to script data dump diffs for componentized dumps but if we dont componentize it, testing will be hard with intermingled target data. So i think a method with the least pain is to configure a D7 site the way you need and do a dump from that. Then copy parts of that that you need for the test. If idenitfying tose parts is nontrivial, you can make a dump before and after your config and diff those.
Comment #57
clemens.tolboomWhat I tried to accomplish today is a working D7 site to check for how to test the data at hand. I think being able to load the testset into a working D7 is key for people to develop upgrade tests.
When doing
I get
As a logged in user @ http://drupal.d7/node#overlay=admin/config
Then @ http://drupal.d7/node#overlay=admin/modules
where 'Content translation' is not enabled. Do we need that too? (Enabling and clearing cache does not remove the notices)
So I'm stuck here puzzling where these warnings come from.
Following along this issues test (loading the filled and the languages db) I cannot load the filled db as the commands.
fails misarably as drush cannot bootstrap on an empty drupal site.
So stuck again for now.
Comment #58
clemens.tolboomTo make it more clear I have problems with the red arrow. How to load a dumpfile into a pristine D7 install?
Studying
shows how it is done through the simpletest framework on a D8 codebase.
Am I missing something? Guess not as Gabor said somewhere (in ? http://drupal.org/node/1429136 ?) loading a dump file needs some documentation. But we are getting close :p
For today I stop trying to load the dumps and try to add some tests to this issue.
BTW Did you noticed the error in the image? D7 codebase cannot create it's own dumpfile ... which is said :(
Comment #59
clemens.tolboomI only added a new content type 'locale' to drupal-7.language.database.php. Be back on this at the next D8MI sprint @ budapest.
It was trivial to dump but not to extract sensible data out of the dump. I left out i.e. the menu entries added by installing the locale module. But the process we are now getting into place is pretty neat.
Comment #61
clemens.tolboomWeird as we missed(?) the poll.test. I should have ran all tests to find that. But that takes ages :(
Comment #62
Gábor HojtsyThe poll test were extended very recently, therefore the new issues. You can find that out in the git log. We did not miss them before, they were not there.
Comment #63
clemens.tolboom@Gábor Please shed some light on #58 and #59 if possible. Pointers to other issues are welcome too.
What I want to know is how to load my D7 install with test data to work on that.
I'm now checking out #1364798: Impossible to generate meaningful diffs of upgrade db dumps to see what that will bring.
Comment #64
Gábor Hojtsy@clemens: honestly I never tried loading one of those dump files in Drupal 7 (or the aggregates of them for that matter). I agree it would make the process even more complete, however I also believe that Drupal 8 will include so many changes that all aspects of the dump and the functionality it represents will be exercised at some point. I do believe that loading that dump is not a pre-requisite to extending the dumps or writing tests against the updated Drupal instances based on those dumps. At least the changes we make are very self-contained and limited. So I'm afraid I cannot help you with that part.
Comment #65
Gábor HojtsyStill needs work as it is missing tests.
Comment #66
Gábor HojtsyHere are tests. I don't think we need the whole set of new content type and such that you included with your patch. I decided to not test the same setting three ways as documented in #51 but instead do cross-check with a content type that is not multilingual enabled. Since all that we do is to change one option's internal name, all we should test if the option still works after the update. So I've added that option for the existing article content type and a test as to whether languages show up on that (and that languages don't show up on the page content type).
I think this should be enough test coverage.
Would be good to get this in as soon as possible, so we can work on more interesting things like #258785: Provide more flexible settings for initial language on content types on the codesprint (which is dependent on this landing). (Did not change anything outside of the db dump and test, compared to your last patch).
Comment #67
clemens.tolboomThe added tests are so short I hardly noticed them :)
Comment #68
Gábor HojtsyMarking major for itself as well as because other important issues depend on it like #258785: Provide more flexible settings for initial language on content types.
Comment #69
sunSorry for jumping in late. Some minor remarks:
While touching these, it would really make sense to replace those 2s with the explicit TRANSLATION_ENABLED constant for clarity.
That is, because 2 is not a valid value for Language module's original language select element.
Placeholder tokens for attributes should always be escaped; i.e., @languages-url
"Rename node type language variable names."
Specifying #empty_value is sufficient. The default option label for non-required selects is "- None -" already.
Also, by omitting the #empty_option, the label automatically adjusts itself based on whether the select is #required (and thus automatically uses a stronger/clearer "- Select -" if it is)
Comment #70
clemens.tolboomFixed all issues mentioned by @sun.
Comment #71
sunThanks!
Comment #72
Dries CreditAttribution: Dries commentedThis all looks good and I agree that it is the right thing to do. Some nice little clean ups along the way too. Committed to 8.x.
Comment #73
Gábor HojtsyYay, this unblocks #258785: Provide more flexible settings for initial language on content types, thanks!
Comment #74
claudiu.cristea#1074672: Allow language select to be rearranged inside node form is unblocked too. I will reroll the patch there.