Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
8.11 KB

Schema doesn't yet need to be (un)installed in this patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

+  if (TRUE || variable_get('node_type_language_' . $node->type, 0)) {

I guess that's debugging stuff?

Xano’s picture

Status: Needs work » Needs review
FileSize
8.19 KB

Good one. That was what made at least some of the tests fail.

Status: Needs review » Needs work

The last submitted patch failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
20.84 KB

Locally all tests pass, except for "Path alias functionality', which causes a 500 error and all I can find is this crash message.

Status: Needs review » Needs work

The last submitted patch failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
20.88 KB

Hmm, 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.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

A 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.

Xano’s picture

The 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.

Gábor Hojtsy’s picture

Version: 7.x-dev » 8.x-dev

Interesting. 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).

catch’s picture

The 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.

Gábor Hojtsy’s picture

Yes, 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.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Issue tags: +D8MI

Let's make this happen in Drupal 8.

Gábor Hojtsy’s picture

Issue tags: +locale-split
sun’s picture

Title: Decouple Locale from Node » Move node language settings from Locale to Node module
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
14.68 KB

Updated 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).

Gábor Hojtsy’s picture

catch’s picture

Status: Needs review » Needs work
db_query("DELETE FROM {variable} WHERE name LIKE 'node_%'");

Should 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.

Gábor Hojtsy’s picture

Are the variable checks enough for this as opposed to module_exists('locale') - can they get set otherwise?

They 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)?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
14.92 KB

Here 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.

Status: Needs review » Needs work

The last submitted patch, decouple-locale-from-node-24.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
13.98 KB

Instead 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).

Status: Needs review » Needs work

The last submitted patch, decouple-locale-from-node-26.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
14.22 KB

The 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.

Status: Needs review » Needs work
Issue tags: -Framework Initiative, -D8MI, -locale-split

The last submitted patch, decouple-locale-from-node-28.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +Framework Initiative, +D8MI, +locale-split

#28: decouple-locale-from-node-28.patch queued for re-testing.

Gábor Hojtsy’s picture

Ok, 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.

claudiu.cristea’s picture

#1074672: Allow language select to be rearranged inside node form is blocked by this and will be rerolled after committing this to 8.x.

Gábor Hojtsy’s picture

Quick 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.

Status: Needs review » Needs work

The last submitted patch, decouple-locale-from-node-33.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
14.37 KB

Found 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.

Gábor Hojtsy’s picture

Issue tags: -locale-split +sprint

Tagging as current focus.

clemens.tolboom’s picture

Status: Needs review » Needs work

I cannot apply patch #35

git apply ../decouple-locale-from-node-35.patch 
error: patch failed: core/modules/locale/locale.module:1008
error: core/modules/locale/locale.module: patch does not apply
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
14.54 KB

Thanks Clemens. Indeed, the code in question moved to comment.module since the last patch was rolled above. Any other feedback?

clemens.tolboom’s picture

Status: Needs review » Needs work
FileSize
14.54 KB

There are two competing 'workflow' the alter the node_type forms. module_exist versus node_type_form_alter

