Based on #1498634: [meta] Define revision/translation SQL schema for core entities (no patch) there should be a new schema.
Comment should add data table {comment_field_data} it's connected to {comment} directly.

For further details read Added multilingual support to the standard entity schema

Files: 
CommentFileSizeAuthor
#55 1498662-comment-subject-55.patch46.84 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,335 pass(es).
[ View ]

Comments

dawehner’s picture

Issue tags:+D8MI, +language-content

Add tags

Gábor Hojtsy’s picture

Title:Define comment translation schema» Refactor comment entity properties to multilingual
Status:Active» Postponed
Gábor Hojtsy’s picture

Status:Postponed» Active

Comments are converted to Entity NG, so this should be fully possible now.

Gábor Hojtsy’s picture

Tagging for D8MI.

andypost’s picture

andypost’s picture

klonos’s picture

andypost’s picture

Should be done before beta

andypost’s picture

Assigned:Unassigned» andypost
Status:Active» Needs review
StatusFileSize
new875 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Initial patch - just subject needs translation.
There's a lot conversions happen with comment queries in #2068325: [META] Convert entity SQL queries to the Entity Query API

Status:Needs review» Needs work

The last submitted patch, 9: 1498662-comment-subject-9.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
Related issues:+#1740492: Implement a default entity views data handler
StatusFileSize
new11.4 KB
new12.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,333 pass(es), 217 fail(s), and 1,451 exception(s).
[ View ]

hm, not so much left direct queries...

Patch:
- fixed schema, finally get rid of "array-index" - close to postgres ;)
- fixed direct queries, should be combined with other entity query patches
- a hack to remove join on user table (author_name), probably needs @berdir

todo:
1) profile new queries and tune indexes
2) views integration, probably could be solved with #1740492: Implement a default entity views data handler
3) search module integration

andypost’s picture

Status:Needs review» Needs work

The last submitted patch, 11: 1498662-comment-subject-11.patch, failed testing.

andypost’s picture

StatusFileSize
new1.11 KB
new12.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,643 pass(es), 156 fail(s), and 42 exception(s).
[ View ]

Filed #2292815: Remove join comments on users table
the patch to test removal, also subject should use formatter and widget, will file new issue

andypost’s picture

Status:Needs work» Needs review
andypost’s picture

Status:Needs review» Needs work

The last submitted patch, 14: 1498662-comment-subject-14.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new10.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,411 pass(es), 405 fail(s), and 40 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 18: 1498662-comment-subject-18.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new577 bytes
new11.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,513 pass(es), 103 fail(s), and 36 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 20: 1498662-comment-subject-20.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new5.4 KB
new16.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,238 pass(es), 75 fail(s), and 15 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 22: 1498662-comment-subject-22.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new10.78 KB
new26.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,628 pass(es), 65 fail(s), and 38 exception(s).
[ View ]

Start to fix views integration

Status:Needs review» Needs work

The last submitted patch, 24: 1498662-comment-subject-24.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new19.78 KB
new36.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,771 pass(es), 24 fail(s), and 20 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 26: 1498662-comment-subject-26.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new3.6 KB
new37.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,831 pass(es), 3 fail(s), and 15 exception(s).
[ View ]

fix forum and tracker

Status:Needs review» Needs work

The last submitted patch, 28: 1498662-comment-subject-28.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new2.27 KB
new39.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,815 pass(es), 2 fail(s), and 15 exception(s).
[ View ]

Extended and fixed Drupal\content_translation\Tests\ContentTranslationSettingsTest

Now subject field is translatable by default so content translation always set it checked.

Status:Needs review» Needs work

The last submitted patch, 30: 1498662-comment-subject-30.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new2.35 KB
new41.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,844 pass(es), 2 fail(s), and 14 exception(s).
[ View ]

Fix Drupal\entity_reference\Tests\EntityReferenceSelectionAccessTest

Status:Needs review» Needs work

The last submitted patch, 32: 1498662-comment-subject-32.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new3.99 KB
new44.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,859 pass(es).
[ View ]

Fix default views

andypost’s picture

finally green, waiting reviews

larowlan’s picture

Great work only some minor observations.

  1. +++ b/core/modules/comment/comment.views.inc
    @@ -637,12 +645,12 @@ function comment_views_data_alter(&$data) {
    +      if ($entity_type->getDataTable()) {
    +        // Multilingual properties lives in data_table.
             $table = $entity_type->getDataTable();

    Could this be rewritten as

    <?php
     
    if (!($table = $entity_type->getDataTable())) {
     
    $table = $entity_type->getBaseTable();
    }
    ?>
  2. +++ b/core/modules/comment/src/Entity/Comment.php
    @@ -228,6 +229,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +      ->setTranslatable(TRUE)

    Is this the only thing that becomes translatable?

  3. +++ b/core/modules/comment/src/Plugin/entity_reference/selection/CommentSelection.php
    @@ -44,31 +44,20 @@ public function buildEntityQuery($match = NULL, $match_operator = 'CONTAINS') {
    +      // When no conditions used the comment data table should be joined

    This comment could use a tidy up - suggested
    'If no conditions join against the comment data table, it should be joined manually to allow node access processing'.

  4. +++ b/core/modules/comment/src/Plugin/entity_reference/selection/CommentSelection.php
    @@ -44,31 +44,20 @@ public function buildEntityQuery($match = NULL, $match_operator = 'CONTAINS') {
    -    // Alas, the comment entity exposes a bundle, but doesn't have a bundle
    -    // column in the database. We have to alter the query ourselves to go fetch
    -    // the bundle.
    -    $conditions = &$query->conditions();
    -    foreach ($conditions as $key => &$condition) {
    -      if ($key !== '#conjunction' && is_string($condition['field']) && $condition['field'] === 'node_type') {
    -        $condition['field'] = $node_alias . '.type';
    -        foreach ($condition['value'] as &$value) {
    -          if (substr($value, 0, 13) == 'comment_node_') {
    -            $value = substr($value, 13);
    -          }
    -        }
    -        break;
    -      }
    -    }
    -

    Whoops, this should have been gone by now, seeing as though we have had comment-bundles for eons.

  5. +++ b/core/modules/content_translation/src/Tests/ContentTranslationSettingsTest.php
    @@ -94,13 +96,24 @@ function testSettingsUI() {
    +      // Override both comment subject fields to none translatable.

    %s/none/non ?

Gábor Hojtsy’s picture

Great job @andypost, @larowlan, let's fix the remaining items and get this in :)

amitgoyal’s picture

StatusFileSize
new44.04 KB

Patch in #34 no longer applies. Please review revised patch along with fixes in #36.

I am not sure about #36-2.

Status:Needs review» Needs work

The last submitted patch, 38: 1498662-comment-subject-38.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new5.49 KB
new46.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,036 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Re: #34
1) changed
2) Looking on Node seems others needs too - status, created, changed, uid (and parts) also suppose "thread" ML, this will allow to have thread per entity translation.
3) fixed
4) yep, magically this does not affect us before...
5) Suppose Untranslatability is proper term.

