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

Support from Acquia helps fund testing for Drupal Acquia logo

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
FileSize
875 bytes

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
FileSize
11.4 KB
12.02 KB

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

FileSize
1.11 KB
12.43 KB

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
FileSize
10.97 KB

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
FileSize
577 bytes
11.31 KB

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
FileSize
5.4 KB
16.7 KB

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
FileSize
10.78 KB
26.83 KB

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
FileSize
19.78 KB
36.36 KB

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
FileSize
3.6 KB
37.13 KB

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
FileSize
2.27 KB
39.87 KB

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
FileSize
2.35 KB
41.4 KB

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
FileSize
3.99 KB
44.46 KB

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

     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

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

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

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
FileSize
1.44 KB
46.85 KB

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
FileSize
46.13 KB

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
FileSize
897 bytes
46.77 KB

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

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
FileSize
46.84 KB

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.