CommentFileSizeAuthor
#386 interdiff.txt6.24 KBandypost
#386 sandbox-merge-386.patch419.06 KBandypost
#383 interdiff_type.txt3.8 KBandypost
#383 interdiff.txt32.02 KBandypost
#383 sandbox-merge-383.patch419.35 KBandypost
#381 comment-field.reroll.27.patch420.82 KBlarowlan
#379 interdiff_new.txt19.58 KBandypost
#376 interdiff.txt1.19 KBandypost
#376 sandbox-merge-375.patch407.78 KBandypost
#373 interdiff.txt9.32 KBandypost
#373 sandbox-merge-373.patch407.69 KBandypost
#371 interdiff_merge.txt4.02 KBandypost
#371 interdiff.txt3.15 KBandypost
#371 sandbox-merge-371.patch408.6 KBandypost
#367 interdiff.txt6.84 KBandypost
#367 sandbox-merge-367.patch407.3 KBandypost
#365 sandbox-merge-365.patch407.57 KBandypost
#347 decouple-comment-node-731724.helper.347.interdiff.txt711 byteslarowlan
#347 decouple-comment-node-731724.helper.347.patch409.9 KBlarowlan
#345 decouple-comment-node-731724.helper.345.interdiff.txt5.02 KBlarowlan
#345 decouple-comment-node-731724.helper.345.patch409.89 KBlarowlan
#342 decouple-comment-node-731724.helper.342.interdiff.txt2.54 KBlarowlan
#342 decouple-comment-node-731724.helper.342.patch408.95 KBlarowlan
#340 decouple-comment-node-731724.helper.340.patch408.73 KBlarowlan
#339 decouple-comment-node-731724.helper.339.interdiff.txt1.47 KBlarowlan
#339 decouple-comment-node-731724.helper.339.patch408.92 KBlarowlan
#333 decouple-comment-node-731724.helper.333.interdiff.txt871 byteslarowlan
#333 decouple-comment-node-731724.helper.333.patch408.46 KBlarowlan
#331 decouple-comment-node-731724.helper.331.interdiff.txt1.27 KBlarowlan
#331 decouple-comment-node-731724.helper.331.patch408.47 KBlarowlan
#329 decouple-comment-node-731724.332.interdiff.txt4.34 KBlarowlan
#329 decouple-comment-node-731724.332.patch408.31 KBlarowlan
#327 decouple-comment-node-731724.helper.330.interdiff.txt3.55 KBlarowlan
#327 decouple-comment-node-731724.helper.330.patch407.37 KBlarowlan
#325 decouple-comment-node-731724.helper.325.interdiff.txt791 byteslarowlan
#325 decouple-comment-node-731724.helper.325.patch407.26 KBlarowlan
#321 1907960-321.patch807.07 KBlinclark
#321 interdiff.txt1.58 KBlinclark
#317 interdiff.txt3 KBandypost
#317 sandbox-merge-317.patch406.8 KBandypost
#316 interdiff.txt1.84 KBandypost
#316 sandbox-merge-316.patch404.25 KBandypost
#314 sandbox-merge-314.patch404.11 KBandypost
#312 addition.txt3.91 KBandypost
#312 sandbox-merge-312.patch405.77 KBandypost
#310 interdiff.txt3.91 KBandypost
#310 sandbox-merge-310.patch397.78 KBandypost
#308 interdiff.txt6.06 KBandypost
#308 sandbox-merge-308.patch396.82 KBandypost
#306 interdiff.txt6.88 KBandypost
#306 sandbox-merge-306.patch397.22 KBandypost
#304 sandbox-merge-304.patch389.19 KBandypost
#299 interdiff.txt3.42 KBandypost
#299 sandbox-merge-298.patch385.98 KBandypost
#297 interdiff.txt5.03 KBandypost
#297 sandbox-merge-296.patch389.87 KBandypost
#295 sandbox-merge-294.patch388.98 KBandypost
#292 sandbox-merge-292.patch388.89 KBandypost
#290 interdiff.txt3.97 KBandypost
#290 sandbox-merge-290.patch388.38 KBandypost
#287 decouple-comment-node-731724.helper.287.interdiff.txt3.39 KBlarowlan
#287 decouple-comment-node-731724.helper.287.patch390.11 KBlarowlan
#285 decouple-comment-node-731724.helper.285.interdiff.txt13.28 KBlarowlan
#285 decouple-comment-node-731724.helper.285.patch389.83 KBlarowlan
#283 sandbox-merge-283.patch387.53 KBandypost
#262 interdiff.txt2.77 KBandypost
#262 interdiff2.txt832 bytesandypost
#262 sandbox-merge-263.patch390.07 KBandypost
#260 interdiff.txt616 bytesandypost
#260 sandbox-merge-260.patch387.72 KBandypost
#257 decouple-comment-node-731724.helper.257.interdiff.txt1.04 KBlarowlan
#257 decouple-comment-node-731724.257.patch389.75 KBlarowlan
#255 decouple-comment-node-731724.helper.255.patch388.7 KBlarowlan
#255 decouple-comment-node-731724.helper.255.interdiff.txt1.3 KBlarowlan
#253 decouple-comment-node-731724.helper.253.patch387.66 KBlarowlan
#250 interdiff.txt679 bytesandypost
#250 sandbox-merge-247.patch385.74 KBandypost
#246 interdiff.txt8.13 KBandypost
#246 sandbox-merge-245_.patch386.02 KBandypost
#244 decouple-comment-node-731724.helper.244.patch387.8 KBlarowlan
#239 decouple-comment-node-731724.helper.328.interdiff.txt3.21 KBlarowlan
#239 decouple-comment-node-731724.238.patch388.78 KBlarowlan
#236 decouple-comment-node-731724.helper.236.patch386.58 KBlarowlan
#231 interdiff.txt2.26 KBandypost
#231 sandbox-merge-231.patch386.27 KBandypost
#230 sandbox-merge-230.patch386.2 KBandypost
#229 sandbox-merge-229.patch385.37 KBandypost
#227 sandbox-merge-227.patch385 KBandypost
#225 interdiff.txt2.39 KBandypost
#225 sandbox-merge-225.patch385.34 KBandypost
#224 interdiff.txt594 bytesandypost
#224 sandbox-merge-224.patch385.41 KBandypost
#223 interdiff.txt403.12 KBandypost
#223 sandbox-merge-223.patch385.3 KBandypost
#221 interdiff.txt4.23 KBandypost
#221 sandbox-merge-221.patch385.25 KBandypost
#220 sandbox-merge-220.patch385.76 KBandypost
#219 sandbox-merge-218.patch389.19 KBandypost
#218 sandbox-merge-217.patch389.13 KBandypost
#215 sandbox-merge-215.patch389.13 KBandypost
#211 interdiff.txt1018 bytesandypost
#211 sandbox-merge-210.patch389.67 KBandypost
#209 comment-debug.txt2.55 KBandypost
#205 sandbox-merge-205.patch389.29 KBandypost
#203 interdiff.txt6.67 KBandypost
#203 sandbox-merge-203.patch389.57 KBandypost
#202 interdiff.txt887 bytesandypost
#202 sandbox-merge-202.patch394.33 KBandypost
#200 decouple-comment-node-731724.helper.200.patch395.51 KBlarowlan
#195 interdiff.txt11.61 KBandypost
#195 sandbox-merge-195.patch391.28 KBandypost
#193 sandbox-merge-193.patch391.01 KBandypost
#190 interdiff.txt1.02 KBandypost
#190 sandbox-merge-190.patch390.38 KBandypost
#189 sandbox-merge-189.patch389.36 KBandypost
#189 comment-wrapper.twig_.txt1.04 KBandypost
#188 interdiff.txt935 bytesandypost
#187 decouple-comment-node-731724.helper.187.interdiff.txt7.95 KBlarowlan
#187 decouple-comment-node-731724.helper.187.patch393.28 KBlarowlan
#186 interdiff.txt2.32 KBandypost
#186 sandbox-merge-186.patch388.38 KBandypost
#184 decouple-comment-node-731724.helper.184.interdiff.txt3.33 KBlarowlan
#180 interdiff.txt1.3 KBandypost
#180 sandbox-merge-180.patch386.13 KBandypost
#178 interdiff.txt11.91 KBandypost
#178 sandbox-merge-178.patch385.41 KBandypost
#176 interdiff.txt5.18 KBandypost
#176 sandbox-merge-176.patch383.69 KBandypost
#175 interdiff.txt2.37 KBandypost
#175 sandbox-merge-175.patch381.98 KBandypost
#173 interdiff.txt4.28 KBandypost
#173 sandbox-merge-173.patch381.95 KBandypost
#171 sandbox-merge-171.patch381.87 KBandypost
#170 decouple-comment-node-731724.helper.171.interdiff.txt18.4 KBlarowlan
#170 decouple-comment-node-731724.helper.171.patch383.87 KBlarowlan
#169 decouple-comment-node-731724.helper.169.interdiff.txt823 byteslarowlan
#169 decouple-comment-node-731724.helper.169.patch377.93 KBlarowlan
#165 decouple-comment-node-731724.helper.165.patch408.34 KBlarowlan
#165 xhprof-output.zip17.87 KBlarowlan
#164 interdiff.txt5.63 KBandypost
#164 decouple-comment-node-731724.164.patch385.12 KBandypost
#164 decouple-comment-node-731724.164_.patch376.3 KBandypost
#162 decouple-comment-node-731724.helper.162.interdiff.txt1.16 KBlarowlan
#162 decouple-comment-node-731724.162.patch377.35 KBlarowlan
#161 decouple-comment-node-731724.helper.161.interdiff.txt712 byteslarowlan
#161 decouple-comment-node-731724.161.patch377.65 KBlarowlan
#159 decouple-comment-node-731724.helper.159.interdiff.txt2.69 KBlarowlan
#159 decouple-comment-node-731724.helper.159.patch378.6 KBlarowlan
#156 decouple-comment-node-731724.helper.156.patch377.51 KBlarowlan
#154 comment-updates.txt2.53 KBandypost
#154 comment-def.txt2.42 KBandypost
#152 sandbox-merge-151.patch378.13 KBandypost
#150 sandbox-merge-150.patch386.9 KBandypost
#150 interdiff.txt982 bytesandypost
#148 interdiff.txt1.59 KBandypost
#148 sandbox-merge-148.patch377.84 KBandypost
#146 sandbox-merge-146.patch376.81 KBandypost
#144 interdiff.txt1.93 KBandypost
#144 sandbox-merge-144.patch374.12 KBandypost
#142 sandbox-merge-142.patch373.09 KBandypost
#140 decouple-comment-node-731724.helper.140.interdiff.txt2.58 KBlarowlan
#140 decouple-comment-node-731724.helper.140.patch374.32 KBlarowlan
#138 sandbox-merge-138.patch372.63 KBandypost
#135 sandbox-merge-M25-135.patch373.15 KBandypost
#134 drupal-1907960-134.patch376.09 KBdawehner
#133 interdiff.txt4.96 KBandypost
#133 sandbox-133.patch381.31 KBandypost
#130 731724-upgrade-path.patch389.06 KBandypost
#130 sandbox-merge-130.patch381.12 KBandypost
#127 interdiff.txt1.64 KBandypost
#127 sandbox-merge-127.patch381.14 KBandypost
#127 731724-upgrade-path.patch389.08 KBandypost
#125 sandbox-merge-125.patch379.96 KBandypost
#123 upgrade-path.txt13.95 KBandypost
#123 731724-upgrade-path.patch387.9 KBandypost
#122 decouple-comment-node-731724.helper.122.patch372.61 KBlarowlan
#121 decouple-comment-node-731724.helper.121.interdiff.txt2.31 KBlarowlan
#121 decouple-comment-node-731724.helper.121.patch373.45 KBlarowlan
#116 decouple-comment-node-731724.helper.116.interdiff.txt604 byteslarowlan
#116 decouple-comment-node-731724.helper.116.patch373.81 KBlarowlan
#115 decouple-comment-node-731724.helper.115.interdiff.txt3.16 KBlarowlan
#115 decouple-comment-node-731724.helper.115.patch373.81 KBlarowlan
#114 decouple-comment-node-731724.114.interdiff.txt6.21 KBlarowlan
#114 decouple-comment-node-731724.114.patch373.7 KBlarowlan
#112 comment-views.txt3.96 KBandypost
#112 comment-views.patch380.87 KBandypost
#110 sandbox-merge-110.patch379 KBandypost
#108 sandbox-merge-108.patch375.44 KBandypost
#106 sandbox-merge-106.patch375.51 KBandypost
#104 sandbox-merge-104.patch375.67 KBandypost
#102 sandbox-merge-102.patch366.93 KBandypost
#98 interdiff-98.txt1.88 KBandypost
#98 sandbox-98.patch375.93 KBandypost
#95 decouple-comment-node-731724.385.interdiff.txt3.02 KBlarowlan
#95 decouple-comment-node-731724.385.patch368.55 KBlarowlan
#93 sandbox.merge_.patch375.72 KBandypost
#91 formatters6.patch387.76 KBandypost
#89 formatters6.patch395.53 KBandypost
#88 comment-minimize.txt17.98 KBandypost
#88 comment-minimize.patch374.95 KBandypost
#87 rename-nested-interdiff.txt30.29 KBandypost
#87 rename-nested-interdiff.patch382.28 KBandypost
#85 formatters5.txt13.39 KBandypost
#85 formatters5.patch389.83 KBandypost
#83 formatters4.txt8.91 KBandypost
#83 formatters4.patch388.63 KBandypost
#81 formatters3.txt578 bytesandypost
#81 formatters3.patch388.59 KBandypost
#79 formatters2.txt3.78 KBandypost
#79 formatters2.patch388.57 KBandypost
#77 formatters.txt42.43 KBandypost
#77 formatters.patch388.46 KBandypost
#76 cleanup_default_value.patch6.74 KBandypost
#76 sandbox+default.patch375.11 KBandypost
#73 sandbox-73.txt1.79 KBandypost
#73 sandbox-73.patch366.26 KBandypost
#71 sandbox-70pre.txt3.59 KBandypost
#71 sandbox-70pre.patch374.75 KBandypost
#68 sandbox-68.txt1.66 KBandypost
#68 sandbox-68.patch371.53 KBandypost
#66 sandbox-66.txt1.59 KBandypost
#66 sandbox-66.patch373.1 KBandypost
#64 sandbox-64.txt634 bytesandypost
#64 sandbox-64.patch372.13 KBandypost
#63 sandbox-63.patch372.48 KBandypost
#58 sandbox-58.patch371.51 KBandypost
#56 sandbox-56.patch374.97 KBandypost
#54 sandbox-54.txt7.57 KBandypost
#54 sandbox-54.patch375.69 KBandypost
#52 sandbox.patch360.03 KBandypost
#50 731724-interdiff-339.txt2.35 KBandypost
#50 731724-comment-decouple-339m.patch360.03 KBandypost
#50 731724-comment-decouple-339.patch369.13 KBandypost
#49 sandbox.patch369.13 KBandypost
#47 731724-comment-decouple-338.patch360.03 KBandypost
#46 46.interdif.txt4.82 KBandypost
#46 46.patch362.84 KBandypost
#42 interdiff.txt35 KBandypost
#42 44.patch367.74 KBandypost
#41 41-field-interdiff.txt30.57 KBandypost
#41 41.patch367.85 KBandypost
#39 39.interdiff.txt1.17 KBandypost
#39 39.patch371.9 KBandypost
#38 upgrade.patch.txt13.44 KBandypost
#36 731724-comment-decouple-327.interdiff.txt961 bytesandypost
#36 731724-comment-decouple-327.patch371.71 KBandypost
#34 731724-comment-decouple-326.patch370.04 KBdixon_
#33 sandbox-33.patch371.73 KBandypost
#31 comment-31.patch368.89 KBandypost
#29 interdiff-29.txt2.56 KBandypost
#29 comment-upgrade-config-29.patch369.08 KBandypost
#27 interdiff-27.txt1.86 KBandypost
#27 comment-upgrade-config.patch368.89 KBandypost
#25 sandbox.patch368.44 KBandypost
#24 sandbox-upgrade-plugins.patch368.44 KBandypost
#23 db.interdiff.txt2.46 KBandypost
#23 23.patch389.53 KBandypost
#23 comment-formatter.patch392.47 KBandypost
#21 sandbox-staged.interdiff.txt32.54 KBandypost
#21 20.patch368.44 KBandypost
#20 20.patch378.92 KBandypost
#19 comment-dd4ca06.patch365.5 KBandypost
#16 16-sandbox.patch364.99 KBandypost
#14 interdiff.txt5.18 KBandypost
#14 comment-widget.patch378.95 KBandypost
#12 comment-widget.patch377.3 KBandypost
#10 8.patch368.85 KBandypost
#8 7.patch377.81 KBandypost
#6 5.patch378.29 KBandypost
#3 3.patch378.29 KBandypost
#1 1.patch377.15 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

FileSize
377.15 KB
andypost’s picture

Assigned: Unassigned » andypost
Status: Active » Needs review
FileSize
378.29 KB

Fixed some tests

andypost’s picture

FileSize
378.29 KB

Another fixes

andypost’s picture

FileSize
377.81 KB
andypost’s picture

FileSize
368.85 KB

Main branch tests

andypost’s picture

FileSize
377.3 KB

Another round to move settings to widget

andypost’s picture

FileSize
378.95 KB
5.18 KB

Fixed some tests, just moved other settings to widget in tests

-    $this->setCommentSettings('comment_default_mode', COMMENT_MODE_THREADED, 'Comment paging changed.');
+    $this->setWidgetSettings('default_mode', COMMENT_MODE_THREADED, 'Comment paging changed.');

can't reproduce upgrade tests failures locally so probably depends on php version

andypost’s picture

FileSize
364.99 KB

Testing of sandbox code

andypost’s picture

Status: Needs review » Needs work

Filed #1916556-1: Random test failures - The string cannot be saved with current sandbox code, but patch passed the tests

andypost’s picture

Status: Needs work » Needs review
FileSize
365.5 KB

Commited schema changes for proper length of keys

andypost’s picture

FileSize
378.92 KB

my patch with formatters. fixed formatter and re-roll for HEAD

andypost’s picture

FileSize
368.44 KB
32.54 KB

Patch against sandbox - adds formatter and widget + upgrade path for entity display

andypost’s picture

FileSize
392.47 KB
389.53 KB
2.46 KB

So probably this caused by my attempt to minimize a key size according #1852896: Throw an exception if a schema defines a key that would be over 1000 bytes in MySQL

andypost’s picture

FileSize
368.44 KB

Current sandbox after merge

andypost’s picture

FileSize
368.44 KB

Current sandbox code

andypost’s picture

Used a way like user_update_8011()

andypost’s picture

andypost’s picture

Next round should be:
- fix views (required for RTBC)

- Move instance settings to formatter settings
- get rid of hook_entity_view()