andypost’s picture

Suppose thrading per translation needs more tests and would be fixed after #2156089: Remove comment_get_thread() in favour of method on CommentStorage

Status:Needs review» Needs work

The last submitted patch, 40: 1498662-comment-subject-37.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new1.44 KB
new46.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1498662-comment-subject-43.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fix broken test

Most of properties are multilingual, so only "thread" needs to check that comments are attached to translation

Gábor Hojtsy’s picture

@larowlan: looks good? :)

larowlan’s picture

Status:Needs review» Reviewed & tested by the community

Looks good, great job @andypost

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 43: 1498662-comment-subject-43.patch, failed testing.

larowlan’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new46.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,402 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Straight re-roll

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 47: comment-i18n-1498662.47.patch, failed testing.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new897 bytes
new46.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,223 pass(es).
[ View ]

Fixed re-roll

larowlan’s picture

Status:Needs review» Reviewed & tested by the community

just a re-roll, back to rtbc if green

andypost’s picture

StatusFileSize
new2.51 KB
new46.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,581 pass(es).
[ View ]

Discussed with Gabor in #d8mi meeting - thread should not be translatable, parent (thread) can't be changed while translating

Also fixes merge conflict

larowlan’s picture

Still RTBC
As this makes data changes, really need this in before beta

plach’s picture

I have to admit I don't understand every single line of this patch, but overall it looks good to me :)
Just a couple of remarks:

  1. +++ b/core/modules/comment/src/Entity/Comment.php
    @@ -254,19 +260,23 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +      ->setTranslatable(TRUE)

    Do we really want to allow people to store a different host per language? ;)

  2. +++ b/core/modules/comment/src/Plugin/views/field/NodeNewComments.php
    @@ -118,7 +118,7 @@ public function preRender(&$values) {
             LEFT JOIN {history} h ON h.nid = n.nid AND h.uid = :h_uid WHERE n.nid IN (:nids)

    +++ b/core/modules/forum/src/ForumIndexStorage.php
    @@ -96,14 +96,14 @@ public function update(NodeInterface $node) {
    +    $count = $this->database->query("SELECT COUNT(cid) FROM {comment_field_data} c INNER JOIN {forum_index} i ON c.entity_id = i.nid WHERE c.entity_id = :nid AND c.field_name = 'comment_forum' AND c.entity_type = 'node' AND c.status = :status AND c.default_langcode = 1", array(
    ...
    +      $last_reply = $this->database->queryRange("SELECT cid, name, created, uid FROM {comment_field_data} WHERE entity_id = :nid AND field_name = 'comment_forum' AND entity_type = 'node' AND status = :status AND default_langcode = 1 ORDER BY cid DESC", 0, 1, array(

    +++ b/core/modules/tracker/tracker.module
    @@ -279,7 +280,7 @@ function _tracker_calculate_changed($nid) {
    +  $latest_comment = db_query_range("SELECT cid, changed FROM {comment_field_data} WHERE entity_type = 'node' AND entity_id = :nid AND status = :status AND default_langcode = 1 ORDER BY changed DESC", 0, 1, array(

    @@ -312,7 +313,7 @@ function _tracker_remove($nid, $uid = NULL, $changed = NULL) {
    +      $keep_subscription = db_query_range("SELECT COUNT(*) FROM {comment_field_data} WHERE entity_type = 'node' AND entity_id = :nid AND uid = :uid AND status = :status AND default_langcode = 1", 0, 1, array(

    In these cases hard-coding the check against the default language won't always work as we might have different statuses per-language. We should open a follow-up to review these queries with a multilingual forum.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs change record

Pretty sure that a change record is required here.

andypost’s picture

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs change record
StatusFileSize
new46.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,335 pass(es).
[ View ]

CR added https://www.drupal.org/node/2301459 and re-roll after #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)

Filed follow-up #2301465: Make comment threading per entity translation to address #53.2

@plach #53.1 yes, hostname is a part of author information so should be able to store translator's data

Also https://www.drupal.org/node/1722906 needs to link the issue

andypost’s picture

Issue summary:View changes

fixed summary

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 837d726 and pushed to 8.x. Thanks!

  • alexpott committed 837d726 on 8.x
    Issue #1498662 by andypost, larowlan | dawehner: Refactor comment entity...
Gábor Hojtsy’s picture

Issue tags:-sprint

Woot, amazing!

andypost’s picture

Assigned:andypost» Unassigned

Status:Fixed» Closed (fixed)

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