Files: 
CommentFileSizeAuthor
#20 feed-langcode-1905870-20.patch935 bytesEric_A
PASSED: [[SimpleTest]]: [MySQL] 50,659 pass(es).
[ View ]
#16 feed-langcode-1905870-16.patch10.83 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 49,828 pass(es).
[ View ]
#16 feed-langcode-1905870-16-interdiff.txt1.77 KBBerdir
#14 feed-langcode-1905870-14.patch10.73 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 49,812 pass(es).
[ View ]
#4 feed-langcode-1905870-4.patch6.37 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 49,471 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Comments

Gábor Hojtsy’s picture

Issue tags:+language-content

What do you imagine would be the requirements here?

Berdir’s picture

I think just being able to specify a language for a feed and that language is then copied to the feed item would be a considerable improvement to what we have now.

Gábor Hojtsy’s picture

Definitely :) I don't think we want to have multilingual properties on feeds or feed items, at least I cannot think of a need for that. Those syncing site content via feeds would use node entities as their target anyway (not with the core modules).

Berdir’s picture

Status:Active» Needs review
Issue tags:+Needs tests
StatusFileSize
new6.37 KB
FAILED: [[SimpleTest]]: [MySQL] 49,471 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Not sure if the tests are going to work and this will need test coverage but manual tests worked fine. Allowed me to set a language and the feed items take over that language.

Status:Needs review» Needs work

The last submitted patch, feed-langcode-1905870-4.patch, failed testing.

Berdir’s picture

Failed with this:

exception 'PDOException' with message 'SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'John' for key 'name'' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Statement.php:58
Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Statement.php(58): PDOStatement->execute(Array)
#1 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php(523): Drupal\Core\Database\Statement->execute(Array, Array)
#2 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Driver/mysql/Insert.php(34): Drupal\Core\Database\Connection->query('INSERT INTO {vi...', Array, Array)
#3 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views/lib/Drupal/views/Tests/ViewUnitTestBase.php(43): Drupal\Core\Database\Driver\mysql\Insert->execute()
#4 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(714): Drupal\views\Tests\ViewUnitTestBase->setUp()
#5 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(404): Drupal\simpletest\TestBase->run()
#6 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('678', 'Drupal\views\Te...')
#7 {main}

Next exception 'Drupal\Core\Database\IntegrityConstraintViolationException' with message 'SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'John' for key 'name': INSERT INTO {views_test_data} (name, age, job, created) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3), (:db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7), (:db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11), (:db_insert_placeholder_12, :db_insert_placeholder_13, :db_insert_placeholder_14, :db_insert_placeholder_15), (:db_insert_placeholder_16, :db_insert_placeholder_17, :db_insert_placeholder_18, :db_insert_placeholder_19); Array
(
    [:db_insert_placeholder_0] => John
    [:db_insert_placeholder_1] => 25
    [:db_insert_placeholder_2] => Singer
    [:db_insert_placeholder_3] => 946684800
    [:db_insert_placeholder_4] => George
    [:db_insert_placeholder_5] => 27
    [:db_insert_placeholder_6] => Singer
    [:db_insert_placeholder_7] => 946771200
    [:db_insert_placeholder_8] => Ringo
    [:db_insert_placeholder_9] => 28
    [:db_insert_placeholder_10] => Drummer
    [:db_insert_placeholder_11] => 946708230
    [:db_insert_placeholder_12] => Paul
    [:db_insert_placeholder_13] => 26
    [:db_insert_placeholder_14] => Songwriter
    [:db_insert_placeholder_15] => 946706400
    [:db_insert_placeholder_16] => Meredith
    [:db_insert_placeholder_17] => 30
    [:db_insert_placeholder_18] => Speaker
    [:db_insert_placeholder_19] => 946708210
)
' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php:551
Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Driver/mysql/Insert.php(34): Drupal\Core\Database\Connection->query('INSERT INTO {vi...', Array, Array)
#1 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views/lib/Drupal/views/Tests/ViewUnitTestBase.php(43): Drupal\Core\Database\Driver\mysql\Insert->execute()
#2 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(714): Drupal\views\Tests\ViewUnitTestBase->setUp()
#3 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(404): Drupal\simpletest\TestBase->run()
#4 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('678', 'Drupal\views\Te...')
#5 {main}FATAL Drupal\views\Tests\Handler\FieldXssTest: test runner returned a non-zero error code (1).

Gábor Hojtsy’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests, -D8MI, -language-content

#4: feed-langcode-1905870-4.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, feed-langcode-1905870-4.patch, failed testing.