if (module_exists('language')) {
+++ b/core/modules/node/content_types.inc
@@ -187,6 +187,14 @@ function node_type_form($form, &$form_state, $type = NULL) {
+      '#description' => t('Add a language selection field to the editing form, allowing you to select from one of the <a href="!languages">enabled languages</a>. If disabled, new posts are saved with the default language. Existing content will not be affected by changing this option.', array('!languages' => url('admin/international/language'))),
+    );

This description is competing with translation_form_node_type_form_alter

+++ b/core/modules/translation/translation.module
@@ -111,9 +111,10 @@ function translation_permission() {
+  $form['workflow']['node_type_language']['#description'] = t('Add a language selection field to the editing form, allowing you to select from one of the <a href="!languages">enabled languages</a>. You can also turn on translation for this content type, which lets you have content translated to any of the installed languages. If disabled, new posts are saved with the default language. Existing content will not be affected by changing this option.', array('!languages' => url('admin/international/language')));
 }

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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

@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(?):

You can also turn on translation for this content type, which lets you have content translated to any of the installed languages.

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?

Status: Needs review » Needs work

The last submitted patch, decouple-locale-from-node-39.patch, failed testing.

clemens.tolboom’s picture

The code looks good to me for a RTBC but I'm only scratching D8MI now :(

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
14.6 KB

Ok, well, the above patch did not apply anymore either due to other changes in core. Rerolled. No changes.

tstoeckler’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/comment.module
@@ -1841,7 +1841,7 @@ function comment_form($form, &$form_state, $comment) {
-  if (($comment_langcode == LANGUAGE_NONE) && variable_get('language_content_type_' . $node->type, 0)) {
+  if (($comment_langcode == LANGUAGE_NONE) && variable_get('node_type_language_' . $node->type, 0)) {

This needs an update path I think.

Gábor Hojtsy’s picture

@tstoeckler: right. Can you or Clemens help with that? It would be superb! Thanks!

clemens.tolboom’s picture

Added node_update_8001 ... does the testbot test upgrades too?

clemens.tolboom’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

@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.

Gábor Hojtsy’s picture

Status: Needs review » Postponed

@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.

Gábor Hojtsy’s picture

Issue tags: +language-content

Tagging for the content handling leg of D8MI.

Gábor Hojtsy’s picture

Status: Postponed » Needs work

Ok #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?

Gábor Hojtsy’s picture

Assigned: Xano » Unassigned

Not being worked on by Xano.

Gábor Hojtsy’s picture

Issue tags: +Needs tests

Needs tests.

clemens.tolboom’s picture

(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

drush sql-drop --yes
drush site-install --yes standard

# Is malfunctioning as it enabled non-core modules from my sites/default/modules
drush pml --core --status="not installed" --pipe | xargs drush en --yes

# Why aren't the scripts in the d7 git checkout?

# Dumping for comparing purposes only
php ../../d8/www/core/scripts/dump-database-d7.sh > ../../d8/www/core/modules/simpletest/tests/upgrade/drupal-7.bare.database.php

php ../../d8/www/core/scripts/generate-d7-content.sh

# This fails due to ?
drush php-script ../../d8/www/core/modules/simpletest/tests/upgrade/drupal-7.language.database.php

Why aren't the scripts located in D7 branch?
Which fails as we need to add a language?

clemens.tolboom’s picture

The 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.

Gábor Hojtsy’s picture

Locale 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.

clemens.tolboom’s picture

What 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

git reset --hard
git pull
git apply 
drush site-install --yes standard --account-name=admin --account-pass=drupal
drush php-script ../../d8/www/core/modules/simpletest/tests/upgrade/drupal-7.language.database.php 

I get

Notice: Undefined property: stdClass::$language in locale_language_url_fallback() (line 311 of /Users/clemens/Sites/drupal/d7/www/includes/locale.inc).

As a logged in user @ http://drupal.d7/node#overlay=admin/config

Notice: Undefined property: stdClass::$language in drupal_lookup_path() (line 77 of /Users/clemens/Sites/drupal/d7/www/includes/path.inc).

Then @ http://drupal.d7/node#overlay=admin/modules

Notice: Undefined property: stdClass::$language in drupal_lookup_path() (line 77 of /Users/clemens/Sites/drupal/d7/www/includes/path.inc).
Notice: Undefined property: stdClass::$language in locale_language_url_fallback() (line 311 of /Users/clemens/Sites/drupal/d7/www/includes/locale.inc).

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.

drush sql-drop --yes
drush php-script ../../d8/www/core/modules/simpletest/tests/upgrade/drupal-7.filled.database.php

fails misarably as drush cannot bootstrap on an empty drupal site.

So stuck again for now.

clemens.tolboom’s picture

FileSize
6.22 KB

To make it more clear I have problems with the red arrow. How to load a dumpfile into a pristine D7 install?

upgrade-workflow.png

Studying

abstract class UpgradePathTestCase extends DrupalWebTestCase {
...
  protected function setUp() {
...
    // Load the database from the portable PHP dump.
    // The files can be gzipped.
    foreach ($this->databaseDumpFiles as $file) {
      if (substr($file, -3) == '.gz') {
        $file = "compress.zlib://$file";
      }
      require $file;
    }

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 :(

clemens.tolboom’s picture

Status: Needs work » Needs review
FileSize
20.46 KB

I 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.

Status: Needs review » Needs work

The last submitted patch, decouple-locale-from-node-540294-59.patch, failed testing.

clemens.tolboom’s picture

Status: Needs work » Needs review
FileSize
21.15 KB

Weird as we missed(?) the poll.test. I should have ran all tests to find that. But that takes ages :(

Gábor Hojtsy’s picture

The 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.

clemens.tolboom’s picture

@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.

Gábor Hojtsy’s picture

@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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Still needs work as it is missing tests.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
17.47 KB

Here 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).

clemens.tolboom’s picture

Status: Needs review » Reviewed & tested by the community

The added tests are so short I hardly noticed them :)

Gábor Hojtsy’s picture

Priority: Normal » Major

Marking 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.

sun’s picture

Status: Reviewed & tested by the community » Needs review

Sorry for jumping in late. Some minor remarks:

+++ b/core/modules/field/modules/text/text.test
@@ -434,7 +434,7 @@ class TextTranslationTestCase extends DrupalWebTestCase {
+    $edit = array('node_type_language' => 2);

+++ b/core/modules/path/path.test
@@ -260,7 +260,7 @@ class PathLanguageTestCase extends DrupalWebTestCase {
+    variable_set('node_type_language_page', 2);

+++ b/core/modules/poll/poll.test
@@ -843,7 +843,7 @@ class PollTranslateTestCase extends PollTestCase {
+    $edit['node_type_language'] = 2;

+++ b/core/modules/translation/translation.module
@@ -111,9 +111,10 @@ function translation_permission() {
+  $form['workflow']['node_type_language']['#options'] = array(t('Disabled'), t('Enabled'), TRANSLATION_ENABLED => t('Enabled, with translation'));

+++ b/core/modules/translation/translation.test
@@ -38,7 +38,7 @@ class TranslationTestCase extends DrupalWebTestCase {
+    $edit['node_type_language'] = 2;

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.

+++ b/core/modules/node/content_types.inc
@@ -187,6 +187,14 @@ function node_type_form($form, &$form_state, $type = NULL) {
+      '#description' => t('Add a language selection field to the editing form, allowing you to select from one of the <a href="!languages">enabled languages</a>. If disabled, new posts are saved with the default language. Existing content will not be affected by changing this option.', array('!languages' => url('admin/config/regional/language'))),

Placeholder tokens for attributes should always be escaped; i.e., @languages-url

+++ b/core/modules/node/node.install
@@ -525,6 +526,22 @@ function node_update_8000() {
+ * Convert existing node type language settings to its new name.

"Rename node type language variable names."

+++ b/core/modules/node/node.pages.inc
@@ -181,6 +181,30 @@ function node_form($form, &$form_state, $node) {
+    $form['language'] = array(
+      '#type' => 'select',
...
+      '#empty_value' => LANGUAGE_NONE,
+      '#empty_option' => t('- None -'),

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)

clemens.tolboom’s picture

Fixed all issues mentioned by @sun.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

This 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.

Gábor Hojtsy’s picture

claudiu.cristea’s picture

#1074672: Allow language select to be rearranged inside node form is unblocked too. I will reroll the patch there.

Status: Fixed » Closed (fixed)
Issue tags: -Framework Initiative, -D8MI, -language-content

Automatically closed -- issue fixed for 2 weeks with no activity.