+++ b/core/modules/comment/comment.installundefined
@@ -330,6 +330,16 @@ function comment_field_schema($field) {
 /**
+ * Implements hook_update_dependencies().
+ */
+function comment_update_dependencies() {
+  $dependencies['comment'][8006] = array(
+    'field' => 8002,
+  );
+  return $dependencies;

Not sure it's needed

andypost’s picture

FileSize
368.89 KB

Let's try

andypost’s picture

FileSize
371.73 KB

- fix joins for the last_comment_name field/sort
- try to get field_view_field working with entityNG (possible not the best workaround)

dixon_’s picture

New patch based on #33 with updates from Berdir's latest review in the main issue.

andypost’s picture

Reverted back result of comment_field_name_load() used for field_ui to return a bundle name string

EDIT I'm working on changed upgrade path & tests to add comment fields only for node types that have comments open OR have at least one comment OR any node that have comments enabled

andypost’s picture

FileSize
13.44 KB

Started to optimize upgrade path, still needs some work to properly tests statistics and conversion of disabled nodes
Added "mark deleted" unused fields that could be attached to comment bundle

andypost’s picture

andypost’s picture

andypost’s picture

FileSize
367.74 KB
35 KB

current sandbox

andypost’s picture

FileSize
362.84 KB
4.82 KB

Fixed remains of ER convertion

current diffstat
105 files changed, 3276 insertions(+), 1425 deletions(-)

andypost’s picture

Current sandbox.. minimized patch

105 files changed, 3243 insertions(+), 1416 deletions(-)

andypost’s picture

FileSize
369.13 KB

and again

andypost’s picture

I bit of more clean-up in touched doc-blocks
Also testing "m" minimized (-M25%) git option

andypost’s picture

FileSize
360.03 KB

let's re-test sandbox

andypost’s picture

andypost’s picture

FileSize
374.97 KB
andypost’s picture

FileSize
371.51 KB

merged HEAD

jthorson’s picture

On testbot, I'm seeing an error page after attempting to apply updates on the failed tests (and it isn't detected by UpgradePathTestBase.php):

An unrecoverable error has occured. You can find the error message below. It is advised to copy it to the clipboard for reference. Please continue to the error page

However, the error is not displayed, and progressing to the error page can't happen after the batch has completed. I'm going to try and add debug code to detect this scenario and hijack the test so that we can see what actual error is being triggered.

EDIT: Clicking through on the error page link is resulting in a SQL warning:

Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest942233cache_toolbar' doesn't exist: TRUNCATE {cache_toolbar} ; Array () Group: Drupal\Core\Database\DatabaseExceptionWrapper

The verbose output from clicking the error page link is empty (zero bytes).

EDIT2: Opened #1922034: UpgradePathTestBase does not detect "An unrecoverable error has occurred" install errors. for the UpgradePathTestBase error detection.

jthorson’s picture

Issue summary: View changes

added related

jthorson’s picture

Status: Needs review » Needs work

Captured the string(s) tripping the StringStorageException in StringDatabaseStorage.php line 513:

'String:
Drupal\\locale\\SourceString Object
(
    [lid] => 
    [locations] => Array
        (
            [path] => Array
                (
                    [/checkout/core/update.php?op=selection&token=7l8rG8PMDucSjF-dhNN2W5jCKnKd3O-EitmLk314gjs&id=8&op=do_nojs] => 1
                )

        )

    [source] => Created required fields for each comment bundle
    [context] => 
    [version] => 8.0-dev
    [storage:protected] => Drupal\\locale\\StringDatabaseStorage Object
        (
            [connection:protected] => Drupal\\Core\\Database\\Driver\\mysql\\Connection Object
                (
                    [needsCleanup:protected] => 
                    [target:protected] => default
                    [key:protected] => default
                    [logger:protected] => 
                    [transactionLayers:protected] => Array
                        (
                        )

                    [driverClasses:protected] => Array
                        (
                            [Merge] => Drupal\\Core\\Database\\Driver\\mysql\\Merge
                            [Select] => Drupal\\Core\\Database\\Driver\\mysql\\Select
                            [Insert] => Drupal\\Core\\Database\\Driver\\mysql\\Insert
                            [Update] => Drupal\\Core\\Database\\Driver\\mysql\\Update
                            [Delete] => Drupal\\Core\\Database\\Driver\\mysql\\Delete
                            [Schema] => Drupal\\Core\\Database\\Driver\\mysql\\Schema
                        )

                    [statementClass:protected] => Drupal\\Core\\Database\\Statement
                    [transactionSupport:protected] => 1
                    [transactionalDDLSupport:protected] => 
                    [temporaryNameIndex:protected] => 0
                    [connectionOptions:protected] => Array
                        (
                            [database] => drupaltestbotmysql
                            [username] => drupaltestbot-my
                            [password] => AorwZB8hFWzB
                            [host] => localhost
                            [port] => 
                            [namespace] => Drupal\\Core\\Database\\Driver\\mysql
                            [driver] => mysql
                            [prefix] => Array
                                (
                                    [default] => simpletest384124
                                )

                        )

                    [schema:protected] => Drupal\\Core\\Database\\Driver\\mysql\\Schema Object
                        (
                            [connection:protected] => Drupal\\Core\\Database\\Driver\\mysql\\Connection Object
 *RECURSION*
                            [placeholder:protected] => 0
                            [defaultSchema:protected] => public
                            [uniqueIdentifier:protected] => 51244e13bef2b3.58452716
                        )

                    [prefixes:protected] => Array
                        (
                            [default] => simpletest384124
                        )

                    [prefixSearch:protected] => Array
                        (
                            [0] => {
                            [1] => }
                        )

                    [prefixReplace:protected] => Array
                        (
                            [0] => simpletest384124
                            [1] => 
                        )

                )

            [options:protected] => Array
                (
                    [target] => default
                )

        )

)
'
'String:
Drupal\\locale\\SourceString Object
(
    [lid] => 
    [locations] => Array
        (
            [path] => Array
                (
                    [/checkout/core/update.php?op=selection&token=7l8rG8PMDucSjF-dhNN2W5jCKnKd3O-EitmLk314gjs&id=8&op=do_nojs] => 1
                )

        )

    [source] => %type: !message in %function (line %line of %file).
    [context] => 
    [version] => 8.0-dev
    [storage:protected] => Drupal\\locale\\StringDatabaseStorage Object
        (
            [connection:protected] => Drupal\\Core\\Database\\Driver\\mysql\\Connection Object
                (
                    [needsCleanup:protected] => 
                    [target:protected] => default
                    [key:protected] => default
                    [logger:protected] => 
                    [transactionLayers:protected] => Array
                        (
                        )

                    [driverClasses:protected] => Array
                        (
                            [Merge] => Drupal\\Core\\Database\\Driver\\mysql\\Merge
                            [Select] => Drupal\\Core\\Database\\Driver\\mysql\\Select
                            [Insert] => Drupal\\Core\\Database\\Driver\\mysql\\Insert
                            [Update] => Drupal\\Core\\Database\\Driver\\mysql\\Update
                            [Delete] => Drupal\\Core\\Database\\Driver\\mysql\\Delete
                            [Schema] => Drupal\\Core\\Database\\Driver\\mysql\\Schema
                        )

                    [statementClass:protected] => Drupal\\Core\\Database\\Statement
                    [transactionSupport:protected] => 1
                    [transactionalDDLSupport:protected] => 
                    [temporaryNameIndex:protected] => 0
                    [connectionOptions:protected] => Array
                        (
                            [database] => drupaltestbotmysql
                            [username] => drupaltestbot-my
                            [password] => AorwZB8hFWzB
                            [host] => localhost
                            [port] => 
                            [namespace] => Drupal\\Core\\Database\\Driver\\mysql
                            [driver] => mysql
                            [prefix] => Array
                                (
                                    [default] => simpletest384124
                                )

                        )

                    [schema:protected] => Drupal\\Core\\Database\\Driver\\mysql\\Schema Object
                        (
                            [connection:protected] => Drupal\\Core\\Database\\Driver\\mysql\\Connection Object
 *RECURSION*
                            [placeholder:protected] => 0
                            [defaultSchema:protected] => public
                            [uniqueIdentifier:protected] => 51244e13bef2b3.58452716
                        )

                    [prefixes:protected] => Array
                        (
                            [default] => simpletest384124
                        )

                    [prefixSearch:protected] => Array
                        (
                            [0] => {
                            [1] => }
                        )

                    [prefixReplace:protected] => Array
                        (
                            [0] => simpletest384124
                            [1] => 
                        )

                )

            [options:protected] => Array
                (
                    [target] => default
                )

        )

)
'
jthorson’s picture

From that same function:
if (($table = $this->dbStringTable($string)) && ($fields = $this->dbStringValues($string, $table))) {

$table = locales_source
$fields = array()

Once the test completes, I don't see a locales_source table in the database ... though I'm not sure whether I should?

andypost’s picture

Status: Needs work » Needs review
FileSize
372.48 KB
andypost’s picture

FileSize
372.13 KB
634 bytes

Let's see, seems data in upgrade broken

andypost’s picture

FileSize
373.1 KB
1.59 KB

Both patches

andypost’s picture

FileSize
371.53 KB
1.66 KB

Let's see if get_t() removed

yched’s picture

core/modules/node/templates/node.html.twig :
     {# We hide the comments and links now so that we can render them later. #}
-    {% hide(content.comments) %}
     {% hide(content.links) %}

The comment needs to be updated accordingly :-p. Same in node.tpl.php

larowlan’s picture

Great work here @andypost

andypost’s picture

FileSize
374.75 KB
3.59 KB

@yched thanx for pointing. Now let's see how this removal affects a tests :)

Re-open #1916556: Random test failures - The string cannot be saved with proper details

larowlan’s picture

woot!
Moving over to main issue at @andpost's request

andypost’s picture

FileSize
366.26 KB
1.79 KB

Merged HEAD

podarok’s picture

few tests with #73
15 comments without tree
10 comments per page
second page for testing
cache disabled
ab third run for drupal warm-up

$ /d/php/apache/bin/ab -c5 -n1000 http://127.0.0.1:8887/drupal/node/1?page=1
This is ApacheBench, Version 2.3 <$Revision: 1178079 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking 127.0.0.1 (be patient)
Completed 100 requests
Completed 200 requests
Completed 300 requests
Completed 400 requests
Completed 500 requests
Completed 600 requests
Completed 700 requests
Completed 800 requests
Completed 900 requests
Completed 1000 requests
Finished 1000 requests


Server Software:        Apache/2.4.2
Server Hostname:        127.0.0.1
Server Port:            8887

Document Path:          /drupal/node/1?page=1
Document Length:        5534 bytes

Concurrency Level:      5
Time taken for tests:   233.031 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      6078000 bytes
HTML transferred:       5534000 bytes
Requests per second:    4.29 [#/sec] (mean)
Time per request:       1165.156 [ms] (mean)
Time per request:       233.031 [ms] (mean, across all concurrent requests)
Transfer rate:          25.47 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   1.8      0      16
Processing:   828 1156 122.1   1125    1656
Waiting:      813 1127 120.8   1094    1625
Total:        828 1156 122.1   1125    1672

Percentage of the requests served within a certain time (ms)
  50%   1125
  66%   1203
  75%   1250
  80%   1266
  90%   1328
  95%   1375
  98%   1422
  99%   1484
 100%   1672 (longest request)

without patch

$ /d/php/apache/bin/ab -c5 -n1000 http://127.0.0.1:8887/drupal/node/1?page=1
This is ApacheBench, Version 2.3 <$Revision: 1178079 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking 127.0.0.1 (be patient)
Completed 100 requests
Completed 200 requests
Completed 300 requests
Completed 400 requests
Completed 500 requests
Completed 600 requests
Completed 700 requests
Completed 800 requests
Completed 900 requests
Completed 1000 requests
Finished 1000 requests


Server Software:        Apache/2.4.2
Server Hostname:        127.0.0.1
Server Port:            8887

Document Path:          /drupal/node/1?page=1
Document Length:        5541 bytes

Concurrency Level:      5
Time taken for tests:   235.469 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      6085000 bytes
HTML transferred:       5541000 bytes
Requests per second:    4.25 [#/sec] (mean)
Time per request:       1177.344 [ms] (mean)
Time per request:       235.469 [ms] (mean, across all concurrent requests)
Transfer rate:          25.24 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   1.8      0      16
Processing:   891 1165 259.8   1125    5078
Waiting:      859 1134 228.6   1094    4938
Total:        891 1165 259.8   1125    5078

Percentage of the requests served within a certain time (ms)
  50%   1125
  66%   1203
  75%   1234
  80%   1266
  90%   1328
  95%   1391
  98%   1438
  99%   1469
 100%   5078 (longest request)

looks like we have no performance regression

andypost’s picture

See patch #1870036-4: Comment settings for content generated by devel is coming closed. to generate comments for nodes again.

To make tests:
1) install vanila core
2) use devel_generate to make nodes with comments
3) apply patch
4) run update.php

andypost’s picture

@larowlan Please check this clean-up and commit if green. Also I think you can write better comment about Only first value

I think better to have default value in one place.

Also UI should be collapsed when same as default

andypost’s picture

FileSize
388.46 KB
42.43 KB

Initial patch for formatters

andypost’s picture

FileSize
388.57 KB
3.78 KB

small fixes

andypost’s picture

FileSize
388.59 KB
578 bytes

fix upgrade path. Links still need some work

andypost’s picture

FileSize
388.63 KB
8.91 KB

HEAD was broken

andypost’s picture

FileSize
389.83 KB
13.39 KB

last one for today, should work except rdf and search

andypost’s picture

andypost’s picture

FileSize
374.95 KB
17.98 KB

Some clean-up to minimize patch size

andypost’s picture

FileSize
395.53 KB

Another round on formatters

andypost’s picture

FileSize
387.76 KB

Reverted back comment_node_view() for rss, changed rdf and merged current sandbox

andypost’s picture

FileSize
375.72 KB

merging head

podarok’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/comment.installundefined
@@ -36,50 +38,52 @@ function comment_uninstall() {
+  $comment_fields = comment_get_comment_fields();
+  $entity_info = entity_get_info();
+++ b/core/modules/comment/comment.views.incundefined
@@ -322,6 +314,61 @@ function comment_views_data() {
+  // Provide a relationship for each entity type except comment.
+  foreach (entity_get_info() as $type => $entity_info) {
+++ b/core/modules/comment/comment.views.incundefined
@@ -372,18 +419,31 @@ function comment_views_data() {
+  // Provide a relationship for each entity type except comment.
+  foreach (entity_get_info() as $type => $entity_info) {
+++ b/core/modules/comment/comment.views.incundefined
@@ -495,57 +571,70 @@ function comment_views_data_alter(&$data) {
+  // Provide a integration for each entity type.
+  foreach (entity_get_info() as $entity_type => $entity_info) {
+    if (!isset($entity_info['base_table'])) {
+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/argument/UserUid.phpundefined
@@ -57,16 +57,28 @@ function default_actions($which = NULL) {
+    if ($this->table != 'comment') {
+      $table_info = drupal_container()->get('views.views_data')->get($this->table);
+      $entity_info = entity_get_info($table_info['table']['entity type']);

entity_get_info() deprecated. Use drupal_container()->get('plugin.manager.entity')->getDefinitions()
http://drupal.org/node/1929006

All other looks good

larowlan’s picture

larowlan’s picture

@andypost - patch at #95 based on sandbox HEAD - is that up to date? Size of patch has dropped.

andypost’s picture

FileSize
375.93 KB
1.88 KB

Somehow Annotation now lives in Component

andypost’s picture

Locally the tests are passed
#98: sandbox-98.patch queued for re-testing.

larowlan’s picture

Green

andypost’s picture

FileSize
366.93 KB

testing changes before commit

andypost’s picture

FileSize
375.67 KB

Pushed this commit, testing sandbox

andypost’s picture

FileSize
375.51 KB

Changed entity_get_info() to use Drupal::service() notation also removed some unused calls

Failed tests are caused by statically cached entity_info so comment_views_data() operates with wrong info.
So views_invalidate_cache() could be removed in DefaultViewRecentComments.php

interdiff http://pastebin.com/pa1jz0BW

andypost’s picture

andypost’s picture

FileSize
379 KB

Fixed broken tests except views handlers

andypost’s picture

FileSize
380.87 KB
3.96 KB

This fixes 'entity_id', 'entity_type' and 'field_name' but NodeNewComments still broken - views wont join statistics table

larowlan’s picture

This adds \Drupal\views\ViewsDataCache::flush() to selectively clear \Drupal\views\ViewsDataCache->storage
Uses that to flush comment and comment_entity_statistics form storage after adding the fields.
Couldn't see any other way to do it with the existing class.
Also adds a check in field_view_field to make sure the field actually contains a value for non NG entities.

@andypost, this not yet committed to the sandbox

larowlan’s picture

Committed patch at 114 to sandbox.
Changed the flush() method to delete() at request of @dawehner and also clears the cache entries at the same time (also at his suggestion).
This not yet committed to sandbox

larowlan’s picture

Incorrect reference to $table in 115.

andypost’s picture

Awesome! @larowlan++

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.php
@@ -256,4 +256,19 @@ public function destruct() {
+  /**
+   * Allows an entry in the storage to be removed.
+   *
+   * @param string $key
+   *   (Optional) The key to clear in the storage. Defaults to NULL.
+   */

Suppose this change should be filed as separate issue. As increased DX

dawehner’s picture

If we add a new method to the viewsDataCache we should certainly add a new test for that, so maybe we should as andypost said move this to a new issue.

In general this ViewsDataCache changes look pretty good.

larowlan’s picture

Patch at 116 pushed to sandbox, will file new issue for views changes

larowlan’s picture

larowlan’s picture

larowlan’s picture

Chasing HEAD

andypost’s picture

FileSize
387.9 KB
13.95 KB

New patch with upgrade path

andypost’s picture

FileSize
379.96 KB

Sanbox re-test

andypost’s picture

Status: Needs review » Needs work
FileSize
389.08 KB
381.14 KB
1.64 KB
andypost’s picture

Status: Needs work » Needs review

Pushed follow-up commit for

 /**
- * Defines the 'test_field' entity field item.
+ * Defines the 'comment_field' entity field item.
  */
andypost’s picture

And now in proper place

andypost’s picture

Status: Needs review » Needs work

Looks like comment_entity_load() is broken, so no comment_statistics attached and no Hidden option availiable

andypost’s picture

Status: Needs work » Needs review
FileSize
381.31 KB
4.96 KB

Here's a interdiff to be commited:
1) LANGUAGE should not be used in drupalCreateNode() for comment field
2) comment.maintain_entity_statistics is not converted to state
3) fix for come comment could be skipped

views relation for node broken again so views.view.comments_recent.yml is not working (suppose we need @dawehner here again)

dawehner’s picture

FileSize
376.09 KB

Does someone know why the interdiff fails?

andypost’s picture

FileSize
373.15 KB

http://privatepaste.com/b5e8547945 dawehner's changes are merged and commited http://privatepaste.com/2468b5ddc7
Also added check that entity has comment fields in comment_entity_load() and comment_entity_insert()

andypost’s picture

Status: Needs review » Needs work
andypost’s picture

Status: Needs work » Needs review
FileSize
372.63 KB

Let's see how much broken for last merge

larowlan’s picture

andypost’s picture

FileSize
373.09 KB

Another merge

andypost’s picture

FileSize
374.12 KB
1.93 KB

Fix some broken tests

andypost’s picture

FileSize
376.81 KB

@dawehner++

andypost’s picture

FileSize
377.84 KB
1.59 KB

Should be green. Upgrade tests are fixed
Commits list

Interdiff to show last commit (debug does not included - just to point why === changed to ==)

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/formatter/CommentDefaultFormatter.phpundefined
@@ -0,0 +1,86 @@
+      $comment_settings = $this->instance['settings'];
...
+        $mode = $comment_settings['default_mode'];
+        $comments_per_page = $comment_settings['per_page'];
+        if ($cids = comment_get_thread($entity, $field['field_name'], $mode, $comments_per_page)) {

$mode always returns as string value

andypost’s picture

FileSize
982 bytes
386.9 KB

As fields now entities - the easiest way to add right dependency

andypost’s picture

FileSize
378.13 KB

Fix random failures

andypost’s picture

Status: Needs review » Needs work
FileSize
2.42 KB
2.53 KB

Current todo:

1) Fix broken tests - needs to solve dependency graph
patch comment-updates.txt - user pictures still broken

2) Find a way to implement 'entity_id' definition constrains
patch comment-def.txt - still no clue how 'properly' to get entity

3) Fix Drupal/comment/Plugin/entity_reference/selection/CommentSelection.php
Looks similar to 2) but needs to remove node relation

fago’s picture

Why are you overriding getValue() in CommentEntityItem ? It's not supposed to be computed, is it?

larowlan’s picture

Status: Needs work » Needs review
FileSize
377.51 KB

Chases head, still to-do's as per #154 and any test fails

larowlan’s picture

Status: Needs review » Needs work

So looks like same fails as above plus the upgrade issues identified at #154

larowlan’s picture

Adds the update deps from #154

larowlan’s picture

larowlan’s picture

andypost’s picture

Fix broken tests git show 5a22142..73ed66a > interdiff.txt

PS decouple-comment-node-731724.164_.patch just same patch with -M25%

larowlan’s picture

Reverts 4ec8332
Also xhprof output is attached:

Before:

Total Incl. Wall Time (microsec):	5,834,274 microsecs
Total Incl. CPU (microsecs):	5,744,359 microsecs
Total Incl. MemUse (bytes):	15,029,036 bytes
Total Incl. PeakMemUse (bytes):	15,960,536 bytes
Number of Function Calls:	469,702

After:

Total Incl. Wall Time (microsec):	5,817,289 microsecs
Total Incl. CPU (microsecs):	5,716,357 microsecs
Total Incl. MemUse (bytes):	13,609,980 bytes
Total Incl. PeakMemUse (bytes):	14,551,396 bytes
Number of Function Calls:	479,456
larowlan’s picture

Note xhprof output is for node view with 100 comments and per-page set to 150 (there is no 100 option)

larowlan’s picture

Reverting 4ec8332 broke file access

larowlan’s picture

andypost’s picture

FileSize
381.87 KB

Merge for Issue #1043198: Convert view modes to ConfigEntity
also commited

--- a/core/modules/comment/lib/Drupal/comment/Tests/CommentAdminTest.php
+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentAdminTest.php
@@ -2,7 +2,7 @@
 
 /**
  * @file
- * Definition of Drupal\comment\Tests\CommentAdminTest.
+ * Contains \Drupal\comment\Tests\CommentAdminTest.
  */
andypost’s picture

FileSize
381.95 KB
4.28 KB

Fix routing with stab in hook_menu()
Normalize path to common comments/manage/{bundle}
Changed deprecated ControllerInterface

Also fix upgrade path tests

--- a/core/modules/comment/comment.install
+++ b/core/modules/comment/comment.install
@@ -582,12 +582,11 @@ function comment_update_8006(&$sandbox) {
     }
     _update_7000_field_create_field($field);
 
-    $existing_instance = db_select('field_config_instance', 'f')
-      ->condition('field_name', $field['field_name'])
-      ->condition('bundle', $node_type)
-      ->condition('entity_type', 'node')
-      ->execute()
-      ->fetch();
+    $existing_instance = (bool) db_query_range('SELECT 1 FROM {field_config_instance} WHERE field_name = :field_name AND bundle = :bundle AND entity_type = :ent
+      'field_name' => $field['field_name'],
+      'bundle' => $node_type,
+      'entity_type' => 'node',
+    ))->fetchField();
andypost’s picture

FileSize
381.98 KB
2.37 KB

Fix the rest broken tests. I think /manage is great improvement so allowing contrib to attach own tabs to comment bundles

andypost’s picture

FileSize
383.69 KB
5.18 KB

Another clean-ups
- form alter
- bundle stub-page

alexpott’s picture

+++ b/core/modules/comment/comment.admin.incundefined
@@ -82,23 +82,30 @@ function comment_admin_overview($form, &$form_state, $arg) {
+  if (module_exists('node')) {
+    // Special case to ensure node access works.

Module_exists is deprecated... use Drupal::moduleHandler() instead

+++ b/core/modules/comment/comment.api.phpundefined
@@ -198,5 +202,36 @@ function hook_comment_delete(Drupal\comment\Comment $comment) {
+ * @todo replace this with entity access controls once generic access controller
+ *   lands.
+ *
+ * @see http://drupal.org/node/1696660

This has happened see #1696660: Add an entity access API for single entity access

+++ b/core/modules/comment/comment.info.ymlundefined
@@ -6,6 +6,5 @@ version: VERSION
-  - node

yay!

+++ b/core/modules/comment/comment.installundefined
@@ -36,50 +38,52 @@ function comment_uninstall() {
+  state()->set('comment.maintain_entity_statistics', TRUE);

+++ b/core/modules/comment/comment.moduleundefined
@@ -974,233 +1017,156 @@ function comment_view_multiple($comments, $view_mode = 'full', $langcode = NULL)
+  if ($fields && state()->get('comment.maintain_entity_statistics', TRUE)) {

+++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.phpundefined
@@ -181,45 +172,56 @@ protected function postDelete($comments) {
+    if (!state()->get('comment.maintain_entity_statistics', TRUE)) {

get only accepts a key...
should be something like
$maintain_entity_statistics = state()->get('comment.maintain_entity_statistics') ?: TRUE;

+++ b/core/modules/comment/comment.installundefined
@@ -390,7 +451,321 @@ function comment_update_8003(&$sandbox) {
+    $existing_instance = (bool) db_query_range('SELECT 1 FROM {field_config_instance} WHERE field_name = :field_name AND bundle = :bundle AND entity_type = :entity_type', 0, 1, array(

Shouldn't we be using the config system here...

+++ b/core/modules/comment/comment.installundefined
@@ -390,7 +451,321 @@ function comment_update_8003(&$sandbox) {
+    module_load_install('entity');

deprecated in favour of the module handler if the entity module has been enabled at this point

+++ b/core/modules/comment/comment.moduleundefined
@@ -4,18 +4,23 @@
+require_once DRUPAL_ROOT . '/core/modules/comment/comment.field.inc';

Should be require __DIR__ . '/comment.field.inc'; ... I don't think we need require_once we already ensure we load the comment.module only once...

+++ b/core/modules/comment/comment.moduleundefined
@@ -62,19 +67,39 @@
-const COMMENT_NODE_OPEN = 2;

This is still referenced in node.api.php

+++ b/core/modules/comment/comment.moduleundefined
@@ -98,38 +123,154 @@ function comment_help($path, $arg) {
+  if ($entity->entityType() != 'node') {
+    // Only content needs comment links.
+    return;
+  }

Why? This comment could do with some more meat... I would have thought other entity type might want to benefit from the functionality...

+++ b/core/modules/comment/comment.moduleundefined
@@ -98,38 +123,154 @@ function comment_help($path, $arg) {
+  $fields = field_info_instances($entity->entityType(), $entity->bundle());
+  foreach ($fields as $field_name => $instance) {
+    $links = array();
+    $field = field_info_field($field_name);
+    if ($field['type'] != 'comment') {
+      continue;
+    }

Is there really not more optimal way of doing this... 10 node teasers on the frontpage... each has 10 fields 2 of which is a comment field... aren't we looping 90 times unnecessarily here?

+++ b/core/modules/comment/comment.moduleundefined
@@ -256,93 +403,107 @@ function comment_menu() {
+  // Legacy redirect handler for links of form comment/reply/%nid
+  if (module_exists('node')) {
+    $items['comment/reply/%node'] = array(
+      'title' => 'Add new comment',
+      'page callback' => 'comment_node_redirect',
+      'page arguments' => array(2),
+      'access callback' => 'node_access',
+      'access arguments' => array('view', 2),
+      'file' => 'comment.pages.inc',
+    );
+  }

+++ b/core/modules/comment/comment.pages.incundefined
@@ -120,5 +141,33 @@ function comment_approve(Comment $comment) {
+/**
+ * Page callback: Legacy handler to redirect old comment reply urls to new url.
+ *
+ * @param \Drupal\node\Plugin\Core\Entity\Node $node
+ *   The node to which the comments are attached.
+ * @param int $pid
+ *   (optional) Some comments are replies to other comments. In those cases,
+ *   $pid is the parent comment's comment ID. Defaults to NULL.
+ *
+ * @throw \Symfony\Component\HttpKernel\Exception\NotFoundHttpException
+ *   When there are no comment fields attached to the node.
+ */
+function comment_node_redirect(Node $node, $pid = NULL) {
+  $fields = comment_get_comment_fields('node');
+  foreach ($fields as $field_name => $detail) {
+    // Pick the first comment field found on the node.
+    if (in_array($node->bundle(), $detail['bundles']['node'], TRUE)) {
+      drupal_goto('comment/reply/node/' . $node->id() . '/' . $field_name);
+    }
+  }
+  throw new NotFoundHttpException();

Should be a route... and return a redirect response

+++ b/core/modules/comment/comment.moduleundefined
@@ -256,93 +403,107 @@ function comment_menu() {
+function comment_reply_access(EntityInterface $entity) {
+  $function = $entity->entityType() . '_access';
+  // @todo replace this with entity access controls once generic access
+  // controller lands.
+  // @see http://drupal.org/node/1696660
+  if (function_exists($function)) {
+    switch ($function) {
+      case 'user_access':
+        return $entity->access('view');
 
-/**
- * Implements hook_node_type_update().
- */
-function comment_node_type_update($info) {
-  if (!empty($info->old_type) && $info->type != $info->old_type) {
-    entity_invoke_bundle_hook('rename', 'comment', 'comment_node_' . $info->old_type, 'comment_node_' . $info->type);
+      case 'taxonomy_term_access':
+        return user_access('access content');
+
+      default:
+        return $function('view', $entity);
+    }
   }

Generic controller has landed and the needs more comments if it stays in like this..

+++ b/core/modules/comment/comment.moduleundefined
@@ -256,93 +403,107 @@ function comment_menu() {
+  return !in_array(COMMENT_ACCESS_DENY, $access, TRUE);

Do we really need the strict type checking here?

... note to self... I've got to comment_new_page_count()

andypost’s picture

FileSize
385.41 KB
11.91 KB

Fixed part of 177, also changed type-hitting to CommentInterface

andypost’s picture

FileSize
386.13 KB
1.3 KB

merged head and reverted access check - node_access() works different then $node->access('view')

larowlan’s picture

Shouldn't we be using the config system here...

this runs before the field -> config entity update path so must reference the old table.
There is no helper function for instances (like for fields)
The rest seems reasonable, but I'd ask if we could punt the links on other than node to a follow up (we have one already for moving those to a formatter option)

larowlan’s picture

Should be a route... and return a redirect response

@andypost - I have this bit done locally

larowlan’s picture

pushed
b08132e Move comment_node_redirect to a route to sandbox

aspilicious’s picture

Where will the module exist for nodes happen?

andypost’s picture

FileSize
388.38 KB
2.32 KB

Suppose better to drop this legacy redirect. Otherwise we need to implement http://drupal.org/node/1800686 route subsciber to check for node module(see Dynamic paths) tnx to @aspilicious

Should be green - fix entity form display with upgrade path

EDIT pushed weight change

--- a/core/modules/comment/comment.install
+++ b/core/modules/comment/comment.install
@@ -641,7 +641,7 @@ function comment_update_8006(&$sandbox) {
     $display_options_default = array(
       'type' => 'comment_default',
       'settings' => array(),
-      'weight' => 1,
+      'weight' => 50,
larowlan’s picture

Pushed to sandbox

* 060e237 Adds route subscriber for redirect and tests
* d3112fc Refactor comment_entity_view to use comment_get_comment_fields
* b492e7e Clarify comments around comment links
* d23a306 Remove reference to COMMENT_NODE_HIDDEN in node.api.php

@alexpott can we get some more reviews?

andypost’s picture

FileSize
935 bytes

pushed last change and filed RTBC patch to #731724-406: Convert comment settings into a field to make them work with CMI and non-node entities

27ca306 Remove reference to COMMENT_NODE_OPEN in node.api.php
andypost’s picture

test merge after twig, still not fixed hunk for comment-wrapper

-  <?php if ($content['comments'] && $node->type != 'forum'): ?>
+  <?php if ($content['comments'] && ($entity->entityType() != 'node' || $entity->bundle() != 'forum')): ?>
andypost’s picture

FileSize
390.38 KB
1.02 KB

Converted

andypost’s picture

FileSize
391.01 KB

Another merge patch

andypost’s picture

FileSize
391.28 KB
11.61 KB

merge for language

andypost’s picture

patch is rtbc, could be re-rolled to primary issue

larowlan’s picture

andypost’s picture

FileSize
394.33 KB
887 bytes

should fix most of tests

andypost’s picture

FileSize
389.57 KB
6.67 KB

Fix block and minimize patch size

EDIT
Sandbox code now
24a23188 Fix SearchCommentTest

Currently only 2 tests are broken
HandlerAllTest.php

Drupal\Core\Database\DatabaseExceptionWrapper: Exception in []: SQLSTATE[42S22]: Column not found: 1054 Unknown column &#039;node.uid&#039; in &#039;where clause&#039;: SELECT COUNT(*) AS expression FROM (SELECT 1 AS expression FROM {node} node LEFT JOIN {comment} comment_node ON node.nid = comment_node.entity_id AND (comment_node.entity_type = :views_join_condition_0 AND comment_node.field_name = :views_join_condition_1) LEFT JOIN {comment} comment_node_1 ON node.nid = comment_node_1.entity_id AND (comment_node_1.entity_type = :views_join_condition_2 AND comment_node_1.field_name = :views_join_condition_3) LEFT JOIN {taxonomy_index} taxonomy_index ON node.nid = taxonomy_index.nid LEFT JOIN {taxonomy_term_data} taxonomy_term_data_node ON taxonomy_index.tid = taxonomy_term_data_node.tid INNER JOIN {comment_entity_statistics} comment_entity_statistics ON node.nid = comment_entity_statistics.entity_id AND comment_entity_statistics.entity_type = :views_join_condition_4 WHERE (( (node.nid = :db_condition_placeholder_0) AND (node.langcode IN (:db_condition_placeholder_1)) AND( (node.uid = :db_condition_placeholder_2) OR ( EXISTS (SELECT c.cid AS cid FROM {comment} c WHERE (c.uid = :db_condition_placeholder_3) AND (c.entity_id = node.nid) AND (c.entity_type = :db_condition_placeholder_4) )) )AND (node.nid IN (SELECT tn.nid AS nid FROM {taxonomy_index} tn WHERE ( (tn.tid = :db_condition_placeholder_5) ))) ))) subquery; Array ( [:db_condition_placeholder_0] =&gt; [:db_condition_placeholder_1] =&gt; 1 [:db_condition_placeholder_2] =&gt; [:db_condition_placeholder_3] =&gt; [:db_condition_placeholder_4] =&gt; node [:db_condition_placeholder_5] =&gt; 1 [:views_join_condition_0] =&gt; node [:views_join_condition_1] =&gt; comment_node_forum [:views_join_condition_2] =&gt; node [:views_join_condition_3] =&gt; comment [:views_join_condition_4] =&gt; node ) in Drupal\views\Plugin\views\query\Sql->execute() (line 1515 of ../core8/core/modules/views/lib/Drupal/views/Plugin/views/query/Sql.php).
Drupal\views\Plugin\views\query\Sql->execute(Object)
Drupal\views\ViewExecutable->execute()
Drupal\views\Tests\Handler\HandlerAllTest->testHandlers()

And ModulesDisabledUpgradePathTest.php because of forum_enable()

  if (!field_read_field('comment_node_forum', array('include_inactive' => TRUE))) {
    comment_add_default_comment_field('node', 'forum', 'comment_node_forum', COMMENT_OPEN);
  }

throws

Drupal\Core\Database\SchemaObjectExistsException: Table field_data_comment_node_forum already exists. in Drupal\Core\Database\Schema->createTable() (line 662 of.../www/core8/core/lib/Drupal/Core/Database/Schema.php).
Drupal\Core\Database\Schema->createTable('field_data_comment_node_forum', Array)
db_create_table('field_data_comment_node_forum', Array)
field_sql_storage_field_storage_create_field(Object)
call_user_func_array('field_sql_storage_field_storage_create_field', Array)
Drupal\Core\Extension\ModuleHandler->invoke('field_sql_storage', 'field_storage_create_field', Array)
Drupal\field\Plugin\Core\Entity\Field->save()
comment_add_default_comment_field('node', 'forum', 'comment_node_forum', 2)
forum_enable()
call_user_func_array('forum_enable', Array)
Drupal\Core\Extension\ModuleHandler->invoke('forum', 'enable')
Drupal\Core\Extension\ModuleHandler->enable(Array, 1)
module_enable(Array)
Drupal\system\Tests\Upgrade\ModulesDisabledUpgradePathTest->testDisabledUpgrade()
andypost’s picture

FileSize
389.29 KB

only a ModulesDisabledUpgradePathTest.php because of forum_enable() broken

larowlan’s picture

Status: Needs review » Needs work

We had the issue with forum_enable once before too, field wasn't being removed because of deps from memory. Need to force a purge perhaps?

andypost’s picture

@larowlan now it's different - field_data_comment... table exists but no inactive field definition could be found

andypost’s picture

FileSize
2.55 KB

There's debug that shows that tables exists without field definition we actually need a kind of dropped field_associate_fields() #1740432: Remove forum_enable()'s usage of field_associate_fields()

PS attached code helps to debug #208 by running ModulesDisabledUpgradePathTest.php

larowlan’s picture

I'm offline for weekend but can't we use forum/config for the comment field instead of field_create_field

andypost’s picture

Status: Needs work » Needs review
FileSize
389.67 KB
1018 bytes

It was hard to find that dependencies are not returned :)

EDIT probably another re-roll will be needed after #1969662: Provide new _update_8000_*() CRUD helpers for Field and Instance definitions

larowlan’s picture

ah Drupal commandment 6
Interdiff looks good to me, will move to main issue once bot returns

larowlan’s picture

ah Drupal commandment 6
Interdiff looks good to me, will move to main issue once bot returns

andypost’s picture

yay, green!

andypost’s picture

andypost’s picture

FileSize
389.13 KB

and again

andypost’s picture

FileSize
389.19 KB

Another merge

andypost’s picture

FileSize
385.76 KB

Another merge

andypost’s picture

FileSize
385.25 KB
4.23 KB

Fix upgrade path

andypost’s picture

FileSize
385.3 KB
403.12 KB

fix some notices after UserNG
Comment preview still broken

andypost’s picture

FileSize
385.41 KB
594 bytes

preview now works, but CommentUserTest still throws some notice in user_user_view()

andypost’s picture

FileSize
385.34 KB
2.39 KB

Finally fix reply notices

andypost’s picture

andypost’s picture

Node and comment links for other entities should not addressed in the patch in favour of #1875974: Abstract 'component type' specific code out of EntityDisplay

So finally we have
- field to store commenting state/status and comment form settings (better move'em to a kind of configurable to reference from field so comment form would be a kind of "image style")
- field instance - stores settings (per page, form bottom) for formatter, @todo inherit this settings to formatter
- widget - simply allows to set defaults and value for state (hidden, closed, Open)
- formatter - renders a thread and optionally displays form
- entity comment statistics and #1029708: History table for any entity

andypost’s picture

FileSize
385.37 KB

Merge after #1970360-94: Entities should define URI templates and standard links

+++ b/core/modules/rdf/rdf.module
@@ -417,7 +417,9 @@ function rdf_comment_load($comments) {
     // isn't needed until rdf_preprocess_comment() is called, but set it here
     // to optimize performance for websites that implement an entity cache.
     $comment->rdf_data['date'] = rdf_rdfa_attributes($comment->rdf_mapping['created'], $comment->created->value);
-    $comment->rdf_data['nid_uri'] = url('node/' . $comment->nid->target_id);
+    $entity = entity_load($comment->entity_type->value, $comment->entity_id->value);
+    $uri = $entity->uri();
+    $comment->rdf_data['entity_uri'] = url($uri['path']);
     if ($comment->pid->target_id) {
       $comment->rdf_data['pid_uri'] = url('comment/' . $comment->pid->target_id);
     }
+++ b/core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.php
   public function permalink() {
-
-    $url['path'] = 'node/' . $this->nid->value;
+    $entity = entity_load($this->get('entity_type')->value, $this->get('entity_id')->value);
+    $uri = $entity->uri();
+    $url['path'] = $uri['path'];
     $url['options'] = array('fragment' => 'comment-' . $this->id());
 
     return $url;

I see no way to skip loading of the entity

andypost’s picture

FileSize
386.2 KB

test

andypost’s picture

xjm’s picture

Assigned: andypost » xjm

Alright, time to review the beast. Sorry I didn't get to this last week.

I'll post partial reviews as I go through the patch, but please don't bother responding to them. They're just for my notes. :)

xjm’s picture

Why isn't #1920042: Upgrade path changes part of the main issue?

andypost’s picture

Upgrade path #1920042: Upgrade path changes is not included in the main issue because it's questionable.
Currently we create separate field for each node-type but #1920042 creates fields only for nodes with open comments or having any comments posted

EDIT Suppose we should add core/modules/system/tests/upgrade/drupal-7.comment.database.php and extend upgrade path in that issue

xjm’s picture

Personally, I'm very -1 to having any upgrade path changes and tests postponed to followups. That should all be rock solid in the main issue.

larowlan’s picture

larowlan’s picture

xjm’s picture

Status: Needs review » Needs work

Edit: Sorry, x-post.

Got paranoid after I lost my first review, so here's notes from the second time through 1/10 or so of the patch. (Up from the bottom of the patch through the top of tracker.)

Note that these are just random observations; I'll re-review after I've read the whole thing line by line and then post a comprehensive review on the main issue. Eventually.

  1. +++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerTest.phpundefined
    @@ -262,7 +258,6 @@ function testTrackerAdminUnpublish() {
    -      'comment' => 2,
    

    So, has this been removed entirely from drupalCreateNode()? Note to self: Check when we get there.

  2. +++ b/core/modules/tracker/tracker.moduleundefined
    @@ -314,8 +322,8 @@ function _tracker_add($nid, $uid, $changed) {
    -  $changed = db_query('SELECT changed FROM {node_field_data} WHERE nid = :nid AND default_langcode = 1 ORDER BY changed DESC', array(':nid' => $nid), array('target' => 'slave'))->fetchField();
    ...
    +  $changed = db_query('SELECT changed FROM {node_field_data} WHERE nid = :nid and default_langcode = 1 ORDER BY changed DESC', array(':nid' => $nid), array('target' => 'slave'))->fetchField();
    

    I cannot for the life of me see the difference between these two lines; I'll need to apply it locally and git diff --color-words.

  3. +++ b/core/modules/tracker/tracker.pages.incundefined
    @@ -52,10 +52,9 @@ function tracker_page($account = NULL, $set_title = FALSE) {
    -    // Now, get the data and put into the placeholder array.
         // @todo This should be actually filtering on the desired language and just
         //   fall back to the default language.
    -    $result = db_query('SELECT n.nid, l.comment_count FROM {node_field_data} n INNER JOIN {node_comment_statistics} l ON n.nid = l.nid WHERE n.nid IN (:nids) AND n.default_langcode = 1 ORDER BY n.changed DESC', array(':nids' => $nids), array('target' => 'slave'))->fetchAllKeyed();
    +    $result = db_query("SELECT n.nid, SUM(l.comment_count) AS comment_count FROM {node_field_data} n INNER JOIN {comment_entity_statistics} l ON n.nid = l.entity_id AND l.entity_type = 'node' INNER JOIN {users} u ON n.uid = u.uid WHERE n.nid IN (:nids) AND n.default_langcode = 1 GROUP BY n.nid", array(':nids' => array_keys($nodes)), array('target' => 'slave'))->fetchAllKeyed();
    

    Oy. So all of this is really in tracker's main page callback? Won't it be nice when it's a view? :) #1941830: Convert tracker listings to a view

    I was having trouble reading the query, so I split it up onto multiple lines:

    Old:

    SELECT n.nid, l.comment_count 
    FROM {node_field_data} n 
    INNER JOIN {node_comment_statistics} l ON n.nid = l.nid 
    WHERE n.nid IN (:nids) 
    AND n.default_langcode = 1 
    ORDER BY n.changed DESC
    

    New:

    SELECT n.nid, SUM(l.comment_count) 
    AS comment_count FROM {node_field_data} n 
    INNER JOIN {comment_entity_statistics} l ON n.nid = l.entity_id AND l.entity_type = 'node' 
    INNER JOIN {users} u ON n.uid = u.uid 
    WHERE n.nid IN (:nids) 
    AND n.default_langcode = 1 
    GROUP BY n.nid
    

    So, there are two changes that make sense: the table name and join, obviously, because the table has been changed to include all entity types... and then summing the total comments on all comment fields for the node, since one node type can have multiple different comment fields (per the summary of the main issue). That seems reasonable to me.

    But what's happening to the ORDER BY?

    Then:

    Inane observation 1: l is a really dumb table alias. That's out of scope, though. ;)

    Inane observation 2: Also out of scope, but default_langcode = 1? Really? That should be the appropriate constant, I think. Also the @todo. Wonder if it has an issue?

    Inane observation 3: I have no idea what the removed inline comment means out of context. I'm probably going to have to read all of tracker_page in its hideous majesty.

  4. +++ b/core/modules/tracker/tracker.pages.incundefined
    @@ -68,7 +67,7 @@ function tracker_page($account = NULL, $set_title = FALSE) {
    -        if ($new = comment_num_new($node->nid)) {
    +        if ($new = comment_num_new($node->nid, 'node')) {
    

    This seems like it should be a method on... something. Followup?

  5. +++ b/core/modules/user/lib/Drupal/user/Tests/UserSignatureTest.phpundefined
    @@ -38,6 +38,8 @@ function setUp() {
    +    // Add a comment field.
    +    comment_add_default_comment_field('node', 'page');
    
    @@ -83,7 +85,7 @@ function setUp() {
         // Create a new node with comments on.
    -    $node = $this->drupalCreateNode(array('comment' => COMMENT_NODE_OPEN));
    +    $node = $this->drupalCreateNode();
    

    Minor: So the second inline comment I've highlighted here (the context line) doesn't make sense anymore. Let's remove it and change the first inline comment (in setUp() to say something like:
    Add a comment field with commenting enabled by default.

  6. +++ b/core/modules/user/lib/Drupal/user/Tests/UserSignatureTest.phpundefined
    @@ -105,7 +107,7 @@ function testUserSignature() {
    -    $this->drupalPost('comment/reply/' . $node->nid, $edit, t('Preview'));
    +    $this->drupalPost('comment/reply/node/' . $node->nid .'/comment', $edit, t('Preview'));
    

    So the path to post a comment on an entity is
    comment/reply/entity_type/id/comment?

    Is there any reason this couldn't be something saner like
    path/to/entity/reply? (Totally could be a followup.)

  7. +++ b/core/modules/views/config/views.view.comments_recent.ymlundefined
    @@ -42,10 +42,11 @@ display:
    +          required: '1'
    

    Previously, it wasn't necessary to require this relationship because there were no comments without the relationship. Makes sense.

  8. +++ b/core/modules/views/lib/Drupal/views/Tests/DefaultViewsTest.phpundefined
    @@ -84,6 +84,10 @@ protected function setUp() {
    +    $this->container->get('views.views_data')->clear();
    
    +++ b/core/modules/views/lib/Drupal/views/Tests/Entity/FieldEntityTest.phpundefined
    @@ -45,9 +45,18 @@ public function testGetEntity() {
    +    \Drupal::service('views.views_data')->clear();
    
    +++ b/core/modules/views/lib/Drupal/views/Tests/Handler/HandlerAllTest.phpundefined
    @@ -55,6 +55,10 @@ public static function getInfo() {
    +    \Drupal::service('views.views_data')->clear();
    

    I believe the latter two should be changed to match the first rather than using static calls?

  9. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewExecutableTest.phpundefined
    @@ -25,7 +25,7 @@
     class ViewExecutableTest extends ViewUnitTestBase {
     
    -  public static $modules = array('system', 'node', 'comment', 'user', 'filter');
    +  public static $modules = array('system', 'node', 'user', 'filter', 'comment', 'entity', 'field', 'field_sql_storage', 'text');
    

    Interesting that this test has all these dependencies. Also, why are we adding text as a module here?

  10. +++ b/core/modules/views/tests/views_test_config/test_views/views.view.test_handler_relationships.ymlundefined
    @@ -12,16 +12,16 @@ display:
    -        cid:
    -          id: cid
    +        comment_cid:
    +          id: comment_cid
    ...
    -          field: nid
    -          relationship: cid
    +          field: node
    +          relationship: comment_cid
    

    So, yeah, I hadn't even considered that all the views integration needs to be rewritten. Need to review that specifically when I get up there, and check on the node access support in particular.

    Also, are all of these renames required?

  11. +++ b/core/themes/bartik/templates/node.html.twigundefined
    @@ -98,8 +91,7 @@
    -    {# We hide the comments and links now so that we can render them later. #}
    -    {% hide(content.comments) %}
    +    {# We hide links now so that we can render them later. #}
    
    @@ -110,6 +102,4 @@
    -  {{ content.comments }}
    

    So, comments are no longer rendered by the node template since they're now a field. That makes sense. The display is presumably controlled at the field level.

larowlan’s picture

So the path to post a comment on an entity is
comment/reply/entity_type/id/comment?

Is there any reason this couldn't be something saner like
path/to/entity/reply? (Totally could be a followup.)

the path is comment/reply/entity_type/entity_id/field_name
We could have more than one comment field on a given entity

Apart from that everything else making sense so far

Thanks and keep it coming

larowlan’s picture

Status: Needs work » Needs review
FileSize
387.8 KB

We're getting close to the 300 mark here too

andypost’s picture

FileSize
386.02 KB
8.13 KB

Fixed formatter and widget after #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets

@xjm comments already have upgrade path tests. So if we change it then needs extended test coverage - could be done after 1 july

1. drupalCreateNode() usage is cleaned up so passing default value (2 - open) makes no sense
function comment_add_default_comment_field($entity_type, $bundle, $field_name = 'comment', $default_value = COMMENT_OPEN)
2. tracker - there's a default view that works, so if VDC planing any changes - would be nice
3. todo needs fix
4. yes, needs follow-up
5. same as 1 - will check comments after merge HEAD, fixed
6. #241 entity could have more then one comment field attached so we need field-name here
7. :)
8. fixed and filed #2021831: Replace usage of Views:: in tests with its own services
9. because 'text' field needed, otherwise you'll get Uncaught PHP Exception Drupal\field\FieldException: "Attempt to create a field of unknown type text_long." - moved comment module back
10. views integration was changed a lot - thanx to @dawehner

xjm’s picture

@xjm comments already have upgrade path tests. So if we change it then needs extended test coverage - could be done after 1 july

What I'm saying is we shouldn't postpone any fiddling with the upgrade path to a followup. We should be 100% committed to the upgrade path we add with this patch. So as I said to @larowlan, I think #1920042: Upgrade path changes should either be wontfix or merged into the main patch. (Edit: We can discuss what to do in that issue.)

xjm’s picture

The fails are probably #1893772: Move entity-type specific storage logic into entity classes? (Wouldn't it be nice if that issue documented the API changes in the summary rather than making you read a 200K patch?) :P

andypost’s picture

FileSize
385.74 KB
679 bytes

Other comment tests fixed

larowlan’s picture

re-roll against HEAD

larowlan’s picture

larowlan’s picture

xjm’s picture

Out-of-scope obsevration: A lot of the generic Views tests seem to depend on node and comment. I wonder if we could have made this patch easier and smaler by cleaning that up. At this point it may be worth a followup.

xjm’s picture

Squeezing in a bit more review in DTW. Up through simpletest.

  1. +++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
    @@ -220,7 +220,7 @@ function drupalGetNodeByTitle($title, $reset = FALSE) {
    -   *   - comment: COMMENT_NODE_OPEN.
    +   *   - comment: COMMENT_OPEN.
    

    API change that should go in the change notice.

    Also, I guess we didn't remove this from drupalCreateNode() after all. Why are we deleting all the comment => 2 everywhere then?

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.phpundefined
    @@ -142,8 +142,6 @@ function testBreadCrumbs() {
    -    $this->assertBreadcrumb("admin/structure/types/manage/$type/comment/fields", $trail);
    -    $this->assertBreadcrumb("admin/structure/types/manage/$type/comment/display", $trail);
    
    +++ b/core/modules/system/lib/Drupal/system/Tests/Module/DependencyTest.phpundefined
    @@ -174,28 +174,39 @@ function testUninstallDependents() {
    -    // Disable forum and comment. Both should now be installed but disabled.
    +    // Disable forum, should now be installed but disabled.
         $edit = array('modules[Core][forum][enable]' => FALSE);
         $this->drupalPost('admin/modules', $edit, t('Save configuration'));
         $this->assertModules(array('forum'), FALSE);
    -    $edit = array('modules[Core][comment][enable]' => FALSE);
    -    $this->drupalPost('admin/modules', $edit, t('Save configuration'));
    -    $this->assertModules(array('comment'), FALSE);
     
    -    // Check that the taxonomy module cannot be uninstalled.
    -    $this->drupalGet('admin/modules/uninstall');
    -    $checkbox = $this->xpath('//input[@type="checkbox" and @disabled="disabled" and @name="uninstall[comment]"]');
    -    $this->assert(count($checkbox) == 1, 'Checkbox for uninstalling the comment module is disabled.');
    +    // Check that the comment module cannot be disabled.
    +    $checkbox = $this->xpath('//input[@type="checkbox" and @disabled="disabled" and @name="modules[Core][comment][enable]"]');
    +    $this->assert(count($checkbox) == 1, 'Checkbox for disabling the comment module is disabled.');
     
    -    // Uninstall the forum module, and check that taxonomy now can also be
    -    // uninstalled.
    +    // Uninstall the forum module, and check that taxonomy and comment now can
    +    // also be uninstalled.
         $edit = array('uninstall[forum]' => 'forum');
         $this->drupalPost('admin/modules/uninstall', $edit, t('Uninstall'));
         $this->drupalPost(NULL, NULL, t('Uninstall'));
         $this->assertText(t('The selected modules have been uninstalled.'), 'Modules status has been updated.');
    +    // Disable comment, should now be installed but disabled.
    +    $edit = array('modules[Core][comment][enable]' => FALSE);
    +    $this->drupalPost('admin/modules', $edit, t('Save configuration'));
    +    $this->assertModules(array('comment'), FALSE);
    +    // Now uninstall it.
         $edit = array('uninstall[comment]' => 'comment');
         $this->drupalPost('admin/modules/uninstall', $edit, t('Uninstall'));
         $this->drupalPost(NULL, NULL, t('Uninstall'));
         $this->assertText(t('The selected modules have been uninstalled.'), 'Modules status has been updated.');
    

    Another couple spots where doing some test decoupling ahead of the initial patch would have made this patch smaller. This test should be using test implementations, not actual core modules.

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/CommentUpgradePathTest.phpundefined
    @@ -0,0 +1,57 @@
    +    // Check that comments display on the node.
    +    $this->drupalGet('node/50');
    +    $this->assertText('Node title 50', 'Node 50 displayed after update.');
    +    $this->assertText('First test comment', 'Comment 1 displayed after update.');
    +    $this->assertText('Reply to first test comment', 'Comment 2 displayed after update.');
    +
    +    // Check one instance exists for each node type.
    +    $types = node_type_get_types();
    +    foreach (array_keys($types) as $type) {
    +      $instance = field_info_instance('node', 'comment_node_' . $type, $type);
    +      $this->assertTrue($instance, format_string('Comment field found for the %type node type', array(
    +        '%type' => $type
    +      )));
    

    I wonder if we should have tests for each comment setting (open, closed, read-only, etc.)? As well as anything else in the upgrade path.

  4. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_field_get_entity.ymlundefined
    @@ -38,9 +38,9 @@ display:
    -        nid:
    -          field: nid
    -          id: nid
    +        node:
    +          field: node
    +          id: node
    
    +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_handler_relationships.ymlundefined
    @@ -12,16 +12,16 @@ display:
    -        cid:
    -          id: cid
    +        comment_cid:
    +          id: comment_cid
               table: node
    -          field: cid
    +          field: comment_cid
    

    I wonder, should we file change notices against contrib Views for stuff like this?

andypost’s picture

FileSize
387.72 KB
616 bytes

New merge after #1777956: Provide a way to define default values for entity fields back to active #1919834: Field instance got no default value when created in field UI we still have no ability to save default value with field instance from UI

@xjm Thanx a lot for idea to extend upgrade path (3) - will try to implement

andypost’s picture

FileSize
390.07 KB
832 bytes
2.77 KB

Fix broken test - introduced in #2022769: Fix conversion of field_delete_field() in hook_uninstall(). Also fixed uninstall - see interdiff2.txt

Fix form_alter for field settings - cardinality container.
Clean-up useless settings for field - will see bot's reaction

mgifford’s picture

Wow, this is very, very impressive.

Initially I could see why this would be an interesting exercise in separating entities & nodes so that everything was just a bit more abstracted. But ya, I think this is going to change what is expected of a comment! Not just in Drupal either.

I don't know this issue well enough to RTBC it, but have to say it's looking really good to me. I was looking at accessibility issues to see if I could see anything that was added that would be a barrier to participation.

I didn't find any new problems with it. Some of the known ones are still there, but from an accessibility point of view, this is as good as Core is now.

Wow, just had another idea about what Core can do with this change embedded in it.

This has been a real marathon for folks involved. What needs to happen to make this RTBC?

andypost’s picture

@mgifford Thanx a lot for review! Patch was RTBC many times but for now we are in process of getting technical reviews about code.

xjm’s picture

@mgifford, I'm in the process of doing a line-by-line code review of the whole thing (which is why I have it assigned to me even though @andypost and @larowlan are the ones chasing HEAD). :) It takes awhile...

xjm’s picture

Up through node. There were actually way fewer changes in node itself than I expected. (Reminder, these are just notes for myself.)

  1. +++ b/core/modules/node/lib/Drupal/node/NodeStorageController.phpundefined
    @@ -97,16 +97,6 @@ protected function invokeHook($hook, EntityInterface $node) {
    -   * {@inheritdoc}
    -   */
    -  protected function mapToDataStorageRecord(EntityInterface $entity, $langcode) {
    -    // @todo Remove this once comment is a regular entity field.
    -    $record = parent::mapToDataStorageRecord($entity, $langcode);
    -    $record->comment = isset($record->comment) ? intval($record->comment) : 0;
    -    return $record;
    -  }
    

    Yay.

  2. +++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/Node.phpundefined
    @@ -133,17 +133,6 @@ class Node extends EntityNG implements NodeInterface {
       /**
    -   * The node comment status indicator.
    -   *
    -   * COMMENT_NODE_HIDDEN => no comments
    -   * COMMENT_NODE_CLOSED => comments are read-only
    -   * COMMENT_NODE_OPEN => open (read/write)
    -   *
    -   * @var \Drupal\Core\Entity\Field\FieldInterface
    -   */
    -  public $comment;
    

    API change that should go in the change notice.

  3. +++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/Node.phpundefined
    @@ -218,7 +207,6 @@ protected function init() {
         unset($this->status);
         unset($this->created);
         unset($this->changed);
    -    unset($this->comment);
         unset($this->promote);
         unset($this->sticky);
         unset($this->tnid);
    

    Out of scope: What the heckity heck is all that? BC stuff?

  4. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/wizard/Node.phpundefined
    @@ -116,16 +116,6 @@ protected function buildFormStyle(array &$form, array &$form_state, $type) {
    -        $style_form['row_options']['comments'] = array(
    -          '#type' => 'select',
    -          '#title_display' => 'invisible',
    -          '#title' => t('Should comments be displayed below each node'),
    -          '#options' => array(
    -            1 => t('with comments'),
    -            0 => t('without comments'),
    -          ),
    -          '#default_value' => 0,
    -        );
    
    @@ -222,13 +212,11 @@ protected  function display_options_row(&$display_options, $row_plugin, $row_opt
    -        $display_options['row']['options']['comments'] = !empty($row_options['comments']);
    ...
    -        $display_options['row']['options']['comments'] = !empty($row_options['comments']);
    
    +++ b/core/modules/node/lib/Drupal/node/Tests/Views/RowPluginTest.phpundefined
    @@ -153,25 +156,6 @@ public function testRowPlugin() {
    -    // Test with comments enabled.
    -    $view->rowPlugin->options['comments'] = TRUE;
    -    $output = $view->preview();
    -    $output = drupal_render($output);
    -    foreach ($this->nodes as $node) {
    -      foreach ($this->comments[$node->id()] as $comment) {
    -        $this->assertTrue(strpos($output, $comment->comment_body->value) !== FALSE, 'Make sure the comment appears in the output.');
    -      }
    -    }
    -
    -    // Test with comments disabled.
    -    $view->rowPlugin->options['comments'] = FALSE;
    -    $output = $view->preview();
    -    $output = drupal_render($output);
    -    foreach ($this->nodes as $node) {
    -      foreach ($this->comments[$node->id()] as $comment) {
    -        $this->assertFalse(strpos($output, $comment->comment_body->value) !== FALSE, 'Make sure the comment does not appears in the output when the comments option disabled.');
    -      }
    -    }
    

    Hmm, if we're removing this test coverage, I'd like to know why we had it in the first place. Look closer at this test and git blame.

    Edit: I seem to have found the answer in the patch. Views used to include an option to toggle the display of comments, or not, in the node row style plugin. Here, the option is removed, since technically a comment is now like any other field.

    I'm on the fence about this. It still seems like a feature people would want; comments are still kind of a special sort of field. I guess the way to achieve this now would be to create a different display mode for the bundle and use that for the row style? I'm actually not even sure if/how that would work cross-bundle (since Views just cares if they're nodes or not) or if it's actually possible. I'll test this when I get to testing the patch manually.

  5. +++ b/core/modules/node/lib/Drupal/node/Tests/Views/RowPluginTest.phpundefined
    @@ -90,15 +92,16 @@ protected function setUp() {
       public function drupalCreateComment(array $settings = array()) {
    

    Weird that this is defined in one views test... it's not even overriding a parent method or anything.

  6. +++ b/core/modules/node/lib/Drupal/node/Tests/Views/RowPluginTest.phpundefined
    @@ -90,15 +92,16 @@ protected function setUp() {
    -    $comment->save();
    +    $status = $comment->save();
         return $comment;
       }
    

    What is the point of this change? Merge artifact?

  7. +++ b/core/modules/node/node.api.phpundefined
    @@ -605,8 +605,8 @@ function hook_node_access($node, $op, $account, $langcode) {
    -  if (!isset($node->comment)) {
    -    $node->comment = variable_get("comment_$node->type", COMMENT_NODE_OPEN);
    +  if (!isset($node->widgets)) {
    +    $node->widgets = config("widgets_{$node->type}")->get('per_page');
    

    Out of scope: Single underscore separator? Is that a BC thing or by design?

  8. +++ b/core/modules/node/node.api.phpundefined
    @@ -986,9 +986,8 @@ function hook_node_type_insert($info) {
     function hook_node_type_update($info) {
       if (!empty($info->old_type) && $info->old_type != $info->type) {
    -    $setting = variable_get('comment_' . $info->old_type, COMMENT_NODE_OPEN);
    -    variable_del('comment_' . $info->old_type);
    -    variable_set('comment_' . $info->type, $setting);
    +    language_save_default_configuration('node', $info->type, language_get_default_configuration('node', $info->old_type));
    +    language_clear_default_configuration('node', $info->old_type);
       }
    

    Hunh? Self: look this up.

    Edit: This is a hook definition, so it's just a different example. Duh.

    I'm actually surprised there are only two node hook changes from the comment decoupling, and only in hook examples at that. I think the changes are incomplete, because there are a lot of other instances of the word "comment" in node.api.php, and some of them look suspect. (Keeping in mind I haven't actually reviewed comment itself yet.)

  9. +++ b/core/modules/node/node.installundefined
    @@ -275,12 +269,6 @@ function node_schema() {
    -      'comment' => array(
    -        'description' => 'Whether comments are allowed on this node (at the time of this revision): 0 = no, 1 = closed (read only), 2 = open (read/write).',
    -        'type' => 'int',
    -        'not null' => TRUE,
    -        'default' => 0,
    

    Hooray.

  10. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/CommentAttributesTest.phpundefined
    @@ -45,6 +49,10 @@ public function setUp() {
    +    $this->node = $this->drupalCreateNode(array('type' => 'article', 'promote' => 1));
    

    This should use a constant for promoted rather than a magic integer.

  11. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/CommentAttributesTest.phpundefined
    @@ -277,10 +285,11 @@ function _testBasicCommentRdfaMarkup($graph, $comment, $account = array()) {
       function saveComment($nid, $uid, $contact = NULL, $pid = 0) {
         $values = array(
    -      'nid' => $nid,
    +      'entity_id' => $nid,
    +      'entity_type' => 'node',
    +      'field_name' => 'comment',
           'uid' => $uid,
           'pid' => $pid,
    -      'node_type' => 'comment_node_article',
           'subject' => $this->randomName(),
           'comment_body' => $this->randomName(),
           'status' => 1,
    

    Interesting; why don't we need the bundle here? Or how the heck was the node_type ket used anyway since there are no changes in the rest of the method? I'll need to read the whole method in context, I guess.

    Out of scope: Why is the method name prefixed with an underscore? Either it's a test that someone turned off during development and never turned back on, or it's a very misleadingly named helper method. Note to self: check git blame on this.

  12. +++ b/core/modules/rdf/rdf.moduleundefined
    @@ -417,7 +417,9 @@ function rdf_comment_load($comments) {
    +    $entity = entity_load($comment->entity_type->value, $comment->entity_id->value);
    +    $uri = $entity->uri();
    +    $comment->rdf_data['entity_uri'] = url($uri['path']);
    

    Thank you, entity API.

  13. +++ b/core/modules/rdf/rdf.moduleundefined
    @@ -552,19 +554,23 @@ function rdf_preprocess_node(&$variables) {
    +  if (isset($variables['node']->comment_statistics) && !empty($variables['node']->rdf_mapping['comment_count']['predicates'])) {
    ...
    +    foreach ($variables['node']->comment_statistics as $field_name => $statistics) {
    

    Note to self: what is Node::comment_statistics and how is it different from Node::comment_count? Presumably as array or something since there can be multiple comment fields? Edit: Yes, that looks to be exactly the case.

  14. +++ b/core/modules/rdf/rdf.moduleundefined
    @@ -552,19 +554,23 @@ function rdf_preprocess_node(&$variables) {
    +      $count += $statistics->comment_count;
    +      // Annotates the 'x comments' link in teaser view.
    

    So, presumably, comment itself is also summing the total number of comments over all the comment fields for this link, and all we're doing in rdf.module is updating to match that.

  15. +++ b/core/modules/rdf/rdf.moduleundefined
    @@ -552,19 +554,23 @@ function rdf_preprocess_node(&$variables) {
    +      if (isset($variables['content']['links']['comment__' . $field_name]['#links']['comment-comments'])) {
    

    Hmm, what's with the comment prefix and double-underscore separator here? Hopefully I'll find that somewhere in comment.module.

  16. +++ b/core/modules/rdf/rdf.moduleundefined
    @@ -552,19 +554,23 @@ function rdf_preprocess_node(&$variables) {
    +        // According to RDFa parsing rule number 4, a new subject URI is created
    +        // from the href attribute if no rel/rev attribute is present. To get the
    +        // original node URL from the about attribute of the parent container we
    +        // set an empty rel attribute which triggers rule number 5. See
    +        // http://www.w3.org/TR/rdfa-syntax/#sec_5.5.
    
    @@ -753,15 +759,15 @@ function rdf_preprocess_comment(&$variables) {
    +    // The parent entity URI is precomputed as part of the rdf_data so that it can
    

    Nitpick: These comments needs to be rewrapped; the second line of the first and the second both look to be 82 characters or so.

  17. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchCommentTest.phpundefined
    @@ -109,7 +114,7 @@ function testSearchResultsComment() {
    -    $node->comment = 0;
    +    $node->set('comment', COMMENT_HIDDEN);
    

    Thank you, new field API. But why aren't we using this nice setter in the parts of the patch I've read already?

    Edit: Because the other things are drupalPost() and drupalCreateNode() and so on. Duh. :)

  18. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchCommentTest.phpundefined
    @@ -130,14 +135,17 @@ function testSearchResultsCommentAccess() {
    -    variable_set('comment_preview_article', DRUPAL_OPTIONAL);
    +    // Make preview optional.
    +    $instance = field_info_instance('node', 'comment', 'article');
    +    $instance['settings']['preview'] = DRUPAL_OPTIONAL;
    +    $instance->save();
    

    Wha, since when is this a field-level setting? Hunh.

  19. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchRankingTest.phpundefined
    @@ -29,6 +29,8 @@ public static function getInfo() {
    +    comment_add_default_comment_field('node', 'page');
    

    Note to self to look at this function when I get up there. Looks like Yet Another Procedural Wrapper™ at first glance...

  20. +++ b/core/modules/search/search.moduleundefined
    @@ -845,7 +853,9 @@ function search_comment_publish($comment) {
     function search_comment_unpublish($comment) {
       // Reindex the node when comments are unpublished.
    -  search_touch_node($comment->nid->target_id);
    +  if ($comment->entity_type->value == 'node') {
    +    search_touch_node($comment->entity_id->target_id);
    +  }
    

    Makes sense; search only cares about nodes by default, and previously it was implicit that these were nodes.

xjm’s picture

Done reading everything other than comment module itself. Yay.

  1. +++ b/core/modules/field/field.moduleundefined
    @@ -836,10 +837,14 @@ function field_view_field(EntityInterface $entity, $field_name, $display_options
    +      // @todo Drop once every entity got converted to entityNG.
    +      if ($entity instanceof EntityNG) {
    +        $items_multi = array($id => $entity_translation->get($field_name)->getValue());
           }
    +      else {
    +        $items_multi = array($id => empty($entity->{$field_name}[$display_langcode]) ? array() : $entity->{$field_name}[$display_langcode]);
    +      }
    

    The @todo warrants an explicit followup issue, or a note on whatever postponed "rip out EntityBC" may exist. And, let's put the issue link for that followup in the comment for future reference.

  2. +++ b/core/modules/file/lib/Drupal/file/Tests/FileFieldWidgetTest.phpundefined
    @@ -255,7 +258,8 @@ function testPrivateFileComment() {
    +    comment_add_default_comment_field('node', 'article');
    +    $this->drupalPost('admin/structure/comments/manage/comment/fields', $edit, t('Save'));
    

    This is minor, but it's sort of incongruous to be adding the comment field after defining $edit). Obviously it makes no difference technically, but it doesn't follow the mental model of the integration test. :)

  3. +++ b/core/modules/file/lib/Drupal/file/Tests/FileFieldWidgetTest.phpundefined
    @@ -264,12 +268,10 @@ function testPrivateFileComment() {
         // Create node.
    -    $text_file = $this->getTestFile('text');
    -    $edit = array(
    +    $node = $this->drupalCreateNode(array(
    +      'type' => 'article',
           'title' => $this->randomName(),
    -    );
    -    $this->drupalPost('node/add/article', $edit, t('Save and publish'));
    -    $node = $this->drupalGetNodeByTitle($edit['title']);
    +    ));
    

    Why are we un-defining $text_file? This seems to have nothing to do with the patch.

  4. +++ b/core/modules/forum/forum.installundefined
    @@ -16,6 +16,8 @@ function forum_install() {
    +  // Make sure the comment module is loaded.
    +  drupal_load('module', 'comment');
    

    Why do we need to do this in hook_install()? The field presumably isn't defined in comment.module anyway, and we're not using any API functions after this. And we're not doing the same thing for taxonomy, which is also a dependency.

    From the hook_install() docs:

    Note that since this function is called from a full bootstrap, all functions (including those in modules enabled by the current page request) are available when this hook is called. Use cases could be displaying a user message, or calling a module function necessary for initial setup, etc.

    Since forum (presumably) has comment as a dependency, comment should already be installed and available here.

  5. +++ b/core/modules/forum/forum.installundefined
    @@ -126,9 +132,20 @@ function forum_uninstall() {
    +  // Delete comment field, load comment in case it has been disabled.
    +  drupal_load('module', 'comment');
    

    Hmmm. This seems... not right. What do we need that's in comment.module? Also #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed.

  6. +++ b/core/modules/forum/forum.installundefined
    @@ -126,9 +132,20 @@ function forum_uninstall() {
    +  // Purge field data now to allow taxonomy and comment modules to be
    +  // uninstalled if these were the only fields remaining. We need to call
    +  // field_purge_batch at least twice as the instances won't be removed until
    +  // all data is gone, both cannot be removed in the same call.
    +  foreach (array('data', 'instances', 'fields') as $step) {
    +    field_purge_batch(10);
    

    Nitpick: field_purge_batch() should have parens after it.

    That said... I thought the field API took care of deleting field and instance data for us appropriately? Might want to check with yched or swentel about this.

    Also, should we be calling field_purge_batch() manually to begin with? (I understand this predates the patch...) Note to self: look this up.

  7. +++ b/core/modules/forum/forum.moduleundefined
    @@ -899,7 +906,7 @@ function forum_get_topics($tid, $sortby, $forum_per_page) {
    +      $topic->comment_mode = !empty($topic->comment_node_forum[Language::LANGCODE_NOT_SPECIFIED][0]['comment']) ? $topic->comment_node_forum[Language::LANGCODE_NOT_SPECIFIED][0]['comment'] : COMMENT_HIDDEN;
    

    Oy. We can't use a getter or something here?

xjm’s picture

Up through UserCommentTest, wherein I got a little concerned about the testing situation:

  1. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +/**
    + * Tests basic comment functionality against a user entity.
    + */
    +class CommentUserTest extends WebTestBase {
    

    Excellent, good to see this new test coverage for the new functionality. However, I don't see tests specifically for other entity types? I think it would be better and more maintainable to have a test based on the test entity.

  2. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +  /**
    +   * Modules to enable.
    +   *
    +   * @var array
    +   */
    +  public static $modules = array('comment', 'user');
    

    Out of scope: $modules doesn't seem to be documented on WebTestBase. Prolly we could @inheritdoc all these in all the tests everywhere if it were...

  3. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +  /**
    +   * Provides test information.
    +   */
    +  public static function getInfo() {
    

    Our standards specify that getInfo() should not have a docblock. (Yeah, it's dumb and inconsistent.) See https://drupal.org/node/325974.

  4. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +  /**
    +   * Checks current page for specified comment.
    +   *
    +   * @param \Drupal\comment\Plugin\Core\Entity\Comment $comment
    +   *   The comment object.
    +   * @param boolean $reply
    +   *   Boolean indicating whether the comment is a reply to another comment.
    +   *
    +   * @return boolean
    +   *   Boolean indicating whether the comment was found.
    +   */
    +  function commentExists(Comment $comment = NULL, $reply = FALSE) {
    +    if ($comment) {
    +      $regex = '/' . ($reply ? '<div class="indented">(.*?)' : '');
    +      $regex .= '<a id="comment-' . $comment->id() . '"(.*?)'; // Comment anchor.
    +      $regex .= $comment->subject->value . '(.*?)'; // Match subject.
    +      $regex .= $comment->comment_body->value . '(.*?)'; // Match comment.
    +      $regex .= '/s';
    +
    +      return (boolean)preg_match($regex, $this->drupalGetContent());
    +    }
    +    else {
    +      return FALSE;
    +    }
    +  }
    

    Augh, I recognize this. This is the method from hell I spent so much time with in #1811016: [meta] Decouple tests from Node module and #1812034: Only use standard profile where necessary in comment.module tests (10% bot speed-up). Is this test copied wholesale from there? If so, disregard my following several remarks about this test, but also, we should definitely consider writing better, decoupled tests, as per #1847540: [META] Clean up comment module tests and decouple from node (which I unfortunately postponed over 6 months ago on this patch; I wish I'd worked on that then because it would have made this patch a lot smaller).

  5. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +  /**
    +   * Deletes a comment.
    +   *
    +   * @param \Drupal\comment\Plugin\Core\Entity\Comment $comment
    +   *   Comment to delete.
    +   */
    +  function deleteComment(Comment $comment) {
    +    $this->drupalPost('comment/' . $comment->id() . '/delete', array(), t('Delete'));
    +    $this->assertText(t('The comment and all its replies have been deleted.'), 'Comment deleted.');
    

    Yeah, this is definitely based on the other comment tests. Which are REALLY slow, because they do so much through the internal browser that should just be done with API calls.

  6. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +  /**
    +   * Performs the specified operation on the specified comment.
    +   *
    +   * @param object $comment
    +   *   Comment to perform operation on.
    +   * @param string $operation
    +   *   Operation to perform.
    +   * @param boolean $aproval
    +   *   Operation is found on approval page.
    +   */
    +  function performCommentOperation($comment, $operation, $approval = FALSE) {
    +    $edit = array();
    +    $edit['operation'] = $operation;
    +    $edit['comments[' . $comment->id() . ']'] = TRUE;
    +    $this->drupalPost('admin/content/comment' . ($approval ? '/approval' : ''), $edit, t('Update'));
    +
    +    if ($operation == 'delete') {
    +      $this->drupalPost(NULL, array(), t('Delete comments'));
    +      $this->assertRaw(format_plural(1, 'Deleted 1 comment.', 'Deleted @count comments.'), format_string('Operation "@operation" was performed on comment.', array('@operation' => $operation)));
    +    }
    +    else {
    +      $this->assertText(t('The update has been performed.'), format_string('Operation "@operation" was performed on comment.', array('@operation' => $operation)));
    +    }
    

    Isn't this somewhat redundant with the deleteComment() method?

  7. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +   * Gets the comment ID for an unapproved comment.
    +   *
    +   * @param string $subject
    ...
    +  function getUnapprovedComment($subject) {
    

    Nitpick: Maybe this should be getUnapprovedCommentBySubject()?

  8. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +   *   Comment id.
    

    Nitpick: ID, not id. Also, articles. It should be "The comment ID."

  9. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +  /**
    +   * Tests anonymous comment functionality.
    +   */
    +  function testCommentUser() {
    

    So, this docblock says it tests anonymous comment functionality, but the method name is more generic, and the first several assertions just seem to test CRUD functionality for comments on users. As an administrator. I'd suggest splitting those off into a separate test method.

    The rest of the test tries several different combinations of user permissions, and variously checks (or not) whether:

    • The comments are displayed.
    • The links to log in or post comments are displayed.
    • The comment submission form is displayed.

    Seems like it would be a good idea to test those things more consistently and maybe refactor the test a little so it's easier to follow.

  10. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +    // Unpublish comment.
    ...
    +    // Publish comment.
    ...
    +    // Delete comment.
    

    Nitpicks: "Unpublish the comment", "Publish the comment", "Delete the comment".

  11. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +    $this->performCommentOperation($comment1, 'unpublish');
    +
    +    $this->drupalGet('admin/content/comment/approval');
    +    $this->assertRaw('comments[' . $comment1->id() . ']', 'Comment was unpublished.');
    

    The extra blank line in the middle of this (and the similar checks below) can probably be removed for better readability.

  12. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +    // Check comment was found.
    

    Nitpick: "Check that the comment was found."

  13. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +    // Reset.
    +    user_role_change_permissions(DRUPAL_ANONYMOUS_RID, array(
    +      'access comments' => FALSE,
    +      'post comments' => FALSE,
    +      'skip comment approval' => FALSE,
    +      'access user profiles' => TRUE,
    +    ));
    

    The inline comment here is kind of useless and also misleading. Maybe "Deny anonymous users access to comments"?

  14. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +    // Reset.
    +    user_role_change_permissions(DRUPAL_ANONYMOUS_RID, array(
    +      'access comments' => FALSE,
    +      'post comments' => FALSE,
    +      'skip comment approval' => FALSE,
    +      'access user profiles' => TRUE,
    +    ));
    ...
    +    // Attempt to view comments while disallowed.
    +    // NOTE: if authenticated user has permission to post comments, then a
    +    // "Login or register to post comments" type link may be shown.
    

    Nitpick: This comment is a little terse and awkward. First of all, it's preferred to use complete imperative sentences, including articles like "a" and "the". Also, "while disallowed" is vague and unclear. The word "type" is also unnecessary and a little confusing, because it sounds like we're talking about a thing called a "type link".

    Finally, the second sentence probably belongs above the relevant assertions, as it seems sort of out of the blue here. And, I'm on the fence about whether we should be including test for this here (see below).

  15. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +    $this->assertNoPattern('@<h2[^>]*>Comments</h2>@', 'Comments were not displayed.');
    ...
    +    $this->assertNoPattern('@<h2[^>]*>Comments</h2>@', 'Comments were not displayed.');
    

    This seems like a weak assertion to me, since it relies on the specific template (and furthermore one that I think has a bug in the current patch, though not for users).

  16. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +    // Attempt to view user-comment form while disallowed.
    

    Nitpick: user-comment should not be hyphenated. "User" is modifying "comment form"; "user-comment" is not a thing. That makes it sound like there's a mutant hybrid user-comment entity. Also, "while disallowed" again.

  17. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +    $this->drupalGet('comment/reply/user/' . $this->web_user->id() . '/comment');
    +    $this->assertText('You are not authorized to post comments', 'Error attempting to post comment.');
    +    $this->assertNoFieldByName('subject', '', 'Subject field not found.');
    +    $langcode = Language::LANGCODE_NOT_SPECIFIED;
    +    $this->assertNoFieldByName("comment_body[$langcode][0][value]", '', 'Comment field not found.');
    

    Either this set should also assert that the "login/register" links are found, or that should not be tested with the next set. (See comment below.)

  18. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +    user_role_change_permissions(DRUPAL_ANONYMOUS_RID, array(
    +      'access comments' => TRUE,
    +      'post comments' => FALSE,
    +      'access user profiles' => TRUE,
    +      'skip comment approval' => FALSE,
    ...
    +    $this->drupalGet('user/' . $this->web_user->id());
    +    $this->assertPattern('@<h2[^>]*>Comments</h2>@', 'Comments were displayed.');
    +    $this->assertLink('Log in', 0, 'Link to log in was found.');
    +    $this->assertLink('register', 0, 'Link to register was found.');
    

    Here, we're testing the combination where users can view, but not post comments. In that case, there seems to be an assertion missing--namely, that the comment form is not available.

    Also, I'm on the fence about testing for the "log in/register" links here. That's also dependent on authenticated users' permissions IIRC, and should presumably be tested elsewhere.

  19. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +    // Ensure the page cache is flushed.
    +    drupal_flush_all_caches();
    

    Youch. This is a pretty expensive thing to do. Why is this necessary, and is there something we can do differently or a more targeted cache clear we can do to avoid needing this?

  20. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +    user_role_change_permissions(DRUPAL_ANONYMOUS_RID, array(
    +      'access comments' => FALSE,
    +      'post comments' => TRUE,
    +      'skip comment approval' => TRUE,
    +      'access user profiles' => TRUE,
    +    ));
    

    Here, we're testing the combination of anonymous users being able to post, but not view comments, presumably to ensure that the one doesn't grant access to the other. Let's add a comment to that effect.

  21. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUserTest.phpundefined
    @@ -0,0 +1,341 @@
    +    $this->assertText('You are not authorized to view comments', 'Error attempting to post reply.');
    

    Nitpick: The assertion message here is both confusing and unnecessary. I'd just remove it; the default assertText() message is more specific and informative.

  22. +++ b/core/modules/comment/lib/Drupal/comment/Tests/Views/DefaultViewRecentComments.phpundefined
    

    Out of scope: This test class name should end in Test.

  23. +++ b/core/modules/comment/lib/Drupal/comment/Tests/Views/DefaultViewRecentComments.phpundefined
    @@ -73,13 +73,22 @@ public function setUp() {
    +    \Drupal::service('views.views_data')->clear();
    

    This should use $this->container.

  24. +++ b/core/modules/comment/lib/Drupal/comment/Tests/Views/DefaultViewRecentComments.phpundefined
    @@ -73,13 +73,22 @@ public function setUp() {
    +      // Stagger the comments so the timestamp sorting works.
    +      $comment->created->value = REQUEST_TIME - $i;
    

    I don't quite understand this comment, at least not with the patch context. We're hacking the created time so that each subsequent comment is older than the previous one? I don't get it.

  25. +++ b/core/modules/comment/lib/Drupal/comment/Type/CommentItem.phpundefined
    @@ -0,0 +1,41 @@
    +
    +  /**
    +   * Property definitions of the contained properties.
    +   *
    +   * @see CommentItem::getPropertyDefinitions()
    +   *
    +   * @var array
    +   */
    +  static $propertyDefinitions;
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getPropertyDefinitions() {
    +
    +    if (!isset(static::$propertyDefinitions)) {
    +      static::$propertyDefinitions['status'] = array(
    +        'type' => 'integer',
    +        'label' => t('Comment status value'),
    +        'settings' => array('default_value' => COMMENT_OPEN),
    +      );
    +    }
    +    return static::$propertyDefinitions;
    +  }
    +
    

    The docs here are pretty weak. $propertyDefinitions could use a lot more explanation, and I don't think @inheritdoc is sufficient for the mthod. The docs it's inheriting from ComplexDataInterface::getPropertyDefinitions() would be:

      /**                                                                           
       * Gets an array of property definitions of contained properties.             
       *                                                                            
       * @param array $definition                                                   
       *   The definition of the container's property, e.g. the definition of an    
       *   entity reference property.                                               
       *                                                                            
       * @return array                                                              
       *   An array of property definitions of contained properties, keyed by       
       *   property name.                                                           
       */

    But, we're also setting a default here, initializing $propertyDefinitions['status'] to be COMMENT_OPEN.

    Also, is this the core of the patch? If it is, it needs WAY more docs in addition to this. If not (I haven't read but half the patch yet), then it should probably have an @see to wherever the "this is how comments work" docs are.

  26. +++ b/core/modules/comment/templates/comment-wrapper.html.twigundefined
    @@ -36,7 +36,7 @@
    -  {% if comments and node.type != 'forum' %}
    +  {% if comments and (entity.entityType != 'node' or entity.bundle != 'forum') %}
         {{ title_prefix }}
         <h2 class="title">{{ 'Comments'|t }}</h2>
         {{ title_suffix }}
    

    This logic is changing. Shouldn't it be "and" rather than "or"? I'll double-check the whole template to see what this conditional is actually doing.

  27. +++ b/core/modules/comment/templates/comment.html.twigundefined
    @@ -28,7 +28,8 @@
      *   through CSS. The default values can be one or more of the following:
      *   - comment: The current template type; e.g., 'theming hook'.
      *   - by-anonymous: Comment by an unregistered user.
    - *   - by-node-author: Comment by the author of the parent node.
    + *   - by-{entity-type}-author: Comment by the author of the parent entity,
    + *     eg. by-node-author.
      *   - preview: When previewing a new or edited comment.
      *   The following applies only to viewers who are registered users:
      *   - unpublished: An unpublished comment visible only to administrators.
    @@ -56,7 +57,7 @@
    
    @@ -56,7 +57,7 @@
      *
      * These two variables are provided for context:
      * - comment: Full comment object.
    - * - node: Node entity the comments are attached to.
    + * - entity: Entity the comments are attached to.
      *
      * @see template_preprocess_comment()
    

    So, I need to double check in template_preprocess_comment() that these are correct.

xjm’s picture

Re: the tests, just looked again at #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest. That'd make comment tests betterer.

xjm’s picture

Re: getPropertyDefinitions(), @andypost referred me to #1777956: Provide a way to define default values for entity fields but I'm not quite sure to what end.

xjm’s picture

Okay this is whatever I wrote last night but did not save.

  1. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTestBase.phpundefined
    @@ -267,7 +270,9 @@ function setCommentsPerPage($number) {
    +    $instance = field_info_instance('node', 'comment', 'article');
    +    $instance['settings'][$name] = $value;
    +    $instance->save();
    

    Should we be using $this->container->get(field.info) here instead of field_info_instance()?

  2. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTranslationUITest.phpundefined
    @@ -49,6 +49,16 @@ function setUp() {
    +    // Add a comment field to the article content type.
    +    comment_add_default_comment_field('node', 'article', 'comment_article');
    +    // Add another comment field with new bundle to page content type.
    +    comment_add_default_comment_field('node', 'page');
    +    // Mark this bundle as translatable.
    +    translation_entity_set_config('comment', 'comment_article', 'enabled', TRUE);
    +    // Refresh entity info.
    +    entity_info_cache_clear();
    +    // Flush the permissions after adding the translatable comment bundle.
    +    $this->checkPermissions(array(), TRUE);
    

    ...Ah, so this explains createEntity() a little. There's two comment fields--a translatable one on articles, and a non-translatable one on pages. And the createEntity() method does some magic string matching to create an entity of one bundle or the other.

    This is kind of weird. IMO it'd be better to use test bundles and a test entity type rather than articles and pages... also, setUp() seems to create the article content type, but not the page content type, so how are we using it?

    The weird here partially predates the comment changes, I think, but now it's weirder.

  3. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTranslationUITest.phpundefined
    @@ -71,14 +81,26 @@ function setupTestFields() {
       /**
        * Overrides \Drupal\translation_entity\Tests\EntityTranslationUITest::createEntity().
        */
    -  protected function createEntity($values, $langcode, $node_bundle = NULL) {
    -    if (!isset($node_bundle)) {
    -      $node_bundle = $this->nodeBundle;
    +  protected function createEntity($values, $langcode, $bundle_name = 'comment_article') {
    +    if ($bundle_name == 'comment_article') {
    +      $node_type = 'article';
    +      $field_name = $bundle_name;
         }
    -    $node = $this->drupalCreateNode(array('type' => $node_bundle));
    -    $values['nid'] = $node->nid;
    +    else {
    +      $node_type = 'page';
    +      $field_name = 'comment';
    +    }
    +    $node = $this->drupalCreateNode(array(
    +      'type' => $node_type,
    +      $field_name => array(
    +        array('status' => COMMENT_OPEN)
    +      ),
    +    ));
    +    $values['entity_id'] = $node->nid;
    +    $values['entity_type'] = 'node';
    +    $values['field_name'] = $bundle_name;
         $values['uid'] = $node->uid;
    -    return parent::createEntity($values, $langcode);
    +    return parent::createEntity($values, $langcode, $bundle_name);
       }
    
    @@ -128,8 +150,8 @@ function testTranslateLinkCommentAdminPage() {
    -    $cid_translatable = $this->createEntity(array(), $this->langcodes[0], $this->nodeBundle);
    -    $cid_untranslatable = $this->createEntity(array(), $this->langcodes[0], 'page');
    +    $cid_translatable = $this->createEntity(array(), $this->langcodes[0]);
    +    $cid_untranslatable = $this->createEntity(array(), $this->langcodes[0], 'comment');
    

    This method is... kind of weird. I don't quite get the changes, probably because I don't get the method to begin with. I'll have to look at the whole test and the parent to sort it.

xjm’s picture

So, per @berdir, #1969728: Implement Field API "field types" as TypedData Plugins will make CommentItem include the whole field definition, rather than just this one isolated method. We'll want to convert this if that goes in first, and at that point we can also document CommentItem thoroughly.

xjm’s picture

Reviewed the rest of the test changes. Yeagh.

  1. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentAdminTest.phpundefined
    @@ -129,4 +143,25 @@ function testApprovalNodeInterface() {
    +  /**
    +   * Tests comment bundle admin.
    +   */
    +  public function testCommentAdmin() {
    +    // Login.
    +    $this->drupalLogin($this->admin_user);
    +    // Browse to comment bundle overview.
    +    $this->drupalGet('admin/structure/comments');
    +    $this->assertResponse(200);
    +    // Make sure titles visible.
    +    $this->assertText('Field name');
    +    $this->assertText('Used in');
    +    // Manage fields.
    +    $this->clickLink('manage fields');
    +    $this->assertResponse(200);
    +    // Make sure comment_body field is shown.
    +    $this->assertText('comment_body');
    +    // Rest from here on in is field_ui.
    +  }
    

    Note to self: Re-review this test after testing the patch manually.

  2. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentFieldsTest.phpundefined
    @@ -34,15 +34,13 @@ function testCommentDefaultFields() {
    +    $instance = field_info_instance('comment', 'comment_body', 'comment');
    

    Ditto re: FieldInfo.

  3. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentFieldsTest.phpundefined
    @@ -68,26 +67,19 @@ function testCommentEnable() {
    +    // Make sure that comment module could not be disabled.
    +    $this->drupalGet('admin/modules');
    +    $this->assertText('Required by: Drupal (Field type(s) in use - see Field list)');
    

    What? Why? As far as I can tell reading CommentTestBase and CommentFieldsTest, neither this method nor CommentTestBase::setUp() actually create any comment data, so why should disabling the module be disallowed? Does this happen for other field types provided by modules even if they don't have data yet?

  4. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentInterfaceTest.phpundefined
    @@ -88,23 +88,40 @@ function testCommentInterface() {
    +    // Deliberately use the wrong url to test
    +    // \Drupal\comment\Controller\CommentController::redirectNode().
    +    $this->drupalGet('comment/' . $this->node->nid . '/reply');
    +    // Verify we were correctly redirected.
    +    $this->assertUrl(url('comment/reply/node/' . $this->node->nid . '/comment', array('absolute' => TRUE)));
    +    $this->drupalGet('comment/reply/node/' . $this->node->nid . '/comment/' . $comment->id());
    

    Ah, I am glad we added this; that would be a lot of broken links. +1.

  5. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentInterfaceTest.phpundefined
    @@ -88,23 +88,40 @@ function testCommentInterface() {
    -    // Second reply to comment #3 creating comment #4.
    -    $this->drupalGet('comment/reply/' . $this->node->nid . '/' . $comment->id());
    -    $this->assertText($subject_text, 'Individual comment-reply subject found.');
    -    $this->assertText($comment_text, 'Individual comment-reply body found.');
    +    // Second reply to comment #2 creating comment #4.
    +    $this->drupalGet('comment/reply/node/' . $this->node->nid . '/comment/' . $comment->id());
    +    $this->assertText($comment->subject->value, 'Individual comment-reply subject found.');
    +    $this->assertText($comment->comment_body->value, 'Individual comment-reply body found.');
    

    Mmm, why is the numbering changing here? I'll need to read this whole test in context too.

  6. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentInterfaceTest.phpundefined
    @@ -115,34 +132,34 @@ function testCommentInterface() {
    -    $this->drupalGet('node/' . $this->node->nid, array('query' => array('page' => 1)));
    +    $this->drupalGet('node/' . $this->node->nid, array('query' => array('page' => 2)));
    

    And why is the pager changing here?

  7. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentLinksTest.phpundefined
    @@ -67,7 +67,7 @@ function testCommentLinks() {
           // @todo Complete test coverage for:
    -      //'comments'        => array(COMMENT_NODE_OPEN, COMMENT_NODE_CLOSED, COMMENT_NODE_HIDDEN),
    +      //'comments'        => array(COMMENT_OPEN, COMMENT_CLOSED, COMMENT_HIDDEN),
    

    Heh. I asked for upgrade path tests for this, but it sounds like we don't have complete coverage for it, period?

  8. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentLinksTest.phpundefined
    @@ -167,10 +167,10 @@ function setEnvironment(array $info) {
    +    if ($this->node->comment[Language::LANGCODE_NOT_SPECIFIED][0]['status'] != $info['comments']) {
    +      $this->node->comment[Language::LANGCODE_NOT_SPECIFIED][0]['status'] = $info['comments'];
    

    Again, is it possible to use methods here?

  9. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentNewIndicatorTest.phpundefined
    @@ -38,7 +38,6 @@ public function testCommentNewCommentsIndicator() {
    -    $this->node = $this->drupalCreateNode(array('type' => 'article', 'promote' => 1, 'comment' => COMMENT_NODE_OPEN));
    

    So, previously, this was just overriding the similar node that's created in CommentTestBase, with the only difference that it set the open comment status explicitly.

  10. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTestBase.phpundefined
    @@ -196,7 +199,7 @@ function deleteComment(Comment $comment) {
    -    $this->setCommentSettings('comment_subject_field', ($enabled ? '1' : '0'), 'Comment subject ' . ($enabled ? 'enabled' : 'disabled') . '.');
    +    $this->setCommentSettings('subject', ($enabled ? '1' : '0'), 'Comment subject ' . ($enabled ? 'enabled' : 'disabled') . '.');
    
    @@ -219,7 +222,7 @@ function setCommentPreview($mode) {
    -    $this->setCommentSettings('comment_preview', $mode, format_string('Comment preview @mode_text.', array('@mode_text' => $mode_text)));
    +    $this->setCommentSettings('preview', $mode, format_string('Comment preview @mode_text.', array('@mode_text' => $mode_text)));
    
    @@ -230,7 +233,7 @@ function setCommentPreview($mode) {
    -    $this->setCommentSettings('comment_form_location', ($enabled ? COMMENT_FORM_BELOW : COMMENT_FORM_SEPARATE_PAGE), 'Comment controls ' . ($enabled ? 'enabled' : 'disabled') . '.');
    +    $this->setCommentSettings('form_location', ($enabled ? COMMENT_FORM_BELOW : COMMENT_FORM_SEPARATE_PAGE), 'Comment controls ' . ($enabled ? 'enabled' : 'disabled') . '.');
    
    @@ -243,7 +246,7 @@ function setCommentForm($enabled) {
    -    $this->setCommentSettings('comment_anonymous', $level, format_string('Anonymous commenting set to level @level.', array('@level' => $level)));
    +    $this->setCommentSettings('anonymous', $level, format_string('Anonymous commenting set to level @level.', array('@level' => $level)));
    
    @@ -253,7 +256,7 @@ function setCommentAnonymous($level) {
    -    $this->setCommentSettings('comment_default_per_page', $number, format_string('Number of comments per page set to @number.', array('@number' => $number)));
    +    $this->setCommentSettings('per_page', $number, format_string('Number of comments per page set to @number.', array('@number' => $number)));
    

    So, looks like some key name changes here for the comment settings now that it's a field. We'll want to make that clear in the change notice.

xjm’s picture

  1. +++ b/core/modules/comment/lib/Drupal/comment/Routing/RouteSubscriber.phpundefined
    @@ -0,0 +1,61 @@
    + * Subscriber for Comment routes.
    

    Nitpick: This should start with a verb. ("Defines a...") http://drupal.org/node/1354#functions

  2. +++ b/core/modules/comment/lib/Drupal/comment/Routing/RouteSubscriber.phpundefined
    @@ -0,0 +1,61 @@
    +class RouteSubscriber implements EventSubscriberInterface {
    ...
    +  public function routes(RouteBuildEvent $event) {
    +    $collection = $event->getRouteCollection();
    +    if ($this->moduleHandler->moduleExists('node')) {
    +      $route = new Route(
    +        "/comment/{node}/reply",
    +        array('_controller' => 'Drupal\comment\Controller\CommentController::redirectNode'),
    +        array('_entity_access' => 'node.view')
    +      );
    +      $collection->add('comment_node_redirect', $route);
    +    }
    

    Soo I just lost the comment I spent 20 minutes writing about this class. Uh. I think it said something like:

    I had @berdir explain the purpose of this class to me, which appears to be to conditionally add the legacy node redirection only when the Node module is enabled. (Presumably it's necessary to add it only conditionally because of the {node} upcasting?)

    We should explain this somewhere on the class. I'd suggest adding some inline comments to routes(), and also maybe a bit to the class docblock.

    Also, out of scope, but I feel like this RouteSubscriber pattern is used in several places (and per @berdir needs to be used anywhere where we have logic or foreaches or whatnot in hook_menu() implementations) but it isn't really self-explanatory nor codified nor documented anywhere. I think we maybe want a RouteSubscriberInterface and/or a RouteSubscriberBase and/or something along those lines to codify this pattern (e.g., getSubscribedEvents() seems to be the same for each of them, defining to the routes() or a similarly named callback.

    Edit: Plus, we should update the WSCCI conversion guide to cover this.

  3. +++ b/core/modules/comment/lib/Drupal/comment/Routing/RouteSubscriber.phpundefined
    @@ -0,0 +1,61 @@
    +   * The module handler service
    

    Nitpick: Should end with a period.

  4. +++ b/core/modules/comment/lib/Drupal/comment/Routing/RouteSubscriber.phpundefined
    @@ -0,0 +1,61 @@
    +   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
    +   *   The entity type manager.
    

    I don't think it's the entity type manager? Copypasta error?

  5. +++ b/core/modules/comment/lib/Drupal/comment/Routing/RouteSubscriber.phpundefined
    @@ -0,0 +1,61 @@
    +  /**
    +   * Adds routes for comment.
    +   */
    

    This docblock is missing an @param, and is also a little thin. E.g., most of the routes for comment are already defined statically in hook_menu()--here, we're fetching the existing ones adn then adding in an additional one if node is enabled. Correct?

xjm’s picture

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/Comment.phpundefined
diff --git a/core/modules/comment/lib/Drupal/comment/Plugin/views/field/NodeLink.php b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/EntityLink.php
rename from core/modules/comment/lib/Drupal/comment/Plugin/views/field/NodeLink.php
rename to core/modules/comment/lib/Drupal/comment/Plugin/views/field/EntityLink.php

Aha. So we're not removing the feature; we're just making it properly entity type agnostic. I think there was a removed test assertion somewhere earlier in my review that maybe should be converted to test the updated feature instead?

And now I've found CommentWidget! Hooray!

xjm’s picture

Reviewed the widget and formatter. Conceptually, they seem sound, but it took me awhile to sort out what they were doing, so I think the logic can be improved to make them more self-documenting, and a dash of better inline docs will help as well.

  1. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/formatter/CommentDefaultFormatter.phpundefined
    @@ -0,0 +1,87 @@
    + * Plugin implementation of the default comment formatter.
    
    +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/widget/CommentWidget.phpundefined
    @@ -0,0 +1,83 @@
    + * Plugin implementation of the default comment widget.
    

    Nitpick: This should start with the verb. Also, I don't think it's exactly correct to call it "the plugin implementation of the default comment thingy" because that implies there are other kinds of implementations of these thingies, other than plugins. Try simply:
    Provides a default comment widget/formatter.

  2. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/formatter/CommentDefaultFormatter.phpundefined
    @@ -0,0 +1,87 @@
    + *   id = "comment_default",
    
    +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/widget/CommentWidget.phpundefined
    @@ -0,0 +1,83 @@
    + *   id = "comment_default",
    

    Why comment_default? What's the "default" part for?

  3. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/formatter/CommentDefaultFormatter.phpundefined
    @@ -0,0 +1,87 @@
    + *   label = @Translation("Comment List"),
    

    Nitpick: I think per our standards this should be "Comment list" (sentence case).

  4. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/formatter/CommentDefaultFormatter.phpundefined
    @@ -0,0 +1,87 @@
    +    $commenting_status = _comment_get_default_status($items);
    

    Ew. This function doesn't seem to exist in D7, even. Are we adding it further up in the patch? Can we add it as a method on the field or something instead?

  5. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/formatter/CommentDefaultFormatter.phpundefined
    @@ -0,0 +1,87 @@
    +    if ($commenting_status != COMMENT_HIDDEN && empty($entity->in_preview)) {
    

    So, here, we're not displaying comments or the comment form if comments are hidden or if we're previewing the entity. As far as I can tell, if !empty($additions) below also only happens if this condition is met, so let's move that inside this to make it more self-documenting.

  6. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/formatter/CommentDefaultFormatter.phpundefined
    @@ -0,0 +1,87 @@
    +      // Unpublished comments are not included in
    +      // $entity->comment_statistics[$field_name]->comment_count, so show
    +      // comments unconditionally if the user is an administrator.
    

    I don't understand why P implies Q.

  7. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/formatter/CommentDefaultFormatter.phpundefined
    @@ -0,0 +1,87 @@
    +      if (((!empty($entity->comment_statistics[$field_name]->comment_count) && user_access('access comments')) || user_access('administer comments')) &&
    +      !empty($entity->content['#view_mode']) &&
    +      !in_array($entity->content['#view_mode'], array('search_result', 'search_index'))) {
    ...
    +        // Only show the add comment form if the user has permission and the
    +        // view mode is not search_result or search_index.
    +        if (user_access('post comments') && !empty($entity->content['#view_mode']) &&
    +          !in_array($entity->content['#view_mode'], array('search_result', 'search_index'))) {
    

    So this actually seems more like the place that we are about the search_result and search_index view modes... Aha, wait. Both this and the previous conditional are excluding those two view modes. So, rather than having to subsequent if statements that check the view modes are not one of these two, let's nest them both in a separate if checking the view modes. I think that can also go around the if (!empty($additions) below, because as far as I can tell it's only set when one of these two conditions are met. ...Actually, in fact, the return value is also empty if it's either of those view modes. So, we could either wrap everything inside the method in this conditional, or return early. (Returning early being controversial and all.)

    Also, what's with the && !empty($entity_content['#view_mode'])? Is that just to prevent a warning on the following in_array()? What if the view mode is empty; what does that even mean? Do we still not want to display the comments or the comment form when the view mode isn't set? The logic doesn't seem quite correct.

    Finally, for clarity, I would switch the comment around and say something like:

    Comments are added to the search results and search index by comment_node_update_index() instead of by this formatter, so don't return anything if the view mode is search_index or search_result.

  8. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/formatter/CommentDefaultFormatter.phpundefined
    @@ -0,0 +1,87 @@
    +        // Comment threads aren't added to search results/indexes using the
    +        // formatter, @see comment_node_update_index().
    

    I don't understand what this comment has to do with the lines that follow it. It seems to have ore to do with the conditional above? I think?

    Also, the @see won't be parsed like this AFAIK. It should be on its own separate line.

  9. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/formatter/CommentDefaultFormatter.phpundefined
    @@ -0,0 +1,87 @@
    +          $additions['comments'] = $build;
    ...
    +          $additions['comment_form'] = comment_add($entity, $field_name);
    ...
    +    if (!empty($additions)) {
    +      $elements[] = $additions + array(
    

    WTH are "additions" and when would they be set? I don't see it initialized anywhere other than these two lines that initialize the comments and the comment form.

    Also, using the static wrapper comment_add() seems less than ideal here, because it couples this to the entity manager (twice). Is that in scope? Otherwise, maybe a followup?

  10. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/formatter/CommentDefaultFormatter.phpundefined
    @@ -0,0 +1,87 @@
    +      // Append comment form if needed.
    +      if ($commenting_status == COMMENT_OPEN && $comment_settings['form_location'] == COMMENT_FORM_BELOW) {
    

    What does "if needed" mean? It looks like it's appending the comment form if comments are open and the form is set to display below the entity.

  11. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/formatter/CommentDefaultFormatter.phpundefined
    @@ -0,0 +1,87 @@
    +        '#theme' => 'comment_wrapper__' . $entity->entityType() . '__' . $entity->bundle() . '__' . $field_name,
    

    I hope this underscore-delimited magic is documented in our templates.

  12. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/widget/CommentWidget.phpundefined
    @@ -0,0 +1,83 @@
    +    // If used for the field settings form or the entity doesn't have any
    +    // comments, the "hidden" option makes no sense, so don't even bother
    +    // presenting it to the user.
    +    if (!empty($entity->field_ui_default_value) || empty($entity->comment_statistics[$field->getFieldName()]->comment_count)) {
    +      $element['status'][COMMENT_HIDDEN]['#access'] = FALSE;
    +      // Also adjust the description of the "closed" option.
    +      $element['status'][COMMENT_CLOSED]['#description'] = t('Users cannot post comments.');
    

    I don't quite understand this comment. Also, I'm on the fence as to whether it's good usability to disable the "hidden" option in this way. Is that the behavior in HEAD?

  13. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/widget/CommentWidget.phpundefined
    @@ -0,0 +1,83 @@
    +    // Integrate with advanced settings, if available.
    +    if (isset($form['advanced'])) {
    

    I don't understand from this comment or conditional the circumstances under which $form['advanced'] would or wouldn't be set. Let's improve the comment to clarify that.

  14. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/widget/CommentWidget.phpundefined
    @@ -0,0 +1,83 @@
    +        // Collapse details when value is the same as default for instance.
    

    Without articles, everything in English turns into a string of nouns, or are they verbs? I had to read this sentence several times befeore I made sense of it. Try:
    Collapse the advanced settings when they are the same as the defaults for the instance.

  15. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/widget/CommentWidget.phpundefined
    @@ -0,0 +1,83 @@
    +        '#weight' => 30,
    

    I don't like arbitrary weights, especially when there are no other weights specified in the widget form here. What's 30? Why 30?

xjm’s picture

  1. +++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.phpundefined
    @@ -37,9 +39,9 @@ public function form(array $form, array &$form_state) {
    +      $form['#action'] = url('comment/reply/' . $comment->entity_type->value . '/' . $comment->entity_id->value . '/' . $comment->field_name->value);
    

    I wonder, should this path come from a method rather than us concatenating it together?

  2. +++ b/core/modules/comment/lib/Drupal/comment/CommentRenderController.phpundefined
    @@ -20,7 +20,7 @@ class CommentRenderController extends EntityRenderController {
        * Overrides Drupal\Core\Entity\EntityRenderController::buildContent().
        *
        * In addition to modifying the content key on entities, this implementation
    -   * will also set the node key which all comments carry.
    +   * will also set the comment entity key which all comments carry.
        */
       public function buildContent(array $entities, array $displays, $view_mode, $langcode = NULL) {
    
    @@ -37,22 +37,27 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
    -        throw new \InvalidArgumentException(t('Invalid node for comment.'));
    +        throw new \InvalidArgumentException(t('Invalid entity for comment.'));
    

    I wonder, do we need an @throws in this docblock for InvalidArgumentException? I mean any function throws that exception. Or should we use a more specific exception?

  3. +++ b/core/modules/comment/lib/Drupal/comment/CommentRenderController.phpundefined
    @@ -37,22 +37,27 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
    +    // Load all entities of all comments at once.
    

    How about:
    Load all the entities that have comments attached.

  4. +++ b/core/modules/comment/lib/Drupal/comment/CommentRenderController.phpundefined
    @@ -37,22 +37,27 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
    +    // Load entities in bulk, this is more performant than using
    +    // $comment->entity_id->value as we can load them in bulk per-type.
    

    Comma splice. Just change the comma to a period and capitalize "This." :) Also, "per type" should not be hyphenated.

  5. +++ b/core/modules/comment/lib/Drupal/comment/CommentRenderController.phpundefined
    @@ -61,8 +66,9 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
    +          '#links' => comment_links($entity, $comment_entity, $entity->field_name->value),
    

    This should totally be a method. I guess this can be a followup.

  6. +++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.phpundefined
    @@ -81,21 +87,29 @@ public function updateNodeStatistics($nid) {
    +          // Use created date of entity or default to REQUEST_TIME if none
    +          // exists.
    

    "Use the created date of the entity if it's set, or default to REQUEST_TIME."

  7. +++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.phpundefined
    @@ -81,21 +87,29 @@ public function updateNodeStatistics($nid) {
    +          // @todo refactor when http://drupal.org/node/585838 lands.
    

    "Refactor" should be capitalized. :)

  8. +++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.phpundefined
    @@ -81,21 +87,29 @@ public function updateNodeStatistics($nid) {
    +          // Get uid from entity or default to logged in user if none exists.
    

    "Get the user ID from the entity if it's set, or default to the currently logged in user."

  9. +++ b/core/modules/comment/lib/Drupal/comment/CommentStorageControllerInterface.phpundefined
    @@ -51,19 +52,19 @@ public function getChildCids(array $comments);
    +   * - last_comment_timestamp: The timestamp of the last comment for this entity,
    +   *   or the entity created timestamp if no comments exist for the entity.
    

    Nitpick: this needs rewrapping; the first line is 81 characters.

  10. +++ b/core/modules/comment/lib/Drupal/comment/CommentStorageControllerInterface.phpundefined
    @@ -51,19 +52,19 @@ public function getChildCids(array $comments);
    -  public function updateNodeStatistics($nid);
    +  public function updateEntityStatistics(CommentInterface $comment);
    

    Hang on a sec. This typehints a comment object, not the entity to which it's attached. I guess that makes sense since the place I saw it was in Comment, where naturally $this and $entity would refer to the comment entity itself. My error there. :)

  11. +++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.phpundefined
    @@ -0,0 +1,200 @@
    + * Contains \Drupal\comment\Controller\AdminController
    ...
    +class AdminController implements ControllerInterface {
    

    Missing period. Also, AdminController is a very vague and confusing class name. https://drupal.org/node/608152#naming

    Also, missing docblock again. And let's make sure the docblock actually describes the purpose of the controller instead of just being a fill-in-the-blank stub.

  12. +++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.phpundefined
    @@ -0,0 +1,200 @@
    +   * Entity Manager service.
    ...
    +   * Module handler service.
    ...
    +   * Field info service.
    ...
    +   *   Entity manager service.
    ...
    +   *   Module Handler service.
    ...
    +   *   Field Info service.
    

    Articlesssssss. the the the the the :)

  13. +++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.phpundefined
    @@ -0,0 +1,200 @@
    +   * Constructs a CustomBlock object.
    

    What? No it doesn't.

  14. +++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.phpundefined
    @@ -0,0 +1,200 @@
    +   * Returns markup for the overview of comment bundles.
    

    This docblock would be better if it explained what the purpose of said overview was.

  15. +++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.phpundefined
    @@ -0,0 +1,200 @@
    +    $field_ui = $this->moduleHandler->moduleExists('field_ui');
    +    if ($field_ui) {
    ...
    +      if ($field_ui) {
    

    So this variable does not, in fact, contain the field UI. Let's call it something like $field_ui_enabled?

    Also, an inline comment would be helpful here, e.g. "Add a column for field UI operations if the Field UI module is enabled."

  16. +++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.phpundefined
    @@ -0,0 +1,200 @@
    +    // @todo remove when entity_get_bundles() is a method on the entity manager.
    ...
    +    // @todo remove when entity_get_bundles() is a method on the entity manager.
    

    "Remove" should be capitalized. Also, is there an existing issue for this? If so, let's add the link in the comment, and if not, let's file it.

  17. +++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.phpundefined
    @@ -0,0 +1,200 @@
    +    $fields = array_filter($this->fieldInfo->getFieldMap(), function ($value) {
    +      if ($value['type'] == 'comment') {
    +        return TRUE;
    +      }
    +    });
    

    Inline comment for this hunk would be great, e.g.:
    Fetch a list of all comment fields.

  18. +++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.phpundefined
    @@ -0,0 +1,200 @@
    +    foreach ($fields as $field_name => $field_info_map) {
    +      $field_info = $this->fieldInfo->getField($field_name);
    +      // Initialize the row.
    +      $rows[$field_name]['class'] = $field_info['locked'] ? array('field-disabled') : array('');
    +      $rows[$field_name]['data']['field_name']['data'] = $field_info['locked'] ? t('@field_name (Locked)', array('@field_name' => $field_name)) : $field_name;
    +
    +      $rows[$field_name]['data']['usage']['data'] = array(
    +        '#theme' => 'item_list',
    +        '#items' => array(),
    +      );
    +      foreach ($field_info['bundles'] as $entity_type => $field_bundles) {
    +        $bundles = array();
    +        foreach ($field_bundles as $bundle) {
    +          if (isset($entity_bundles[$entity_type][$bundle])) {
    +            // Add the current instance.
    +            if ($field_ui && ($path = $this->entityManager->getAdminPath($entity_type, $bundle))) {
    +              $bundles[] = l($entity_bundles[$entity_type][$bundle]['label'], $path . '/fields');
    +            }
    +            else {
    +              $bundles[] = $entity_bundles[$entity_type][$bundle]['label'];
    +            }
    +          }
    +        }
    +        // Format used entity bundles.
    +        $rows[$field_name]['data']['usage']['data']['#items'][] = t('@entity_type: !bundles', array(
    +          '@entity_type' => $entity_types[$entity_type]['label'],
    +          '!bundles' => implode(', ', $bundles),
    +        ));
    

    Better inline commenting would be better here as well so I don't have to parse all of this in my brain.

  19. +++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.phpundefined
    @@ -0,0 +1,200 @@
    +        // @todo Check proper permissions for operations.
    

    Uh. This seems like a pretty important @todo. I guess the result currently is that "Manage fields" or "Manage display" would be broken links if the user has permission to administer comments but not fields, which seems like a completely common permission set.

  20. +++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.phpundefined
    @@ -0,0 +1,200 @@
    +          'title' => t('manage fields'),
    ...
    +          'title' => t('manage display'),
    

    These should be sentence-cased (capital M).

  21. +++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.phpundefined
    @@ -0,0 +1,200 @@
    +   * Returns markup help text of comment bundle.
    ...
    +  public function bundleInfo($field_name) {
    

    Articles!
    Returns HTML help text for the comment bundle.

    But, actually, is that what it does? This doesn't look like "help text" and that's also not what the method name would indicate. Can we give the method a better name? How is it used; what are the callers? There are no other instances of bundleInfo() in current HEAD.

  22. +++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.phpundefined
    @@ -0,0 +1,200 @@
    +   * @return array
    +   *   Renderable array.
    

    A renderable array containing... (and describe what it contains).

  23. +++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.phpundefined
    @@ -0,0 +1,200 @@
    +    $field_info = $this->fieldInfo->getField($field_name);
    +    foreach ($field_info['bundles'] as $entity_type => $field_bundles) {
    +      $bundles = array();
    +      foreach ($field_bundles as $bundle) {
    +        if (isset($entity_bundles[$entity_type][$bundle])) {
    +          // Add the current instance.
    +          if ($field_ui && ($path = $this->entityManager->getAdminPath($entity_type, $bundle))) {
    +            $bundles[] = l($entity_bundles[$entity_type][$bundle]['label'], $path . '/fields');
    +          }
    +          else {
    +            $bundles[] = $entity_bundles[$entity_type][$bundle]['label'];
    +          }
    +        }
    +      }
    +      // Format used entity bundles.
    +      $build['usage']['#items'][] = t('@entity_type: !bundles', array(
    +        '@entity_type' => $entity_types[$entity_type]['label'],
    +        '!bundles' => implode(', ', $bundles),
    +      ));
    +    }
    

    This could use some inline comments to explain what the purpose of it all is. I looked at #1901110: Improve the UX for comment bundle pages and comment field settings but it's still not clear to me exactly what this method's purpose is.

  24. +++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
    @@ -0,0 +1,67 @@
    + * Contains \Drupal\comment\Controller\CommentController
    ...
    +class CommentController implements ControllerInterface {
    

    Missing period on the first line, and missing docblock for the second. Also, "CommentController" is a pretty nonspecific name and might be confused with controllers for the comment entity. Let's give the class a better name.

    The docblock here should also explain what the controller is for (apparently only redirecting legacy links). I'm still not sure what's providing the rest of the routes...

  25. +++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
    @@ -0,0 +1,67 @@
    +  /**
    +   * Constructs a CommentController object.
    +   *
    +   * @param \Drupal\field\FieldInfo $field_info
    +   *   Field Info service.
    +   */
    +  public function __construct(FieldInfo $field_info) {
    +    $this->fieldInfo = $field_info;
    +  }
    

    Good, we have FieldInfo injected here. That reinforces to me that we should do so elsewhere as well (as per my earlier reviews).

  26. +++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
    @@ -0,0 +1,67 @@
    +   * Redirects legacy node links to new path.
    

    Nitpick: add an article.
    Redirects legacy node links to the new path.

  27. +++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
    @@ -0,0 +1,67 @@
    +   * @param \Drupal\Core\Entity\EntityInterface $node
    +   *   The node which the comment is a reply to.
    ...
    +  public function redirectNode(EntityInterface $node) {
    

    So, I almost said "We missed replacing the word node with entity here," but of course in this case it is only a node, because these are legacy links we're redirecting. So:

    • Should we typehint NodeInterface instead?
    • For the parameter documentation, how about: The node object identified by the legacy URL.
  28. +++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
    @@ -0,0 +1,67 @@
    +    // First field will do.
    

    Let's flesh out this comment a little, shall we? How about:

    Legacy nodes only had a single comment field, so use the first comment field on the entity.

  29. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.phpundefined
    @@ -35,14 +35,16 @@
      *   entity_keys = {
      *     "id" = "cid",
    - *     "bundle" = "node_type",
    + *     "bundle" = "field_name",
      *     "label" = "subject",
      *     "uuid" = "uuid"
      *   },
    + *   bundle_keys = {
    + *     "bundle" = "field_name"
    + *   },
    

    This always confuses me, even though I worked on the docs for it, so I need to look this bit up on the... manager? Are the docs still on EntityManager or did we move them to EntityInterface finally? In either case, that's irrelevant here. Just a note for myself. :)

  30. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.phpundefined
    @@ -68,20 +70,34 @@ class Comment extends EntityNG implements CommentInterface {
    -   * The parent comment ID if this is a reply to a comment.
    +   * The entity ID to which this comment is attached.
    

    Nitpickiest of nitpicks: the comment is attached to an entity, not an entity ID. So, grammatically:
    The entity ID for the entity to which this comment is attached.

  31. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.phpundefined
    @@ -68,20 +70,34 @@ class Comment extends EntityNG implements CommentInterface {
    +  /**
    +   * The entity type to which this comment is attached.
    

    Comments aren't attached to entity types, either. ;)
    The entity type of the entity to which this comment is attached.

  32. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.phpundefined
    @@ -68,20 +70,34 @@ class Comment extends EntityNG implements CommentInterface {
    +   * The field to which this comment is attached.
    

    I guess the comment is actually attached to a field so I'll leave this one. ;)

  33. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.phpundefined
    @@ -68,20 +70,34 @@ class Comment extends EntityNG implements CommentInterface {
    +   * The parent comment ID if this is a reply to a comment.
    

    Maybe: "...if this is a reply to another comment." (I realize this line already existed.

  34. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.phpundefined
    @@ -68,20 +70,34 @@ class Comment extends EntityNG implements CommentInterface {
    +   * @todo: Rename to 'parent_id'.
    

    I realize this @todo was pre-existing, but is there an issue for it?

  35. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.phpundefined
    @@ -189,7 +198,8 @@ protected function init() {
         unset($this->cid);
         unset($this->uuid);
         unset($this->pid);
    -    unset($this->nid);
    +    unset($this->entity_id);
    +    unset($this->field_name);
         unset($this->subject);
         unset($this->uid);
         unset($this->name);
    @@ -200,7 +210,7 @@ protected function init() {
    
    @@ -200,7 +210,7 @@ protected function init() {
         unset($this->changed);
         unset($this->status);
         unset($this->thread);
    -    unset($this->node_type);
    +    unset($this->entity_type);
         unset($this->new);
    

    I need to check again why we're unsetting all this stuff.

  36. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.phpundefined
    @@ -214,26 +224,12 @@ public function id() {
    -    // Make sure we have a proper bundle name.
    -    if (!isset($this->node_type->value)) {
    -      $this->node_type->value = 'comment_node_' . $this->nid->entity->type;
    -    }
    
    @@ -315,8 +311,8 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
    -    // Update the {node_comment_statistics} table prior to executing the hook.
    -    $storage_controller->updateNodeStatistics($this->nid->target_id);
    +    // Update the {comment_entity_statistics} table prior to executing the hook.
    +    $storage_controller->updateEntityStatistics($this);
    
    @@ -340,7 +336,7 @@ public static function postDelete(EntityStorageControllerInterface $storage_cont
    -      $storage_controller->updateNodeStatistics($entity->nid->target_id);
    +      $storage_controller->updateEntityStatistics($entity);
    

    Looks like this first hunk is removed without replacement because we have the full entity object now. updateEntityStatistics() also apparently takes a whole entity now.

Done with comment/lib! Yayyyy. Now just some (a lot of) procedural comment module code to go. But hopefully a lot of deletions. ;)

xjm’s picture

#1969728: Implement Field API "field types" as TypedData Plugins landed so there's rerolly stuff to be done here.

xjm’s picture

I've now reviewed everything after comment.module, as well as (somewhat cursorily) reviewing the last bit of that file. I'll need to revisit it later because right now I can't stand to look at it anymore. :)

  1. +++ b/core/modules/comment/comment.moduleundefined
    @@ -269,80 +420,82 @@ function comment_menu() {
    -function _comment_body_field_create($info) {
    +function _comment_body_field_create($entity_type, $bundle, $field_name) {
    
    @@ -1867,3 +1890,166 @@ function comment_library_info() {
    +  _comment_body_field_create($entity_type, $bundle, $field_name);
    ...
    +    _comment_body_field_create($instance['entity_type'], $instance['bundle'], $instance['field_name']);
    

    So I'm thinking maybe we want a followup to revisit the way this works.

  2. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1347,8 +1322,12 @@ function comment_load($cid, $reset = FALSE) {
    + *   The entity type to count comments for.
    

    "Entity type of the entity to which the comments are attached." We don't want to count all comments on all users everywhere, for example.

  3. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1356,22 +1335,43 @@ function comment_load($cid, $reset = FALSE) {
    +          // @todo remove once http://drupal.org/node/1029708 lands.
    

    Remove should be capitalized.

  4. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1385,31 +1385,32 @@ function comment_num_new($nid, $timestamp = 0) {
    + *   Field instance as returned from field_info_instance.
    

    Nitpick: This should end with parens.

  5. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1431,26 +1432,29 @@ function comment_get_display_ordinal($cid, $node_type) {
    -function comment_get_display_page($cid, $node_type) {
    -  $ordinal = comment_get_display_ordinal($cid, $node_type);
    -  $comments_per_page = variable_get('comment_default_per_page_' . $node_type, 50);
    +function comment_get_display_page($cid, $instance) {
    +  $ordinal = comment_get_display_ordinal($cid, $instance);
    +  $comments_per_page = $instance['settings']['per_page'];
    

    So it used to be a variable and is now a per-instance setting? That's nice.

  6. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1431,26 +1432,29 @@ function comment_get_display_ordinal($cid, $node_type) {
    +  // @todo Drop the conversion after http://drupal.org/node/1818580 in.
    +  $entity = $entity->getBCEntity();
    

    Unless we're planning on putting comments on menu links or config entities, I think this probably could be done now? Or do we need to wait until the BC layer is removed entirely?

  7. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1480,13 +1484,20 @@ function comment_preview(Comment $comment) {
    +    // We temporarily disable rendering of this field to prevent infinite
    +    // loop.
    +    $original_value = $entity->{$comment->field_name->value};
    +    $entity->{$comment->field_name->value} = array(
    +      Language::LANGCODE_NOT_SPECIFIED => array(array('status' => COMMENT_HIDDEN)),
    +    );
    +    $build = entity_view($entity, 'full');
    +    $entity->{$comment->field_name->value} = $original_value;
    

    I don't actually understand how the lines following this comment are doing what the comment is saying. Same goes for wherever else I saw this bit. Do we need like a somethingorotherLoadUnchanged()? Thing? entity_load_unchanged() or whatever?

  8. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1510,10 +1521,13 @@ function comment_preprocess_block(&$variables) {
    + *   The comment to provide author.
    

    What does that even mean?

  9. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1841,8 +1863,9 @@ function comment_rdf_mapping() {
    +      // Check access to parent entity.
    +      return comment_reply_access($commented_entity);
    

    Does this have test coverage, particularly for non-node entity types?

  10. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1867,3 +1890,166 @@ function comment_library_info() {
    +/**
    + * Utility function to return an array of comment fields.
    + *
    + * @param string $entity_type
    + *   (optional) Specify a entity type if you want to just return fields which
    + *   are attached on a certain entity type. Defaults to NULL.
    + *
    + * @return array
    + *   An array of comment field map definitions, keyed by field name. Each value
    + *   is an array with two entries:
    + *   - type: The field type.
    + *   - bundles: The bundles in which the field appears, as an array with entity
    + *     types as keys and the array of bundle names as values.
    + *
    + * @see field_info_field_map().
    + */
    +function comment_get_comment_fields($entity_type = NULL) {
    +  return array_filter(field_info_field_map(), function ($value) use($entity_type) {
    +    if ($value['type']  == 'comment') {
    +      if (isset($entity_type)) {
    +        return isset($value['bundles'][$entity_type]);
    +      }
    +      return TRUE;
    +    }
    +  });
    

    Hmmm.

  11. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1867,3 +1890,166 @@ function comment_library_info() {
    +function comment_mark(CommentInterface $comment) {
    

    Is there any reason this can't be a method on Comment?

  12. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1867,3 +1890,166 @@ function comment_library_info() {
    +      // @todo - decide how to handle last viewed for other entities. For now
    +      // assume REQUEST_TIME.
    +      $cache[$cid] = REQUEST_TIME;
    

    Hm. So we're setting updated marks for comments on non-node entities based on whether it's been updated since the first time comment_mark() ran or since the cache was cleared? Do we update or clear the static cache for this function when we view the comment, or something? This is... weird. Bad. I think it would feel buggy.

    I'd almost prefer to just not do the new markers if the entity isn't in the history table, and add that as a followup.

  13. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1867,3 +1890,166 @@ function comment_library_info() {
    +/**
    + * Utility method to add the default comment field to an entity.
    + *
    + * Attaches a comment field named 'comment' to the given entity type and bundle.
    + * Largely replicates the default behaviour in Drupal 7 and earlier.
    + *
    + * @param string $entity_type
    + *   The entity type to attach the default comment field to.
    + * @param string $bundle
    + *   The bundle to attach the default comment field instance to.
    + * @param string $field_name
    + *   (optional) Field name to use for the comment field. Defaults to 'comment'.
    + * @param string $default_value
    + *   (optional) Default value, one of COMMENT_HIDDEN, COMMENT_OPEN,
    + *   COMMENT_CLOSED. Defaults to COMMENT_OPEN.
    + */
    +function comment_add_default_comment_field($entity_type, $bundle, $field_name = 'comment', $default_value = COMMENT_OPEN) {
    

    Hm. So... hmm. I need to look back at this function (not a method) and all is uses in the patch after I finish reading the whole thing.

  14. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1867,3 +1890,166 @@ function comment_library_info() {
    +function comment_field_create_instance($instance) {
    ...
    +function comment_field_delete_instance($instance) {
    

    I guess we probably need test coverage to make sure the things that the cleanup from these hook implementations actually happens?

  15. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1867,3 +1890,166 @@ function comment_library_info() {
    +    cache()->delete('comment_entity_info');
    ...
    +    cache()->delete('comment_entity_info');
    

    Should these be using (at least) Drupal::cache()? Should we actually be deleting stuff here, or just invalidating it?

  16. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1867,3 +1890,166 @@ function comment_library_info() {
    +/**
    + * Helper to get default value for field.
    + *
    + * @param array $values
    + *   The field values array or NULL for field create with field UI.
    + *
    + * @return int
    + *   The value when exists or COMMENT_OPEN.
    + *
    + * @todo Replace with field default value after http://drupal.org/node/1919834
    + */
    +function _comment_get_default_status($values) {
    +  // We only ever process first value if any.
    +  return isset($values[0]['status']) ? $values[0]['status'] : COMMENT_OPEN;
    

    Euh. I looked at #1919834: Field instance got no default value when created in field UI, but I'm still not convinced that this helper is that much better than just falling back to COMMENT_OPEN; it obfuscates more than it helps, I think.

  17. +++ b/core/modules/comment/comment.pages.incundefined
    @@ -9,46 +9,54 @@
    -function comment_reply(EntityInterface $node, $pid = NULL) {
    +function comment_reply(EntityInterface $entity, $field_name, $pid = NULL) {
    

    Mmm, so:
    #1978948: Convert comment_approve() to a Controller
    #1978958: Convert comment_reply() to a Controller
    (Also #1946348: Convert all of confirm_form() in comment.admin.inc to the new form interface and #1978904: Convert comment_admin() to a Controller.)

  18. +++ b/core/modules/comment/comment.pages.incundefined
    @@ -9,46 +9,54 @@
    +  drupal_set_breadcrumb(array(
    

    I guess it's out of scope for this patch to avoid making it even bigger, but drupal_set_breadcrumb() is deprecated.

  19. +++ b/core/modules/comment/comment.pages.incundefined
    @@ -60,40 +68,56 @@ function comment_reply(EntityInterface $node, $pid = NULL) {
    +      // We make sure the field value isn't set so we don't end up with a redirect loop.
    

    Nitpick: Needs to be wrapped.

  20. +++ b/core/modules/comment/comment.pages.incundefined
    @@ -60,40 +68,56 @@ function comment_reply(EntityInterface $node, $pid = NULL) {
    +      $bc_entity = $entity->getBCEntity();
    +      $original_value = $bc_entity->{$field_name};
    +      $bc_entity->{$field_name} = array(
    +        Language::LANGCODE_NOT_SPECIFIED => array(array('status' => COMMENT_HIDDEN)),
    +      );
    +      $build['comment_entity'] = entity_view($bc_entity, 'full');
    +      $bc_entity->{$field_name} = $original_value;
    

    Should we still be doing this?

  21. +++ b/core/modules/comment/comment.pages.incundefined
    @@ -60,40 +68,56 @@ function comment_reply(EntityInterface $node, $pid = NULL) {
    @@ -121,5 +145,11 @@ function comment_approve(Comment $comment) {
    
    @@ -121,5 +145,11 @@ function comment_approve(Comment $comment) {
    +  throw new NotFoundHttpException();
    

    This needs an @throws.

  22. +++ b/core/modules/comment/comment.routing.ymlundefined
    @@ -1,7 +1,18 @@
    +    _content: 'Drupal\comment\Controller\AdminController::overviewBundles'
    ...
    +    _content: 'Drupal\comment\Controller\AdminController::bundleInfo'
    

    Ah, so this is what those methods are for--they're page callbacks. Let's update the method documentation to clarify that.

  23. +++ b/core/modules/comment/comment.tokens.incundefined
    @@ -16,13 +16,14 @@ function comment_token_info() {
    +  // @todo make this work per field.
    

    "Make" should be capitalized. I think this is actually a feature request to add per-field comment counts. So, let's file a feature request for that.

  24. +++ b/core/modules/comment/comment.tokens.incundefined
    @@ -79,9 +80,15 @@ function comment_token_info() {
    +  // Provide for backwards compatability as no upgrade path available.
    
    @@ -191,17 +198,33 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
    +        case 'node':
    +          if ($comment->entity_type->value == 'node') {
    +            $entity = entity_load($comment->entity_type->value, $comment->entity_id->value);
    +            $title = $entity->label();
    +            $replacements[$original] = $sanitize ? filter_xss($title) : $title;
    +          }
    +          else {
    +            $replacements[$original] = NULL;
    +          }
    +          break;
    

    Edit: Ah, here we go. So let's reword this comment to:

    Support legacy comment node tokens, since tokes are embedded in user data and can't be upgraded directly.

    Then, let's add an inline comment to that effect for the second hunk, and also add an @todo and a 9.x issue to remove it in D9. :)

    Also, I can see supporting it comment_tokens(), but is there a way we can indicate that it's deprecated in comment_token_info()?

  25. +++ b/core/modules/comment/comment.tokens.incundefined
    @@ -79,9 +80,15 @@ function comment_token_info() {
    -    'description' => t("The node the comment was posted to."),
    +    'description' => t("The Node the comment was posted to."),
    

    This change is both out of scope and wrong.

  26. +++ b/core/modules/comment/comment.tokens.incundefined
    @@ -191,17 +198,33 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
    +    if ($entity_tokens = $token_service->findwithPrefix($tokens, 'entity')) {
    

    Pre-existing and out of scope, but is this really the correct case for this method name?

  27. +++ b/core/modules/comment/comment.views.incundefined
    @@ -300,16 +300,11 @@ function comment_views_data() {
    +    'title' => t('Entity id'),
    

    Entity ID. :)

xjm’s picture

I've kind of reached my tolerance for this patch for now, so I'll revisit it more another day. :)

andypost’s picture

Assigned: xjm » andypost

Currently only one way to fix default values #1919834: Field instance got no default value when created in field UI - comment_field_instance_presave() according #2013939: Remove hook_field_[CRUD]_() in favor of hook_ENTITY_TYPE_[CRUD]() calls
Assigning to me - I'm starting to re-implement comment field after #1969728: Implement Field API "field types" as TypedData Plugins
Also will try to address @xjm's review bits

andypost’s picture

FileSize
387.53 KB

Merged HEAD

larowlan’s picture

larowlan’s picture

andypost’s picture

Status: Needs review » Needs work

Here's a related core bug #2030913: Comment module WSOD due to old translation_entity module name used
EDIT Also working on conversion #2013939: Remove hook_field_[CRUD]_() in favor of hook_ENTITY_TYPE_[CRUD]() calls fields attached to comment entities are not deleted

andypost’s picture

Status: Needs work » Needs review
FileSize
388.38 KB
3.97 KB

Fix some bugs.

Also after removing a comment field from entity and visiting /admin/reports/fields there's still notices

Notice: Undefined index: field_comment1 in Drupal\field_ui\FieldListController->buildRow() (line 118 of core/modules/field_ui/lib/Drupal/field_ui/FieldListController.php).
Notice: Undefined index: field_comment2 in Drupal\field_ui\FieldListController->buildRow() (line 118 of core/modules/field_ui/lib/Drupal/field_ui/FieldListController.php).

comment1(2) was fields added and later removed

$ ll *comment1*
-rw-r--r-- 1 andypost www-data 242 июня  29 06:25 entity.display.comment.field_comment1.default.yml
-rw-r--r-- 1 andypost www-data 258 июня  29 06:25 entity.form_display.comment.field_comment1.default.yml
-rw-r--r-- 1 andypost www-data 348 июня  29 06:25 field.instance.comment.field_comment1.comment_body.yml
+++ b/core/modules/comment/comment.moduleundefined
@@ -2019,3 +2016,26 @@ function _comment_get_default_status($values) {
+function _comment_field_instance_settings_form_process($element, $form_state) {

I think we should drop fieldset wrapper around settings

andypost’s picture

Status: Needs review » Needs work
FileSize
388.89 KB

Another round on sandbox code, merged all tremendous changes, Asked about default value in #2031203-4: Configurable fields should use applyDefaultValue()

andypost’s picture

Status: Needs work » Needs review
FileSize
388.98 KB

A bit more fixes, primary questionable is #2031203-4: Configurable fields should use applyDefaultValue()

andypost’s picture

FileSize
389.87 KB
5.03 KB

Introduced _update_comment_get_comment_fields() to be used in updates
Replaced more deprecated calls to field-functions

andypost’s picture

FileSize
385.98 KB
3.42 KB

reverted changed test - see interdiff.txt
minimized noise in patch and more fixes for deprecated functions (in sandbox)

PS: need some sleep

Anonymous’s picture

Status: Needs review » Needs work

Was just reviewing this when I was helping in debugging the RDF test, and I saw this:

+++ b/core/modules/rdf/rdf.moduleundefined
@@ -288,21 +290,25 @@ function rdf_preprocess_node(&$variables) {
+    $count = 0;
+    foreach ($variables['node']->comment_statistics as $field_name => $statistics) {
+      $count += $statistics->comment_count;

I'm not sure whether this is intentional, but I'm not sure why we set $count to 0 and then add the actual count. We don't seem to manipulate $count elsewhere.

Anonymous’s picture

Actually, I'm just realizing that I probably don't understand how comment statistics works... so my last comment can probably be ignored.

larowlan’s picture

Opened https://docs.google.com/a/previousnext.com.au/spreadsheet/ccc?key=0AlTpa... to track all of the work identified by @xjm. Let me know if you want 'edit' access.

andypost’s picture

Status: Needs work » Needs review
FileSize
389.19 KB
andypost’s picture

FileSize
397.22 KB
6.88 KB

So here's a patch with fixes from #2031707: field_sync_field_status() does not enable fields defined without hook_field_info() and suggested hack from #2032369: _update_8003_field_create_field() relies on field schemas never changing and plugins being available
Also extended test for forum uninstall to make sure that comment fields are deleted

Added prepareCache() that should be fixed in #2032393: Limit APIs to FieldDefinitionInterface when FieldInstance isn't needed

andypost’s picture

FileSize
396.82 KB
6.06 KB

Another round, filed #2032699: Preserve taxonomy_forums field when uninstalling forum module because while we preserve node type there's no reason to drop fields from.

interdiff still not commited, but now we have tests for field and instance deletion (commited http://privatepaste.com/e92fb18bc3 ) so tests for forum are extended within interdiff

andypost’s picture

FileSize
397.78 KB
3.91 KB

Added CommentManager

andypost’s picture

FileSize
405.77 KB
3.91 KB

Another set of fixed tests

andypost’s picture

FileSize
404.11 KB

Revert changes and test current state

--- a/core/modules/comment/lib/Drupal/comment/Tests/CommentTestBase.php
+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTestBase.php
@@ -98,7 +98,7 @@ function postComment($node, $comment, $subject = '', $contact = NULL) {
     $edit = array();
     $edit['comment_body[' . $langcode . '][0][value]'] = $comment;
 
-    $instance = $this->container->get('field.info')->getInstance('node', $node->bundle(), 'comment');
+    $instance = $this->container->get('field.info')->getInstance('node', 'article', 'comment');
     $preview_mode = $instance->settings['preview'];
     $subject_mode = $instance->settings['subject'];
 
andypost’s picture

FileSize
404.25 KB
1.84 KB

Proper fix and rdf now works

andypost’s picture

FileSize
406.8 KB
3 KB

A bit more fixed tests

andypost’s picture

Status: Needs review » Needs work

In #317 I drop unused core/profiles/standard/config/rdf.mapping.comment.comment_node_page.yml

Currently we ship standard profile with image and tags fields that added with config yml files - so the question:

+++ b/core/profiles/standard/standard.installundefined
@@ -24,8 +24,8 @@ function standard_install() {
+  // Add comment field to article node type.
+  comment_add_default_comment_field('node', 'article', 'comment', COMMENT_OPEN);

Should we drop this in favour of config yml files?

Anonymous’s picture

I tried applying the patch against HEAD, but it doesn't work. I'm not sure which sandbox this is and which branch of the sandbox I should be using.

EDIT: nevermind, alexpott found it for me :)

Anonymous’s picture

Status: Needs work » Needs review
FileSize
1.58 KB
807.07 KB

In #317 I drop unused core/profiles/standard/config/rdf.mapping.comment.comment_node_page.yml

This is the right thing to do.

Also, andypost asked about a @todo in RDF module which referenced this issue. I have resolved that todo in this patch.

The patch is against HEAD because I don't know what branch you are diffing against. The interdiff shows the RDF specific addition.

larowlan’s picture

Status: Needs review » Needs work

thanks @linclark, will refactor this into the sandbox

larowlan’s picture

Note our breadcrumb builder has been rolled into #1978958: Convert comment_reply() to a Controller so we'll need a re-roll when that goes in but smaller patch size = win

larowlan’s picture

Merges head and attempts to fix the EnableDisable test

larowlan’s picture

Some $account->uid/$user->uid calls.
Expecting a few more, but lets see what the bot says.

larowlan’s picture

Fixes the failing tests locally.

larowlan’s picture

larowlan’s picture

dsnopek’s picture

Huzzah! Green! :-)

This patch is huge and the content is beyond my ability to review, but I did do some manual testing on Simplytest.me.

I tried:

  • Leaving a comment
  • Editing the comment field instance settings on "article" - and then leaving more comments
  • Creating a new content type with a new comment field (and entity) - and leaving some comments

Everything worked worked fine! I didn't encounter any problems with comments that don't already exist in the 8.x branch without this patch. :-)

micbar’s picture

Wow!
This is a great concept!

Im not able to review that huge patch, but i tested the functionality

I applied the patch to 8.x-dev. ->worked
i had to run update.php ->failed due to missing {node_type} table called in _update_7000_node_get_types() in comment_update_8006() #111715: Convert node/content types into configuration

added comment instance to page node type manually ->worked
posted comments on page node ->worked

created comment instance on user entity type -> worked
posted comment on a user profile ->worked

created comment instance on vocabulary "tags"-> worked
posted comment on a taxonomy term ->worked

dsnopek’s picture

@larowlan: Maybe it's time to take this patch back to the main issue for wider review and, hopefully, to be committed?

yched’s picture

-class CommentItem extends ConfigFieldItemBase {
+class CommentItem extends ConfigFieldItemBase implements PrepareCacheInterface {

 ...
 
+  /**
+   * {@inheritdoc}
+   */
+  public function prepareCache() {}

No need to implement PrepareCacheInterface if your prepareCache() method is empty. This has a non-minor CPU impact at "load on field cache miss" because this makes EntityNG create the Field/FieldItem objects just to call prepareCache().

-  public static function schema(Field $field) {
+  public static function schema(Field $field = NULL) {

Hackish. If comment_update_8006() needs to do some dirty tricks to work around comment.module being possibly disabled, then the dirty tricks should be kept in comment_update_8006().
If it needs a $field ConfigEntity to call schema($field), then it should build it itself ($field = new Field($field_config)).
However, I fail to see how setting the 'schema' property in the array passed to _update_8003_field_create_field() changes anything ?

larowlan’s picture

No need to implement PrepareCacheInterface if your prepareCache() method is empty.

Ok then that is a bug in HEAD see http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul... we call prepareCache regardless of whether the item implements the interface. Opened #2048833: Prepare cache called on field items without checking if PrepareCacheInterface is implemented

Hackish

Agreed, will sort during next re-roll.

larowlan’s picture

Addresses #337 comment 2
Rerolls against HEAD

larowlan’s picture

larowlan’s picture

larowlan’s picture

Status: Needs review » Needs work

working on the fails

larowlan’s picture

Failing tests pass locally, merges HEAD

larowlan’s picture

xjm’s picture

First reviewing the interdiffs, starting way back in #285. Editing the same comment as I go to save thread space and not lose any review.

  1. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.phpundefined
    @@ -0,0 +1,162 @@
    +class CommentItem extends ConfigFieldItemBase {
    

    Man, our terminology is so confusing... Not in scope. Just, man.

  2. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.phpundefined
    @@ -0,0 +1,162 @@
    +    // We always want the values saved so we can rely on them.
    

    I don't quite understand this comment?

  3. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1980,24 +1980,26 @@ function comment_add_default_comment_field($entity_type, $bundle, $field_name =
    +      // Set comments enabled by default when field created via UI.
    

    This needs to have a few more words to become an English sentence. ;)

  4. +++ b/core/modules/comment/comment.installundefined
    @@ -77,6 +76,35 @@ function comment_enable() {
    + * This function similar to comment_get_comment_fields() that cannot be used in
    + * maintenance mode because comment.module is disabled and it's fields inactive.
    

    Another comment that could use a little work. I'd say:

    comment_get_comment_fields() cannot be used in maintenance mode because comment is disabled and its fields are inactive.

    However, let's also explain why we need this safe-for-maintenance-mode function.

  5. +++ b/core/modules/comment/comment.installundefined
    @@ -77,6 +76,35 @@ function comment_enable() {
    +  return $fields;
    +}
    +
    +
    +/**
      * Implements hook_modules_enabled().
    

    Nit: Extra blank line.

  6. +++ b/core/modules/system/tests/upgrade/drupal-7.all-disabled.database.phpundefined
    @@ -26,3 +26,12 @@
    +// @todo Remove this after https://drupal.org/node/2032369
    +db_update('system')
    +  ->fields(array(
    +    'status' => 1,
    +  ))
    +  ->condition('type', 'module')
    +  ->condition('name', 'comment')
    +  ->execute();
    

    Issue is in; will double-check that this is removed in a later reroll.

  7. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentFieldsTest.phpundefined
    @@ -69,8 +69,16 @@ function testCommentEnable() {
    +    field_purge_batch(10);
    ...
    +    field_purge_batch(10);
    

    Why are we arbitrarily using a magic number 10 here? Maybe this is just moved code? Did I comment on this already elsewhere?

  8. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.phpundefined
    @@ -68,6 +68,10 @@ public static function schema(Field $field = NULL) {
    +        // Required when upgrading and comment module is disabled.
    +        // @todo Remove in D9.
    +        'indexes' => array(),
    +        'foreign keys' => array(),
    

    Why in D9? Can we clarify? File a 9.x issue? ;)

  9. +++ b/core/modules/forum/forum.installundefined
    @@ -106,7 +106,7 @@ function forum_enable() {
    -  if (!Drupal::service('field.info')->getInstance('node', 'forum', 'comment_node_forum')) {
    +  if (!field_read_field('comment_node_forum', array('include_inactive' => TRUE))) {
    

    Hm, why reverting this?

  10. +++ b/core/modules/comment/comment.moduleundefined
    @@ -1917,6 +1917,10 @@ function comment_add_default_comment_field($entity_type, $bundle, $field_name =
       // We only ever process first value if any.
    +  // @todo Remove when all entities are NG.
    +  if (is_object($values)) {
    +    $values = $values->getValue();
    +  }
    

    Followup issue?

  11. +++ b/core/modules/comment/comment.pages.incundefined
    @@ -93,13 +93,22 @@ function comment_reply(EntityInterface $entity, $field_name, $pid = NULL) {
    +      // @todo Remove when node is an NG entity.
    +      if ($entity->entityType() == 'node') {
    

    Post an explicit followup and link it on #1939994: Complete conversion of nodes to the new Entity Field API?

  12. +++ b/core/modules/file/lib/Drupal/file/Tests/FileFieldWidgetTest.phpundefined
    @@ -205,7 +205,7 @@ function testMultiValuedWidget() {
         // Grant the admin user required permissions.
    -    user_role_grant_permissions($this->admin_user->roles[1], array('administer node fields'));
    +    user_role_grant_permissions($this->admin_user->roles[0]->value, array('administer node fields'));
    

    Whoa, I don't get this change.

Edit final: Okay, done with interdiffs. Now onto the part of the patch I hadn't reviewed yet.

xjm’s picture

Down to the top of comment.module. I read, but did not take time to parse, the upgrade path. Some more comments explaining what each update hook is doing would help me review it. As would upgrade path tests that codify what it's supposed to do.

  1. +++ b/core/modules/comment/comment-entity-form.jsundefined
    @@ -0,0 +1,19 @@
    +/**
    + * @file
    + * Attaches comment behaviors to the entity form.
    + */
    +
    +(function ($) {
    +
    +"use strict";
    +
    +Drupal.behaviors.commentFieldsetSummaries = {
    +  attach: function (context) {
    +    var $context = $(context);
    +    $context.find('fieldset.comment-entity-settings-form').drupalSetSummary(function (context) {
    +      return Drupal.checkPlain($(context).find('.form-item-comment input:checked').next('label').text());
    +    });
    +  }
    
    +++ /dev/nullundefined
    @@ -1,40 +0,0 @@
    -/**
    - * @file
    - * Attaches comment behaviors to the node form.
    - */
    -
    -(function ($) {
    -
    -"use strict";
    -
    -Drupal.behaviors.commentDetailsSummaries = {
    -  attach: function (context) {
    -    var $context = $(context);
    -    $context.find('.comment-node-settings-form').drupalSetSummary(function (context) {
    -      return Drupal.checkPlain($(context).find('.form-item-comment input:checked').next('label').text());
    -    });
    -
    -    // Provide the summary for the node type form.
    -    $context.find('.comment-node-type-settings-form').drupalSetSummary(function(context) {
    -      var $context = $(context);
    -      var vals = [];
    -
    -      // Default comment setting.
    -      vals.push($context.find(".form-item-comment select option:selected").text());
    -
    -      // Threading.
    -      var threading = $(context).find(".form-item-comment-default-mode input:checked").next('label').text();
    -      if (threading) {
    -        vals.push(threading);
    -      }
    -
    -      // Comments per page.
    -      var number = $context.find(".form-item-comment-default-per-page select option:selected").val();
    -      vals.push(Drupal.t('@number comments per page', {'@number': number}));
    -
    -      return Drupal.checkPlain(vals.join(', '));
    -    });
    -  }
    

    Why is the new JS so much smaller than the old JS?

  2. +++ b/core/modules/comment/comment.admin.incundefined
    +++ b/core/modules/comment/comment.admin.incundefined
    @@ -83,37 +83,45 @@ function comment_admin_overview($form, &$form_state, $arg) {
    

    What's the status of this WSCCI conversion... it doesn't seem to be listed on #1971384: [META] Convert page callbacks to controllers?

    Also, #1978904: Convert comment_admin() to a Controller was closed as a dupe of #1986606: Convert the comments administration screen to a view... while that would make the access handling here at least less of a one-off, I'm not sure that was the correct decision. The router conversions are release-blocking, whereas Dries has stated several time that View conversions are not, and they need case-by-case approval after API freeze. (I'm of course 100% behind replacing it with a View if at all possible, but I have to point out that it's not a guarantee.)

    Neither of these things are part of this issue, but they definitely will affect this form and the patch. I'll follow up on those issues later.

  3. +++ b/core/modules/comment/comment.admin.incundefined
    @@ -83,37 +83,45 @@ function comment_admin_overview($form, &$form_state, $arg) {
    -  $query->join('node_field_data', 'n', 'n.nid = c.nid');
    -  $query->addTag('node_access');
    +  if (Drupal::moduleHandler()->moduleExists('node')) {
    +    // Special case to ensure node access works.
    +    $query->leftJoin('node_field_data', 'n', "n.nid = c.entity_id AND c.entity_type = 'node'");
    +    $query->addTag('node_access');
    +  }
    ...
    -    ->fields('c', array('cid', 'nid', 'subject', 'name', 'changed'))
    +    ->fields('c', array('cid', 'subject', 'name', 'changed', 'entity_id', 'entity_type', 'field_name'))
    

    So, at a glance, this implies to me the comment listing page now can list comments across all entity types, and that we need to respect access control.

    1. What about other entity types' access handling? We've added interfaces for access for other entity types and it seems like comments should never be available if the parent entity is not available. I don't think our bases are covered in that regard because this is just using a db_select().
    2. The changes to this interface page require some fairly heavy-duty new test coverage. I'm betting we might not even have existing test coverage that this page respects node access...
  4. +++ b/core/modules/comment/comment.api.phpundefined
    @@ -195,5 +199,36 @@ function hook_comment_delete(Drupal\comment\Comment $comment) {
    + * Controls access to entity comment forms.
    ...
    +function hook_comment_access(\Drupal\Core\Entity\EntityInterface $entity) {
    

    Just the form? Not created entities? If so the hook name is kind of unintuitive.

  5. +++ b/core/modules/comment/comment.api.phpundefined
    @@ -195,5 +199,36 @@ function hook_comment_delete(Drupal\comment\Comment $comment) {
    + * Used to control access to commenting on an entity where no
    + * {%entity_type}_access function exists for the given entity.
    

    Nit: There should probably be parens after {%entity_type}_access().

  6. +++ b/core/modules/comment/comment.api.phpundefined
    @@ -195,5 +199,36 @@ function hook_comment_delete(Drupal\comment\Comment $comment) {
    + * the logged in user has access to view an entity in order to reply to a
    

    Nit: logged-in should be hyphenated.

  7. +++ b/core/modules/comment/comment.api.phpundefined
    @@ -195,5 +199,36 @@ function hook_comment_delete(Drupal\comment\Comment $comment) {
    + * @return mixed
    + *   - COMMENT_ACCESS_DENY: if the operation is to be denied.
    + *   - FALSE: to not affect this operation at all.
    

    These return values are a little confusing; why isn't there a constant for COMMENT_ACCESS_IGNORE or something?

  8. +++ b/core/modules/comment/comment.api.phpundefined
    @@ -195,5 +199,36 @@ function hook_comment_delete(Drupal\comment\Comment $comment) {
    + * @todo replace this with entity access controls once generic access controller
    + *   lands.
    + *
    + * @see http://drupal.org/node/1696660
    

    This is in.

  9. +++ b/core/modules/comment/comment.api.phpundefined
    @@ -195,5 +199,36 @@ function hook_comment_delete(Drupal\comment\Comment $comment) {
    +  if ($entity->entityType() == 'comment') {
    +    return COMMENT_ACCESS_DENY;
    +  }
    

    This was kind of mind-bending as an example; maybe add an inline comment that says:

    Disallow commenting on comment entities themselves.

    Or something.

  10. +++ b/core/modules/comment/comment.api.phpundefined
    @@ -195,5 +199,36 @@ function hook_comment_delete(Drupal\comment\Comment $comment) {
    +  // Returning nothing from this function would have the same effect.
    

    This is a pretty useless comment; let's replace with:

    By default, do not alter access to the entity comment form.

  11. +++ b/core/modules/comment/comment.info.ymlundefined
    @@ -6,6 +6,5 @@ version: VERSION
     core: 8.x
     dependencies:
       - datetime
    -  - node
       - text
    

    WOOO YEAH \m/

  12. +++ b/core/modules/comment/comment.installundefined
    @@ -5,24 +5,20 @@
    +  // We cannot use CommentManager::getFields() because comment.module is
    +  // disabled and it's fields inactive.
    

    "...and its fields are inactive."

  13. +++ b/core/modules/comment/comment.installundefined
    @@ -5,24 +5,20 @@
    +  foreach (_update_comment_get_comment_fields() as $field_name => $field) {
    +    entity_invoke_bundle_hook('delete', 'comment', $field_name);
    
    @@ -33,47 +29,80 @@ function comment_uninstall() {
    +  $comment_fields = _update_comment_get_comment_fields();
    

    Okay, so this is one use of the update helper that has nothing to do with DB updates. Is there a better function name?

  14. +++ b/core/modules/comment/comment.installundefined
    @@ -33,47 +29,80 @@ function comment_uninstall() {
      * Implements hook_enable().
      */
     function comment_enable() {
    

    I'd like to see a second paragraph in the docblock here that explains in a sentence or two what we're doing, because the code that follows is dense.

  15. +++ b/core/modules/comment/comment.installundefined
    @@ -33,47 +29,80 @@ function comment_uninstall() {
    +          // Insert records into the comment_entity_statistics for entities that
    +          // are missing.
    

    comment_entity_statistics is a table name right? If so it should be in curlies. Also, the word "the" can be removed.

  16. +++ b/core/modules/comment/comment.installundefined
    @@ -33,47 +29,80 @@ function comment_uninstall() {
    +          if (!empty($schema[$table]['fields']['created'])) {
    +            $query->addField('e', 'created', 'last_comment_timestamp');
    +          }
    +          else {
    +            // No created field for this entity type, default to now.
    +            $query->addExpression(REQUEST_TIME, 'last_comment_timestamp');
    +          }
    +          if (!empty($schema[$table]['fields']['uid'])) {
    +            $query->addField('e', 'uid', 'last_comment_uid');
    +          }
    +          else {
    +            // No uid field for this entity type, default to anonymous.
    +            $query->addExpression(0, 'last_comment_uid');
    

    Let's move the inline comments for these two conditionals to above the if statements and adjust them accordingly; that will be easier to follow.

  17. +++ b/core/modules/comment/comment.installundefined
    @@ -33,47 +29,80 @@ function comment_uninstall() {
    +  // Set default value of comment.maintain_entity_statistics.
    +  Drupal::state()->set('comment.maintain_entity_statistics', TRUE);
    

    Kinda useless comment. Better would be something like:

    By default, maintain entity statistics for comments.

    And better yet would be a little more explanation of what that means and/or an @see.

  18. +++ b/core/modules/comment/comment.installundefined
    @@ -33,47 +29,80 @@ function comment_uninstall() {
    + * Returns comment fields.
    ...
    + * This function similar to CommentManager::getFields() that cannot be used in
    + * maintenance mode because comment.module is disabled and it's fields inactive.
    ...
    +function _update_comment_get_comment_fields() {
    

    Whatever I said above about these docs etc.

  19. +++ b/core/modules/comment/comment.installundefined
    @@ -33,47 +29,80 @@ function comment_uninstall() {
    +  $fields = entity_load_multiple_by_properties('field_entity', array(
    

    Yeah, the function name is misleading. This wouldn't be usable during the upgrade path because of the reliance on Entity API here.

  20. +++ b/core/modules/comment/comment.installundefined
    @@ -100,12 +129,26 @@ function comment_schema() {
    -        'description' => 'The {node}.nid to which this comment is a reply.',
    +        'description' => 'The entity_id to which this comment is a reply.',
    ...
    +        'description' => 'The entity_type to which this comment is a reply.',
    
    @@ -203,15 +255,29 @@ function comment_schema() {
    +        'description' => 'The entity_type to which this comment is a reply.',
    

    Grammar nitpick: Comments are not replies to entity IDs or entity types. Try:

    The entity_id of the entity to which this comment is a reply.

  21. +++ b/core/modules/comment/comment.installundefined
    @@ -100,12 +129,26 @@ function comment_schema() {
    +        'description' => 'The field_name through which this comment was added.',
    
    @@ -203,15 +255,29 @@ function comment_schema() {
    +        'description' => 'The field_name through which this comment was added.',
    

    This is a little confusing too. Does it mean: "The field_name of the field that was used to add this comment."?

  22. +++ b/core/modules/comment/comment.installundefined
    @@ -182,9 +225,22 @@ function comment_schema() {
    +        array('entity_type', 32),
    +        array('field_name', 32),
    ...
    +        array('entity_type', 32),
    +        array('field_name', 32),
    
    @@ -243,20 +309,16 @@ function comment_schema() {
    +    'primary key' => array('entity_id', array('entity_type', 32), array('field_name', 32)),
    

    Magic numbers. 32 seems arbitrary, especially with #1709960: declare a maximum length for entity and bundle machine names just hanging around open. Ideally we'd use a constant here (but not in the upgrade path; upgrade path needs a static with a comment referencing the constant).

  23. +++ b/core/modules/comment/comment.installundefined
    @@ -275,6 +337,16 @@ function comment_schema() {
    +  // Node comment status have been turned to fields after the fields and
    +  // instances are converted to ConfigEntities.
    

    I don't understand this comment. Maybe flesh it out a little more to explain what foo_update_N() and foo_update_M() do and why it matters?

  24. +++ b/core/modules/comment/comment.installundefined
    @@ -384,10 +456,331 @@ function comment_update_8003(&$sandbox) {
         'node_cron_comments_scale' => 'comment.node_comment_statistics_scale',
    +    'comment_maintain_node_statistics' => 'comment.maintain_entity_statistics',
    

    We're getting close to when these changes won't be allowed anymore (Beta 1), but not there yet.

  25. +++ b/core/modules/comment/comment.installundefined
    @@ -384,10 +456,331 @@ function comment_update_8003(&$sandbox) {
    + * Updates the {comment_node_statistics} and {comment} tables to new structure.
    

    Nitpick: We say "Update" instead of "Updates" because these docblocks are end-user-facing on update.php.

  26. +++ b/core/modules/comment/comment.installundefined
    @@ -384,10 +456,331 @@ function comment_update_8003(&$sandbox) {
    +function comment_update_8005(&$sandbox) {
    ...
    +function comment_update_8006(&$sandbox) {
    ...
    +function comment_update_8007(&$sandbox) {
    

    These are some seriously heavy-duty upgrade hooks. I hope we have some upgrade path tests?

  27. +++ b/core/modules/comment/comment.installundefined
    @@ -384,10 +456,331 @@ function comment_update_8003(&$sandbox) {
    +    while (!Drupal::config('field.field.' . $field['id'])->isNew()) {
    ...
    +    if (Drupal::config("field.instance.node.$node_type." . $field['id'])->isNew()) {
    

    #1856972: config() isn't safe to use during minor updates if there's a change to configuration storage

  28. +++ b/core/modules/comment/comment.installundefined
    @@ -384,10 +456,331 @@ function comment_update_8003(&$sandbox) {
    +    // Comment module may be disabled so we will need to inject the schema to
    +    // avoid attempting to create a Field of an unknown type.
    +    if (!Drupal::moduleHandler()->moduleExists('comment')) {
    

    Is this automatically doing the thing now where the module handler is the upgrade-path-safe one?

  29. +++ b/core/modules/comment/comment.installundefined
    @@ -384,10 +456,331 @@ function comment_update_8003(&$sandbox) {
    +        // COMMENT_MODE_THREADED
    +        'default_mode' => update_variable_get('comment_default_mode_' . $node_type, 1),
    +        'per_page' => update_variable_get('comment_default_per_page_' . $node_type, 50),
    

    +1, this is the correct way to specify these. :)

  30. +++ b/core/modules/comment/comment.installundefined
    @@ -384,10 +456,331 @@ function comment_update_8003(&$sandbox) {
    +      _update_8003_field_create_instance($field, $instance);
    

    Note to self: find this guy.

  31. +++ b/core/modules/comment/comment.installundefined
    @@ -384,10 +456,331 @@ function comment_update_8003(&$sandbox) {
    +
    +    // Clean up old variables.
    +    variable_del('comment_' . $node_type);
    +    variable_del('comment_default_mode_' . $node_type);
    +    variable_del('comment_default_per_page_' . $node_type);
    +    variable_del('comment_anonymous_' . $node_type);
    +    variable_del('comment_subject_field_' . $node_type);
    +    variable_del('comment_form_location_' . $node_type);
    +    variable_del('comment_preview_' . $node_type);
    

    I don't think we should be doing this... variable_del() will be removed before RC1 and the {variables} table will eventually be removed in #1860986: Drop left-over tables from 8.x.

  32. +++ b/core/modules/comment/comment.installundefined
    @@ -384,10 +456,331 @@ function comment_update_8003(&$sandbox) {
    + * Updates existing values.
    

    That's a tad vague. ;) What exisitng values does it update and how? Also, same nitpick about the silly s.

  33. +++ b/core/modules/comment/comment.installundefined
    @@ -384,10 +456,331 @@ function comment_update_8003(&$sandbox) {
    +/**
    + * Removes the existing fields.
    + */
    +function comment_update_8008(&$sandbox) {
    +  // Remove the {node}.comment field.
    +  db_drop_field('node', 'comment');
    +  // Remove the {node_revision}.comment field.
    +  db_drop_field('node_revision', 'comment');
    

    We've already migrated this data, right? Can we reference the hook that migrated it?

larowlan’s picture

Why are we arbitrarily using a magic number 10 here? Maybe this is just moved code? Did I comment on this already elsewhere?

Yeah 10 was pre-existing and is also used by taxonomy.

Hm, why reverting this?

FieldInfo doesn't consider fields from disabled modules, field_read_fields has a 'include inactive' parameter. We need to use field_read_fields in case the module is disabled.

Followup issue?

Post an explicit followup and link it on #1939994: [META] Complete conversion of nodes to the new Entity Field API?

I think it would make more sense to wait until we're at the 'ok lets commit this issue' stage - at which point we should grep the diff for new @todo's and make sure we have follow-ups. It is very likely, as with many other issues, that will be resolved before this is committed and so I think creating the follow-ups now might be premature.

Whoa, I don't get this change.

Converting users to NG changed the value of $user->roles, previously the 'authenticated user' was the first role and real role was the second, now authenticated is at the end of the array and we need the ->value because its now NG land.

The rest of the suggested improvements are valid observations.

xjm’s picture

FieldInfo doesn't consider fields from disabled modules, field_read_fields has a 'include inactive' parameter. We need to use field_read_fields in case the module is disabled.

Mmm, but #2018319: Remove field_read_field(s)() and field_read_instance(s) in favor of entity_load() and entity_load_multiple_by_properties().

larowlan’s picture

Why is the new JS so much smaller than the old JS?

Most of the javascript was dealing with setting the 'summary' on the vertical tabs at the node type edit form, which is gone in this patch.

Just the form? Not created entities? If so the hook name is kind of unintuitive.

These return values are a little confusing; why isn't there a constant for COMMENT_ACCESS_IGNORE or something?

This was kind of mind-bending as an example; maybe add an inline comment that says:

This is in.

This is a pretty useless comment; let's replace with:

Yeah this was all added before all entities had an access controller as a fallback. It might not be needed anymore. Added to the to-do list.

So, at a glance, this implies to me the comment listing page now can list comments across all entity types, and that we need to respect access control.

Agreed, this was on of our action points and will reflect so in the summary.

Is this automatically doing the thing now where the module handler is the upgrade-path-safe one?

Yes
Everything else makes sense, will add to the spreadsheet

Berdir’s picture

Focusing on NG stuff.

+++ b/core/modules/comment/comment.api.phpundefined
@@ -195,5 +199,36 @@ function hook_comment_delete(Drupal\comment\Comment $comment) {
+ * Used to control access to commenting on an entity where no
+ * {%entity_type}_access function exists for the given entity.
...
+function hook_comment_access(\Drupal\Core\Entity\EntityInterface $entity) {

Seems like this should be dropped now? Note that this hook will actually conflict with the generic hook_$entitytype_access() that is called by the default access controller.

+++ b/core/modules/comment/comment.moduleundefined
@@ -947,234 +957,175 @@ function comment_view_multiple($comments, $view_mode = 'full', $langcode = NULL)
+      // @todo Tidy when all entities are NG.
+     // Default to current user when entity does not have a uid property.
+      $last_comment_uid = $user->id();
+      if (!empty($entity->uid)) {
+        if (is_object($entity->uid)) {
+          $last_comment_uid = $entity->uid->value;
+        }
+        else {
+          $last_comment_uid = $entity->uid;
+        }
+      }
+      // Default to REQUEST_TIME when entity does not have a changed property.
+      $last_comment_timestamp = REQUEST_TIME;
+      if (!empty($entity->changed)) {
+        if (is_object($entity->changed)) {
+          $last_comment_timestamp = $entity->changed->value;
+        }
+        else {
+          $last_comment_timestamp = $entity->changed;
+        }

All entities are NG. Some however have the bc decorator on top of them. Which you can circumvent using get(). So try something like this:

if($entity->getProperyDefinition('uid')) {
  $entity->get('uid')->value;
}

That said, we're trying to move to interfaces wherever we can, Comment just got converted too early to get the interface-treatment ;)

So, what I would suggest here is an EntityAuthorInterface (with getAuthor()/getAuthorId() methods) and a EntityChangedInterface (with getChangedTime() or something like that.

Then you can be sure that those properties don't just exist, they actually have the meaning you expect. And it works when properties are named differently. Files for example have a getChangedTime() method but the property is called timestamp.

+++ b/core/modules/comment/comment.moduleundefined
@@ -1446,13 +1445,29 @@ function comment_preview(Comment $comment) {
+    $entity->{$comment->field_name->value} = array(
+      Language::LANGCODE_NOT_SPECIFIED => array(array('status' => COMMENT_HIDDEN)),

Just COMMENT_HIDDEN, no language/array-y stuff.

If this doesn't break any tests, then you likely have no test coverage for whatever this is doing ;)

+++ b/core/modules/comment/comment.moduleundefined
@@ -1610,8 +1628,10 @@ function template_preprocess_comment(&$variables) {
+    // @todo Drop instanceof condition after http://drupal.org/node/1818580 in.
+    if (($comment_entity instanceof EntityNG && !empty($comment_entity->uid->target_id) && $comment->uid->target_id == $comment_entity->uid->target_id) ||
+        (!empty($comment_entity->uid) && $comment->uid->target_id == $comment_entity->uid)) {
+      $variables['attributes']['class'][] = 'by-' . $comment_entity->entityType() . '-author';

Same as above.

+++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.phpundefined
@@ -81,21 +87,30 @@ public function updateNodeStatistics($nid) {
           'comment_count' => 0,
-          'last_comment_timestamp' => $node->created,
+          // Use the created date of the entity if it's set,
+          // or default to REQUEST_TIME.
+          'last_comment_timestamp' => isset($entity->created->value) ? $entity->created->value : REQUEST_TIME,
           'last_comment_name' => '',
-          'last_comment_uid' => $node->uid,
+          // @todo Refactor when http://drupal.org/node/585838 lands.
+          // Get the user ID from the entity if it's set, or default to the
+          // currently logged in user.
+          'last_comment_uid' => isset($entity->uid->target_id) ? $entity->uid->target_id : $user->id(),

And same here. Not sure why this uses created and not changed, like the others?

About the roles stuff, both the old and the new code is hackish. We have an getRoles() method now. I think it would be useful to have a flag in there to prevent the authenticated/authorized pseudo rid being returned.

xjm’s picture

Issue tags: +Needs tests

That said, we're trying to move to interfaces wherever we can, Comment just got converted too early to get the interface-treatment ;)

Is there an open issue around this?

So, what I would suggest here is an EntityAuthorInterface (with getAuthor()/getAuthorId() methods) and a EntityChangedInterface (with getChangedTime() or something like that.

Then you can be sure that those properties don't just exist, they actually have the meaning you expect. And it works when properties are named differently. Files for example have a getChangedTime() method but the property is called timestamp.

If we want to do that, let's do it in a separate patch.

Also sounds like we need some tests based on #353.

Berdir’s picture

We have #2028025: Expand CommentInterface to provide methods, which adds an interface for the existing comment entity but would probably conflict quite a bit with this.

xjm’s picture

Converting users to NG changed the value of $user->roles, previously the 'authenticated user' was the first role and real role was the second, now authenticated is at the end of the array and we need the ->value because its now NG land.

That sounds extremely fragile. Can we change this test to explicitly pick the role rather than relying on the order of entries in a non-associative array? If it's moved/updating code, we can do it in a separate patch now. If it's a new test, it should happen in this patch.

Berdir’s picture

alexpott’s picture

+++ b/core/modules/comment/comment.installundefined
@@ -384,10 +456,331 @@ function comment_update_8003(&$sandbox) {
+function comment_update_8005(&$sandbox) {

I'm concerned that on large sites (d.o for example) this update function might prove completely unrunable as some of the indexes we're creating will be massive.

+++ b/core/modules/comment/comment.installundefined
@@ -384,10 +456,331 @@ function comment_update_8003(&$sandbox) {
+    variable_del('comment_' . $node_type);
+    variable_del('comment_default_mode_' . $node_type);
+    variable_del('comment_default_per_page_' . $node_type);
+    variable_del('comment_anonymous_' . $node_type);
+    variable_del('comment_subject_field_' . $node_type);
+    variable_del('comment_form_location_' . $node_type);
+    variable_del('comment_preview_' . $node_type);

Should use update_variable_del()

+++ b/core/modules/comment/comment.moduleundefined
@@ -245,20 +289,29 @@ function comment_menu() {
+function comment_entity_reply_load($entity_id, $args) {
+  list(, ,$entity_type, $entity_id, $field_name) = $args;
+  return entity_load($entity_type, $entity_id);
+}

This looks dangerous as we're passing entity_id in and the overwriting with the list and using it in an entity load. Let's change the $entity_id in the list to $comment_entity_id or something like that.

+++ b/core/modules/comment/comment.moduleundefined
@@ -272,94 +325,53 @@ function comment_count_unpublished() {
+  // We can't know how to control access to this entity, invoke
+  // hook_comment_access and if no other modules object, grant access.
+  $access = Drupal::moduleHandler()->invokeAll('comment_access', $entity);

This new hook appears to be untested.

+++ b/core/modules/comment/comment.moduleundefined
@@ -272,94 +325,53 @@ function comment_count_unpublished() {
+function _comment_body_field_create($entity_type, $bundle_name, $field_name) {
+  Drupal::service('comment.manager')->addBodyField($field_name);
 }

This function does not appear to be called

+++ b/core/modules/comment/comment.moduleundefined
@@ -394,19 +406,25 @@ function comment_permission() {
+ *
+ * @todo entity access for other entity types?

Is this still to do?

+++ b/core/modules/comment/comment.moduleundefined
@@ -511,171 +539,143 @@ function theme_comment_block($variables) {
+    // compatability. Should you require comment links for other entity types

Misspelt compatability should be compatibility

+++ b/core/modules/comment/comment.moduleundefined
@@ -511,171 +539,143 @@ function theme_comment_block($variables) {
-              'attributes' => array('title' => t('Share your thoughts and opinions related to this posting.')),
...
+                'attributes' => array('title' => t('Share your thoughts and opinions related to this item.')),

Maybe we need to refactor this text some more.

+++ b/core/modules/comment/comment.moduleundefined
@@ -511,171 +539,143 @@ function theme_comment_block($variables) {
+          if (user_access('post comments')) {

user_access is deprecated

+++ b/core/modules/comment/comment.moduleundefined
@@ -756,32 +758,38 @@ function comment_add(EntityInterface $node, $pid = NULL) {
+    ->addTag('entity_access')
...
+    ->addTag('entity_access')

I can't find query_entity_access_alter() - does this actually do anything at this point?

+++ b/core/modules/comment/comment.moduleundefined
@@ -947,234 +957,175 @@ function comment_view_multiple($comments, $view_mode = 'full', $langcode = NULL)
+    // Do not query database when entity has not comment fields.

not => no

+++ b/core/modules/comment/comment.moduleundefined
@@ -947,234 +957,175 @@ function comment_view_multiple($comments, $view_mode = 'full', $langcode = NULL)
+      $entities[$record->entity_id]->comment_statistics[$record->field_name] = new \StdClass();

What can we do to not use \StdClass?

+++ b/core/modules/comment/comment.moduleundefined
@@ -947,234 +957,175 @@ function comment_view_multiple($comments, $view_mode = 'full', $langcode = NULL)
+  global $user;

We shouldn't be using global $user anymore

+++ b/core/modules/comment/comment.moduleundefined
@@ -947,234 +957,175 @@ function comment_view_multiple($comments, $view_mode = 'full', $langcode = NULL)
-  // maintenance of the {node_comment_statistics} table.
-  if (variable_get('comment_maintain_node_statistics', TRUE)) {
-    db_insert('node_comment_statistics')
-      ->fields(array(
-        'nid' => $node->id(),
+  // maintenance of the {comment_entity_statistics} table.
+  $fields = Drupal::service('comment.manager')->getFields($entity->entityType());
+  if ($fields && Drupal::state()->get('comment.maintain_entity_statistics')) {

The point of comment.maintain_entity_statistics configuration is to speed stuff up so lets put $fields = Drupal::service('comment.manager')->getFields($entity->entityType());
inside the if.

+++ b/core/modules/comment/comment.moduleundefined
@@ -1446,13 +1445,29 @@ function comment_preview(Comment $comment) {
+    // The comment field output includes rendering the parent entity of the
+    // thread to which the comment is a reply. The rendered entity output
+    // includes the comment reply form, which contains the comment preview and
+    // therefore the rendered parent entity. This results in an infinite loop of
+    // parent entity output rendering the comment form and the comment form
+    // rendering the parent entity. To prevent this infinite loop we temporarily
+    // set the value of the comment field on the rendered entity to hidden
+    // before calling entity_view(). That way when the output of the commented
+    // entity is rendered, it excludes the comment field output. As objects are
+    // always addressed by reference we ensure changes are not lost by setting
+    // the value back to its original state after the call to entity_view().
+    $original_value = $entity->{$comment->field_name->value};
+    $entity->{$comment->field_name->value} = array(
+      Language::LANGCODE_NOT_SPECIFIED => array(array('status' => COMMENT_HIDDEN)),

Why? and this is very difficult to grok as an explanation.

+++ b/core/modules/comment/comment.moduleundefined
@@ -1799,3 +1826,142 @@ function comment_library_info() {
+ * @param array $values
+ *   The field values array or NULL for field create with field UI.
...
+  if (is_object($values)) {
+    $values = $values->getValue();

Looking at the code for the function $values can sometime be an object.

+++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.phpundefined
@@ -22,14 +22,16 @@ class CommentFormController extends EntityFormControllerNG {
+    $entity = entity_load($comment->entity_type->value, $comment->entity_id->value);
+    $instance = field_info_instance($comment->entity_type->value, $comment->field_name->value, $entity->bundle());

@@ -177,8 +179,9 @@ public function form(array $form, array &$form_state) {
+    $entity = entity_load($comment->entity_type->value, $comment->entity_id->value);
+    $instance = field_info_instance($comment->entity_type->value, $comment->field_name->value, $entity->bundle());

@@ -315,10 +318,15 @@ public function preview(array $form, array &$form_state) {
+    $entity = entity_load($form_state['values']['entity_type'], $form_state['values']['entity_id']);
...
+    $instance = field_info_instance($entity->entityType(), $field_name, $entity->bundle());

field_info_instance is deprecated - lets inject the field.info service and whilst we're at it we can inject the entity manager too.

+++ b/core/modules/comment/lib/Drupal/comment/CommentManager.phpundefined
@@ -0,0 +1,181 @@
+   * @param string $default_value
+   *   (optional) Default value, one of COMMENT_HIDDEN, COMMENT_OPEN,
+   *   COMMENT_CLOSED. Defaults to COMMENT_OPEN.

This is an integer

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.phpundefined
@@ -0,0 +1,167 @@
+  public static function schema(Field $field) {

Needs to be type hinting on FieldInterface

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.phpundefined
@@ -0,0 +1,167 @@
+      '#access' => user_access('post comments', drupal_anonymous_user()),

user_access is deprecated so shouldn't be used in new code.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentStatisticsTest.phpundefined
@@ -78,16 +78,17 @@ function testCommentNodeCommentStatistics() {
+    $comment_unpublished_loaded = comment_load($anonymous_comment->id());

I think this line is unnecessary

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/CommentUpgradePathTest.phpundefined
@@ -0,0 +1,115 @@
+  public function testCommentUpgrade() {
...
+    $expected_settings = array(
+      'article' => array (
+        'default_mode' => 1,
+        'per_page' => 50,
+        'form_location' => 1,
+        'anonymous' => 0,
+        'subject' => 1,
+        'preview' => 1,
+      ),
+      'blog' => array (
+        'default_mode' => 1,
+        'per_page' => 50,
+        'form_location' => 1,
+        'anonymous' => 0,
+        'subject' => 1,
+        'preview' => 1,
+      ),
+      'book' => array (
+        'default_mode' => 1,
+        'per_page' => 50,
+        'form_location' => 1,
+        'anonymous' => 0,
+        'subject' => 1,
+        'preview' => 1,
+      ),
+      'forum' => array (
+        'default_mode' => 1,
+        'per_page' => 50,
+        'form_location' => 1,
+        'anonymous' => 0,
+        'subject' => 1,
+        'preview' => 1,
+      ),
+      'page' => array (
+        'default_mode' => 1,
+        'per_page' => 50,
+        'form_location' => 1,
+        'anonymous' => 0,
+        'subject' => 1,
+        'preview' => 1,

It might be nice to adjust some the D7 comment variables so they are not all the default values

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/CommentUpgradePathTest.phpundefined
@@ -0,0 +1,115 @@
+      $instance = field_info_instance('node', 'comment_node_' . $type, $type);

Deprecated

This review is half done but I'm on the move and don't want to lose where I am.

alexpott’s picture

+++ b/core/modules/comment/comment.moduleundefined
@@ -1207,17 +1158,23 @@ function comment_node_update_index(EntityInterface $node, $langcode) {
+        $comments = comment_load_multiple($cids);

I think we should take the opportunity swap this out for entity_load_multiple to have one less wrapper.

+++ b/core/modules/comment/comment.moduleundefined
@@ -1799,3 +1826,142 @@ function comment_library_info() {
+    if ($comment->entity_type->value == 'node' && module_exists('history')) {
+      $cache[$cid] = history_read($comment->entity_id->value);

module_exists is deprecated

+++ b/core/modules/comment/comment.moduleundefined
@@ -1799,3 +1826,142 @@ function comment_library_info() {
+      // @todo - decide how to handle last viewed for other entities. For now
+      // assume REQUEST_TIME.
+      $cache[$cid] = REQUEST_TIME;

Do we need to make this decision here? Or is this part of the history for any entity issue? Also "other entities" is technically incorrect since if the history module is not enabled this will include node too.

+++ b/core/modules/comment/comment.tokens.incundefined
@@ -79,9 +79,16 @@ function comment_token_info() {
-    'description' => t("The node the comment was posted to."),
+    'description' => t("The node the comment was posted to (deprecated)."),

I think change this to DEPRECATED: The node the comment was posted to. because we plan to remove this in D9 and it has to stand out.

+++ b/core/modules/comment/comment.views.incundefined
@@ -372,18 +430,31 @@ function comment_views_data() {
+  // Provide a relationship for each entity type except comment.
...
+      // Explain how this table joins to others.

The second comment is now redundant given the first.

+++ b/core/modules/comment/comment.views.incundefined
@@ -372,18 +430,31 @@ function comment_views_data() {
+    // @todo As you can't have multiple joins this join doesn't use the field_name yet.

So what are we going to do?

+++ b/core/modules/comment/lib/Drupal/comment/CommentBreadcrumbBuilder.phpundefined
@@ -0,0 +1,62 @@
+    // @todo This only works for legacy routes. Once
+    // comment/reply/%/%comment_entity_reply/% is converted to the new router
+    // this code will need to be updated.

Let's link to the issue that is doing this.

+++ b/core/modules/comment/lib/Drupal/comment/CommentRenderController.phpundefined
@@ -37,22 +37,27 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
+      $comment_entities[$entity_type] = entity_load_multiple($entity_type, $entity_ids);

Let's inject the entity manager here... we'll need to implement the EntityControllerInterface

+++ b/core/modules/comment/lib/Drupal/comment/CommentRenderController.phpundefined
@@ -76,8 +82,10 @@ protected function alterBuild(array &$build, EntityInterface $comment, EntityDis
+      $comment_entity = entity_load($comment->entity_type->value, $comment->entity_id->value);
+      $instance = field_info_instance($comment_entity->entityType(), $comment->field_name->value, $comment_entity->bundle());

And we can convert both of these to use the injected services too

+++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.phpundefined
@@ -0,0 +1,220 @@
+    // @todo Remove when entity_get_bundles() is a method on the entity manager.

Should have a linked issue.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -40,24 +42,36 @@ class CommentController implements ControllerInterface {
   public static function create(ContainerInterface $container) {
     return new static(
       $container->get('url_generator'),
-      $container->get('http_kernel')
+      $container->get('http_kernel'),
+      $container->get('field.info')

@@ -115,15 +129,18 @@ public function commentApprove(Request $request, CommentInterface $comment) {
+    if ($entity = entity_load($comment->entity_type->value, $comment->entity_id->value)) {

Let's inject the entity manager too

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.phpundefined
@@ -132,4 +149,30 @@ public function commentPermalink(Request $request, CommentInterface $comment) {
+      return new RedirectResponse(url('comment/reply/node/' . $node->id() . '/' . $field_name, array('absolute' => TRUE)));

SHould be using the already injected path generator service.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.phpundefined
@@ -0,0 +1,167 @@
+    $settings = $this->getInstance()->settings;

Should be $this->getFieldSettings();

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.phpundefined
@@ -0,0 +1,167 @@
+      '#access' => user_access('post comments', drupal_anonymous_user()),

Deprecated user_access

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/formatter/CommentDefaultFormatter.phpundefined
@@ -0,0 +1,89 @@
+      if (((!empty($entity->comment_statistics[$field_name]->comment_count) && user_access('access comments')) || user_access('administer comments')) &&
...
+          $comments = comment_load_multiple($cids);
...
+        if (user_access('post comments') && !empty($entity->content['#view_mode']) &&

Let's inject the request by implementing ContainerFactoryPluginInterface and also inject the entity manager

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/Link.phpundefined
@@ -63,7 +62,11 @@ protected function renderLink($data, ResultRow $values) {
+      $entity = entity_load($entity_type, $entity_id);

Let's inject the entity manager - if possible.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentFieldsTest.phpundefined
@@ -68,12 +67,25 @@ function testCommentEnable() {
+    if ($field = field_info_field('comment_node_forum')) {

field_info_field is deprecated

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTokenReplaceTest.phpundefined
@@ -100,11 +100,11 @@ function testCommentTokenReplacement() {
-    $tests['[node:comment-count]'] = 2;
-    $tests['[node:comment-count-new]'] = 2;
+    $tests['[entity:comment-count]'] = 2;
+    $tests['[entity:comment-count-new]'] = 2;

Don't we still have to test the node: to ensure the BC stuff works?

+++ b/core/modules/forum/forum.installundefined
@@ -118,6 +122,13 @@ function forum_uninstall() {
+  // Load the dependent Comment module, in case it has been disabled.
+  drupal_load('module', 'comment');
+
+  if ($field = field_info_field('comment_node_forum')) {
+    $field->delete();
+  }

Very ugly. Disabled modules must die.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTitleTest.phpundefined
@@ -54,7 +55,7 @@ function testNodeTitle() {
+    $this->drupalGet("comment/reply/node/" . $node->id() . "/comment");

Unnecessary double quotes

/**
 * Gathers a listing of links to nodes.
 *
 * @param $result
 *   A database result object from a query to fetch node entities. If your
 *   query joins the {node_comment_statistics} table so that the comment_count
 *   field is available, a title attribute will be added to show the number of
 *   comments.
 * @param $title
 *   (optional) A heading for the resulting list.
 *
 * @return
 *   A renderable array containing a list of linked node titles fetched from
 *   $result, or FALSE if there are no rows in $result.
 */
function node_title_list($result, $title = NULL) {

node_comment_statistics is now entity_comment_statistics

  /**
   * The prefix for the bundles of this entity type.
   *
   * For example, the comment bundle is prefixed with 'comment_node_'.
   *
   * @var string (optional)
   */
  public $bundle_prefix;

In Drupal\Core\Entity\Annotation\EntityType. I don't think this is true anymore

alexpott’s picture

Edit: removing duplicate

alexpott’s picture

And last but not least.

In manual testing the issue I discovered the following functionality.

How to recreate:

  1. Install standard profile
  2. add a comment field to users
  3. set default to be closed
  4. go to user 1 can see comment form

This scenario was not really present before. The closest you could get to this was creating content and then enabling the comment module. And because the node's comment status defaults to 0 commenting is closed on all existing content. This would also occur if you only enabled comment after a while and then added a comment field to a node type. In the past regardless of the default value no old node would get a comment form. Now they do. Perhaps the desired behaviour would be to default to the the default value selected in the field settings.

There is some help text in comment_help() on this "(changes to the settings on existing content must be done manually)". Not sure it is sufficient.

EDIT: and massive apologies if there is duplication with xjm's reviews above but I did this review offline and without access to here work.

larowlan’s picture

We really need someone to document What can we do to not use \StdClass?

I think we can make these a property of the commentitem fielditem. Eg $entity->some_comment_field->count.

larowlan’s picture

if only we could track the review comments here https://github.com/drupal/drupal/pull/12/files

andypost’s picture

FileSize
407.57 KB

testing current merged sandbox

EDIT
still can't push

comment_as_field$ git push
fatal: remote error: Pushes to sandboxes are currently disabled.

Also patch makes some fixes to track node and field module changes

Status: Needs review » Needs work

The last submitted patch, sandbox-merge-365.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
407.3 KB
6.84 KB

Merged and fixed most of all broken tests

Status: Needs review » Needs work

The last submitted patch, sandbox-merge-367.patch, failed testing.

Berdir’s picture

The search fails should be fixable by replacing $node->type in comment_node_update_index() with $node->getType(). The file field test should be somewhat similar, replace a node field with a method.

larowlan’s picture

andypost’s picture

Status: Needs work » Needs review
FileSize
408.6 KB
3.15 KB
4.02 KB

Only upgrade tests still broken, trying to fix

Added CommentManager::getParentEntityUri() helper to properly fix merged DeleteForm for comment (see interdiff_merge.txt)

EDIT Fixed #353 in 79c8b0e1

The last submitted patch, sandbox-merge-371.patch, failed testing.

andypost’s picture

FileSize
407.69 KB
9.32 KB

Set of changes
- removed _comment_get_default_status() in favour of entity field api methods (added @todo - probably we need to compare values not array of values)
- optimized comment_entity_view() to run only on entity fields
- fixed upgrade path
- use $instance->getFieldSetting() for field settings

PS: out of scope - MyIsam throws error Failed: Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42000]: Syntax error or access violation: 1071 Specified key was too long; max key length is 1000 bytes: ALTER TABLE {file_usage} CHANGE `id` `id` VARCHAR(64) NOT NULL DEFAULT '' COMMENT 'The primary key of the object using the file.'; Array ( ) in Drupal\Core\Database\Connection->query() (line 571 of /www/core/lib/Drupal/Core/Database/Connection.php).

Status: Needs review » Needs work

The last submitted patch, sandbox-merge-373.patch, failed testing.

larowlan’s picture

+++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
@@ -43,6 +43,22 @@ public function __construct(FieldInfo $field_info, EntityManager $entity_manager
+  public function getParentEntityUri(CommentInterface $comment) {

Does this fit better on Drupal\comment\CommentInterface and therefore Drupal\comment\Entity\Comment - thoughts?
Or did you put it here because it needs injection?

andypost’s picture

Status: Needs work » Needs review
FileSize
407.78 KB
1.19 KB

Fixed regression, ModulesDisabledUpgradePathTest still broken - seems needs schema fix as user module does

EDIT I think comment manager is a better place for the function because it could be re-usable,
But probably CommentInterface is better place... no idea

Status: Needs review » Needs work

The last submitted patch, sandbox-merge-375.patch, failed testing.

andypost’s picture

I'm starting new branch to re-work the approach after #1497374: Switch from Field-based storage to Entity-based storage
Now we can't use field name as bundle for comment entity, so will merge in separate branch and update summary accordingly

andypost’s picture

FileSize
19.58 KB

Pushed initial code to comment-fieldapi5 branch
The main issue with bundle for comment entity - we can't use the field name now so will try menu_link approach with virtual bundle

larowlan’s picture

larowlan’s picture

Status: Needs work » Needs review
FileSize
420.82 KB

Just pasting the re-roll from #2079093: Patch testing issue for Comment-Field where we tracked the refactor post #1497374: Switch from Field-based storage to Entity-based storage
Interdiffs are there - but in summary - comment bundles are no longer the fieldname, instead {entity_type}__{fieldname}
Now onto the reviews above.

yched’s picture

Review of the @FieldType class.
Based on the last patch in the main issue (#731724-460: Convert comment settings into a field to make them work with CMI and non-node entities), but posting it here to avoid derailing even more :-/

  1. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.php
    @@ -0,0 +1,168 @@
    + *   module = "comment",
    

    Not needed anymore.

  2. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.php
    @@ -0,0 +1,168 @@
    +    $settings = $instance->settings;
    

    $settings = $this->getFieldSettings();

  3. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.php
    @@ -0,0 +1,168 @@
    +      '#field_name' => $field->id(),
    

    '#field_name' property name is not accurate anymore.

    Besides, _comment_field_instance_settings_form_process() is a bit beyond me, but it uses $element['#field_name'] as the second parameter for content_translation_enable_widget() which means it's supposed to be a bundle name - in which case $field->id() doesn't fit. This should be $entity_type__$field_name now, right ?

    $entity_type = $this->getParent()->entityType();
    $field_name = $this->getFieldDefinition()->getFieldName(); // or even $this->name I think
    '#bundle' => "{$entity_type}__{$field_name}

    And then you don't need the $field & $instance variables in there. We do really try to avoid FieldItem classes having code that refer to "configurable fields" definition structures (field / instance), and instead do all of the work only using the single getFieldDefinition().

    Also, _comment_field_instance_settings_form_process() would be better off as a static protected method within the class (then, '#process' => array(array(get_class($this), '_comment_field_instance_settings_form_process')),

  4. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.php
    @@ -0,0 +1,168 @@
    +    //$defaults = $this->getInstance()->default_value;
    +    //$this->setValue(reset($defaults), $notify);
    

    Some code commented out ?

  5. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/widget/CommentWidget.php
    @@ -0,0 +1,85 @@
    +    $entity = $element['#entity'];
    

    Minor, but would be better with $entity = $items->getParent();

andypost’s picture

FileSize
419.35 KB
32.02 KB
3.8 KB

Merged HEAD and addressed #382, @yched thanx a lot!
Also get rid of comment_add_default_comment_field() in favour of CommentManager::addDefaultField()

andypost’s picture

What we should do:

  • Clean-up comments and bits of code #2083895: Clean-up comment module doc blocks
  • remove hook_comment_access() + COMMENT_ACCESS_DENY and re-use entity field access for comment_file_download_access(), probably check comment_reply_access()
  • larowlan wants to re-make statistics as dynamic field property
  • clean-up|deprecate functions to CommentManager (could be done as follow-up
yched’s picture

Thanks @andypost.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.php
@@ -165,4 +163,27 @@ public function isEmpty() {
+  public static function _comment_field_instance_settings_form_process($element) {

Oh, forgot to add that the function name should be renamed / camelCased OO style :-)

andypost’s picture

FileSize
419.06 KB
6.24 KB

Renamed to processSettingsElement()
Also cleaned-up some deprecated calls after #2030151: Convert entity_get_bundles to a method on the entity manager

Status: Needs review » Needs work
Issue tags: -Needs tests

The last submitted patch, sandbox-merge-386.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests

#386: sandbox-merge-386.patch queued for re-testing.

micbar’s picture

I tested the patch from #386 on simplytest.me.
If i add comments to a taxonomy vocabulary, it throws following error saving the field settings

Fatal error: Call to undefined method Drupal\taxonomy\Entity\Term::getType() in /home/s4f69e6ad382ad79/www/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.php on line 85

If i add comments to users, it throws following error saving the field settings
Fatal error: Call to undefined method Drupal\user\Entity\User::getType() in /home/s4f69e6ad382ad79/www/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.php on line 85

Anonymous’s picture

Does this need a reroll? I tried testing with simplytest.me, but install resulted in an error.

dsnopek’s picture

@linclark: The latest version of this patch can be found here #2079093: Patch testing issue for Comment-Field

larowlan’s picture

larowlan’s picture

New patch up at #2079093: Patch testing issue for Comment-Field with a massive interdiff addressing most of the review points.
Still some to-dos in https://docs.google.com/a/previousnext.com.au/spreadsheet/ccc?key=0AlTpa... but we're getting close.

larowlan’s picture

All of these review items except the changes to CommentUserTest have been rectified, lastest patch is at #2079093: Patch testing issue for Comment-Field

We have 4 fails because of shortcomings in entity-render cache in HEAD.

Keep the reviews coming - lets get this in during Drupalcon!

larowlan’s picture

Status: Needs review » Closed (fixed)

All reviews from here addressed.
Please post new reviews on #2079093: Patch testing issue for Comment-Field

larowlan’s picture

Issue summary: View changes

Updated issue summary.