Gábor Hojtsy’s picture

Status:Needs work» Needs review
Issue tags:+Needs tests, +D8MI, +language-content

#4: feed-langcode-1905870-4.patch queued for re-testing.

Gábor Hojtsy’s picture

And now OpenID tests failed. So looks like unrelated flukes. Retesting again.

Status:Needs review» Needs work

The last submitted patch, feed-langcode-1905870-4.patch, failed testing.

Berdir’s picture

Hm, that's a strange fail:

No CKEditor plugins found besides the built-in ones. Other CKEditorPluginManagerTest.php 70 Drupal\ckeditor\Tests\CKEditorPluginManagerTest->testEnabledPlugins()

Possibly a new random one?

Anyway, it doesn't matter if this doesn't get green, needs work is the correct state as it needs tests.

Gábor Hojtsy’s picture

Yeah, there are always different fails :) I agree it needs tests anyway. It would in all likeliness pass alone if not for the flukes on testbot.

Berdir’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new10.73 KB
PASSED: [[SimpleTest]]: [MySQL] 49,812 pass(es).
[ View ]

Ok, added basic test coverage.

Gabor, do you think we need something else here? There isn't much to test, the language is currently not visible nor does it provide any additional features. This will only start to get interesting once the views integration makes it in, which should now be updated to include the language fields/filters.

Gábor Hojtsy’s picture

Status:Needs review» Needs work

Yeah, this will get interesting with views. I don't think there were any added features to aggregator, so there is still no global admin page for items is there? That could get a language filter, if there is one such screen now. Code review notes:

+++ b/core/modules/aggregator/aggregator.installundefined
@@ -293,3 +307,31 @@ function aggregrator_update_8000() {
+  db_update('aggregator_item')
+    ->fields(array('langcode' => LANGUAGE_DEFAULT))
+    ->execute();
+}
\ No newline at end of file

Missing newline at end of file.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/FeedLanguageTest.phpundefined
@@ -0,0 +1,79 @@
+    $this->createSampleNodes();

Create sample nodes?!

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/FeedLanguageTest.phpundefined
@@ -0,0 +1,79 @@
+        $this->assertEqual($item->language()->langcode, $feed->language()->langcode);
+      }
+    }
+  }
+}
\ No newline at end of file

Missing newline at the end of file here too.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new1.77 KB
new10.83 KB
PASSED: [[SimpleTest]]: [MySQL] 49,828 pass(es).
[ View ]

Fixed the file endings, add an assertions that there actually were feed items loaded (because it would pass if there would be none otherwise) and moved and commented the sample nodes method call.

Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community

Looks very nice. Thanks for the updates :)

catch’s picture

Title:Make feeds/feed items multilingual» Change notice: Make feeds/feed items multilingual
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active

Looks great. Committed/pushed to 8.x.

Could use a change notice.

Gábor Hojtsy’s picture

Title:Change notice: Make feeds/feed items multilingual» Make feeds/feed items multilingual
Priority:Critical» Normal
Status:Active» Fixed
Eric_A’s picture

Status:Fixed» Needs review
StatusFileSize
new935 bytes
PASSED: [[SimpleTest]]: [MySQL] 50,659 pass(es).
[ View ]
+
+/**
+ * Adds the langcode field to {aggregator_item} and {aggregator_feed}.
+ */
+function aggregator_update_8001() {
+  // Add the field.
+  db_add_field('aggregator_feed', 'langcode', array(
+    'description' => 'The {language}.langcode of this feed.',
+    'type' => 'varchar',
+    'length' => 12,
+    'not null' => TRUE,
+    'default' => '',
+  ));
+  db_add_field('aggregator_item', 'langcode', array(
+    'description' => 'The {language}.langcode of this feed item.',
+    'type' => 'varchar',
+    'length' => 12,
+    'not null' => TRUE,
+    'default' => '',
+  ));
+  // Set langcode to LANGUAGE_DEFAULT.
+  db_update('aggregator_feed')
+    ->fields(array('langcode' => LANGUAGE_DEFAULT))
+    ->execute();
+  db_update('aggregator_item')
+    ->fields(array('langcode' => LANGUAGE_DEFAULT))
+    ->execute();
+}

We have the 'initial' key for use in the specification. Less code, and I'd say more clarity.

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

Oh, didn't know or forgot about that.

Initial does exactly the same thing internally, so this looks good to me.

chx’s picture

lol i didn't know about that either. when did that get in :) ?

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Yay two line patches. :)

Committed and pushed to 8.x. Thanks!

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