Files: 
CommentFileSizeAuthor
#386 interdiff.txt6.24 KBandypost
#386 sandbox-merge-386.patch419.06 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,382 pass(es).
[ View ]
#383 interdiff_type.txt3.8 KBandypost
#383 interdiff.txt32.02 KBandypost
#383 sandbox-merge-383.patch419.35 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,887 pass(es).
[ View ]
#381 comment-field.reroll.27.patch420.82 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 58,964 pass(es).
[ View ]
#379 interdiff_new.txt19.58 KBandypost
#376 interdiff.txt1.19 KBandypost
#376 sandbox-merge-375.patch407.78 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 58,431 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#373 interdiff.txt9.32 KBandypost
#373 sandbox-merge-373.patch407.69 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 58,791 pass(es), 20 fail(s), and 90 exception(s).
[ View ]
#371 interdiff_merge.txt4.02 KBandypost
#371 interdiff.txt3.15 KBandypost
#371 sandbox-merge-371.patch408.6 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 58,703 pass(es), 7 fail(s), and 7 exception(s).
[ View ]
#367 interdiff.txt6.84 KBandypost
#367 sandbox-merge-367.patch407.3 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 58,490 pass(es), 55 fail(s), and 111 exception(s).
[ View ]
#365 sandbox-merge-365.patch407.57 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 58,000 pass(es), 117 fail(s), and 168 exception(s).
[ View ]
#347 decouple-comment-node-731724.helper.347.interdiff.txt711 byteslarowlan
#347 decouple-comment-node-731724.helper.347.patch409.9 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 58,196 pass(es).
[ View ]
#345 decouple-comment-node-731724.helper.345.interdiff.txt5.02 KBlarowlan
#345 decouple-comment-node-731724.helper.345.patch409.89 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 58,236 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#342 decouple-comment-node-731724.helper.342.interdiff.txt2.54 KBlarowlan
#342 decouple-comment-node-731724.helper.342.patch408.95 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 58,101 pass(es), 3 fail(s), and 5 exception(s).
[ View ]
#340 decouple-comment-node-731724.helper.340.patch408.73 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 55,545 pass(es), 1,027 fail(s), and 634 exception(s).
[ View ]
#339 decouple-comment-node-731724.helper.339.interdiff.txt1.47 KBlarowlan
#339 decouple-comment-node-731724.helper.339.patch408.92 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 57,832 pass(es).
[ View ]
#333 decouple-comment-node-731724.helper.333.interdiff.txt871 byteslarowlan
#333 decouple-comment-node-731724.helper.333.patch408.46 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 57,709 pass(es).
[ View ]
#331 decouple-comment-node-731724.helper.331.interdiff.txt1.27 KBlarowlan
#331 decouple-comment-node-731724.helper.331.patch408.47 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#329 decouple-comment-node-731724.332.interdiff.txt4.34 KBlarowlan
#329 decouple-comment-node-731724.332.patch408.31 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 57,563 pass(es), 0 fail(s), and 33 exception(s).
[ View ]
#327 decouple-comment-node-731724.helper.330.interdiff.txt3.55 KBlarowlan
#327 decouple-comment-node-731724.helper.330.patch407.37 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 57,535 pass(es), 7 fail(s), and 2 exception(s).
[ View ]
#325 decouple-comment-node-731724.helper.325.interdiff.txt791 byteslarowlan
#325 decouple-comment-node-731724.helper.325.patch407.26 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 56,571 pass(es), 637 fail(s), and 265 exception(s).
[ View ]
#321 1907960-321.patch807.07 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to clear checkout directory.
[ View ]
#321 interdiff.txt1.58 KBlinclark
#317 interdiff.txt3 KBandypost
#317 sandbox-merge-317.patch406.8 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 57,574 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#316 interdiff.txt1.84 KBandypost
#316 sandbox-merge-316.patch404.25 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 57,221 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#314 sandbox-merge-314.patch404.11 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 56,778 pass(es), 10 fail(s), and 3 exception(s).
[ View ]
#312 addition.txt3.91 KBandypost
#312 sandbox-merge-312.patch405.77 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 56,752 pass(es), 13 fail(s), and 1 exception(s).
[ View ]
#310 interdiff.txt3.91 KBandypost
#310 sandbox-merge-310.patch397.78 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 57,332 pass(es), 22 fail(s), and 2 exception(s).
[ View ]
#308 interdiff.txt6.06 KBandypost
#308 sandbox-merge-308.patch396.82 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 56,986 pass(es), 3 fail(s), and 3 exception(s).
[ View ]
#306 interdiff.txt6.88 KBandypost
#306 sandbox-merge-306.patch397.22 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 57,219 pass(es), 4 fail(s), and 1 exception(s).
[ View ]
#304 sandbox-merge-304.patch389.19 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 56,955 pass(es), 1 fail(s), and 3 exception(s).
[ View ]
#299 interdiff.txt3.42 KBandypost
#299 sandbox-merge-298.patch385.98 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 56,855 pass(es), 26 fail(s), and 3 exception(s).
[ View ]
#297 interdiff.txt5.03 KBandypost
#297 sandbox-merge-296.patch389.87 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 56,643 pass(es), 27 fail(s), and 3 exception(s).
[ View ]
#295 sandbox-merge-294.patch388.98 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 56,634 pass(es), 27 fail(s), and 126 exception(s).
[ View ]
#292 sandbox-merge-292.patch388.89 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 56,819 pass(es), 124 fail(s), and 210 exception(s).
[ View ]
#290 interdiff.txt3.97 KBandypost
#290 sandbox-merge-290.patch388.38 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 56,898 pass(es), 34 fail(s), and 112 exception(s).
[ View ]
#287 decouple-comment-node-731724.helper.287.interdiff.txt3.39 KBlarowlan
#287 decouple-comment-node-731724.helper.287.patch390.11 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 58,386 pass(es), 35 fail(s), and 162 exception(s).
[ View ]
#285 decouple-comment-node-731724.helper.285.interdiff.txt13.28 KBlarowlan
#285 decouple-comment-node-731724.helper.285.patch389.83 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 58,122 pass(es), 39 fail(s), and 100 exception(s).
[ View ]
#283 sandbox-merge-283.patch387.53 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#262 interdiff.txt2.77 KBandypost
#262 interdiff2.txt832 bytesandypost
#262 sandbox-merge-263.patch390.07 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,867 pass(es).
[ View ]
#260 interdiff.txt616 bytesandypost
#260 sandbox-merge-260.patch387.72 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 58,342 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#257 decouple-comment-node-731724.helper.257.interdiff.txt1.04 KBlarowlan
#257 decouple-comment-node-731724.257.patch389.75 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 58,320 pass(es).
[ View ]
#255 decouple-comment-node-731724.helper.255.patch388.7 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 58,770 pass(es), 1 fail(s), and 3 exception(s).
[ View ]
#255 decouple-comment-node-731724.helper.255.interdiff.txt1.3 KBlarowlan
#253 decouple-comment-node-731724.helper.253.patch387.66 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 58,261 pass(es), 17 fail(s), and 0 exception(s).
[ View ]
#250 interdiff.txt679 bytesandypost
#250 sandbox-merge-247.patch385.74 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,010 pass(es).
[ View ]
#246 interdiff.txt8.13 KBandypost
#246 sandbox-merge-245_.patch386.02 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 57,366 pass(es), 16 fail(s), and 151 exception(s).
[ View ]
#244 decouple-comment-node-731724.helper.244.patch387.8 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 56,551 pass(es), 422 fail(s), and 1,403 exception(s).
[ View ]
#239 decouple-comment-node-731724.helper.328.interdiff.txt3.21 KBlarowlan
#239 decouple-comment-node-731724.238.patch388.78 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-731724.238.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#236 decouple-comment-node-731724.helper.236.patch386.58 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-731724.helper.236.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#231 interdiff.txt2.26 KBandypost
#231 sandbox-merge-231.patch386.27 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,908 pass(es).
[ View ]
#230 sandbox-merge-230.patch386.2 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 56,481 pass(es).
[ View ]
#229 sandbox-merge-229.patch385.37 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 56,306 pass(es).
[ View ]
#227 sandbox-merge-227.patch385 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,669 pass(es).
[ View ]
#225 interdiff.txt2.39 KBandypost
#225 sandbox-merge-225.patch385.34 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sandbox-merge-225.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#224 interdiff.txt594 bytesandypost
#224 sandbox-merge-224.patch385.41 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sandbox-merge-224.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#223 interdiff.txt403.12 KBandypost
#223 sandbox-merge-223.patch385.3 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sandbox-merge-223.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#221 interdiff.txt4.23 KBandypost
#221 sandbox-merge-221.patch385.25 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 57,941 pass(es), 12 fail(s), and 13 exception(s).
[ View ]
#220 sandbox-merge-220.patch385.76 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 57,314 pass(es), 44 fail(s), and 49 exception(s).
[ View ]
#219 sandbox-merge-218.patch389.19 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,690 pass(es).
[ View ]
#218 sandbox-merge-217.patch389.13 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,983 pass(es).
[ View ]
#215 sandbox-merge-215.patch389.13 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,531 pass(es).
[ View ]
#211 interdiff.txt1018 bytesandypost
#211 sandbox-merge-210.patch389.67 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 56,411 pass(es).
[ View ]
#209 comment-debug.txt2.55 KBandypost
#205 sandbox-merge-205.patch389.29 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sandbox-merge-205.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#203 interdiff.txt6.67 KBandypost
#203 sandbox-merge-203.patch389.57 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 56,292 pass(es), 0 fail(s), and 4 exception(s).
[ View ]
#202 interdiff.txt887 bytesandypost
#202 sandbox-merge-202.patch394.33 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 57,365 pass(es), 10 fail(s), and 7 exception(s).
[ View ]
#200 decouple-comment-node-731724.helper.200.patch395.51 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 44,831 pass(es), 549 fail(s), and 403 exception(s).
[ View ]
#195 interdiff.txt11.61 KBandypost
#195 sandbox-merge-195.patch391.28 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,849 pass(es).
[ View ]
#193 sandbox-merge-193.patch391.01 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 55,137 pass(es), 143 fail(s), and 5,200 exception(s).
[ View ]
#190 interdiff.txt1.02 KBandypost
#190 sandbox-merge-190.patch390.38 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 56,190 pass(es).
[ View ]
#189 sandbox-merge-189.patch389.36 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 56,222 pass(es).
[ View ]
#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
PASSED: [[SimpleTest]]: [MySQL] 56,238 pass(es).
[ View ]
#186 interdiff.txt2.32 KBandypost
#186 sandbox-merge-186.patch388.38 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 56,175 pass(es).
[ View ]
#184 decouple-comment-node-731724.helper.184.interdiff.txt3.33 KBlarowlan
#180 interdiff.txt1.3 KBandypost
#180 sandbox-merge-180.patch386.13 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 57,700 pass(es), 48 fail(s), and 95 exception(s).
[ View ]
#178 interdiff.txt11.91 KBandypost
#178 sandbox-merge-178.patch385.41 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 56,005 pass(es), 10 fail(s), and 2 exception(s).
[ View ]
#176 interdiff.txt5.18 KBandypost
#176 sandbox-merge-176.patch383.69 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 56,400 pass(es).
[ View ]
#175 interdiff.txt2.37 KBandypost
#175 sandbox-merge-175.patch381.98 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,970 pass(es).
[ View ]
#173 interdiff.txt4.28 KBandypost
#173 sandbox-merge-173.patch381.95 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 56,358 pass(es), 27 fail(s), and 18 exception(s).
[ View ]
#171 sandbox-merge-171.patch381.87 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 55,361 pass(es), 26 fail(s), and 25 exception(s).
[ View ]
#170 decouple-comment-node-731724.helper.171.interdiff.txt18.4 KBlarowlan
#170 decouple-comment-node-731724.helper.171.patch383.87 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 55,337 pass(es), 25 fail(s), and 25 exception(s).
[ View ]
#169 decouple-comment-node-731724.helper.169.interdiff.txt823 byteslarowlan
#169 decouple-comment-node-731724.helper.169.patch377.93 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 56,213 pass(es).
[ View ]
#165 decouple-comment-node-731724.helper.165.patch408.34 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 55,889 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#165 xhprof-output.zip17.87 KBlarowlan
#164 interdiff.txt5.63 KBandypost
#164 decouple-comment-node-731724.164.patch385.12 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,670 pass(es).
[ View ]
#164 decouple-comment-node-731724.164_.patch376.3 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,951 pass(es).
[ View ]
#162 decouple-comment-node-731724.helper.162.interdiff.txt1.16 KBlarowlan
#162 decouple-comment-node-731724.162.patch377.35 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 55,662 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
#161 decouple-comment-node-731724.helper.161.interdiff.txt712 byteslarowlan
#161 decouple-comment-node-731724.161.patch377.65 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 55,570 pass(es), 2 fail(s), and 77 exception(s).
[ View ]
#159 decouple-comment-node-731724.helper.159.interdiff.txt2.69 KBlarowlan
#159 decouple-comment-node-731724.helper.159.patch378.6 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 55,169 pass(es), 74 fail(s), and 27 exception(s).
[ View ]
#156 decouple-comment-node-731724.helper.156.patch377.51 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 55,212 pass(es), 31 fail(s), and 32 exception(s).
[ View ]
#154 comment-updates.txt2.53 KBandypost
#154 comment-def.txt2.42 KBandypost
#152 sandbox-merge-151.patch378.13 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 54,893 pass(es), 7 fail(s), and 7 exception(s).
[ View ]
#150 sandbox-merge-150.patch386.9 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 54,931 pass(es), 8 fail(s), and 7 exception(s).
[ View ]
#150 interdiff.txt982 bytesandypost
#148 interdiff.txt1.59 KBandypost
#148 sandbox-merge-148.patch377.84 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 55,186 pass(es), 10 fail(s), and 1 exception(s).
[ View ]
#146 sandbox-merge-146.patch376.81 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 55,088 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#144 interdiff.txt1.93 KBandypost
#144 sandbox-merge-144.patch374.12 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 54,653 pass(es), 4 fail(s), and 6 exception(s).
[ View ]
#142 sandbox-merge-142.patch373.09 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 54,495 pass(es), 44 fail(s), and 10 exception(s).
[ View ]
#140 decouple-comment-node-731724.helper.140.interdiff.txt2.58 KBlarowlan
#140 decouple-comment-node-731724.helper.140.patch374.32 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 54,312 pass(es), 44 fail(s), and 12 exception(s).
[ View ]
#138 sandbox-merge-138.patch372.63 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#135 sandbox-merge-M25-135.patch373.15 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 54,090 pass(es), 1 fail(s), and 4 exception(s).
[ View ]
#134 drupal-1907960-134.patch376.09 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 54,135 pass(es), 0 fail(s), and 7 exception(s).
[ View ]
#133 interdiff.txt4.96 KBandypost
#133 sandbox-133.patch381.31 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 54,304 pass(es), 1 fail(s), and 6 exception(s).
[ View ]
#130 731724-upgrade-path.patch389.06 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 53,781 pass(es), 26 fail(s), and 27 exception(s).
[ View ]
#130 sandbox-merge-130.patch381.12 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 54,263 pass(es), 2 fail(s), and 6 exception(s).
[ View ]
#127 interdiff.txt1.64 KBandypost
#127 sandbox-merge-127.patch381.14 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#127 731724-upgrade-path.patch389.08 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#125 sandbox-merge-125.patch379.96 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 54,328 pass(es), 0 fail(s), and 126 exception(s).
[ View ]
#123 upgrade-path.txt13.95 KBandypost
#123 731724-upgrade-path.patch387.9 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 53,895 pass(es), 23 fail(s), and 137 exception(s).
[ View ]
#122 decouple-comment-node-731724.helper.122.patch372.61 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 53,441 pass(es).
[ View ]
#121 decouple-comment-node-731724.helper.121.interdiff.txt2.31 KBlarowlan
#121 decouple-comment-node-731724.helper.121.patch373.45 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 53,298 pass(es).
[ View ]
#116 decouple-comment-node-731724.helper.116.interdiff.txt604 byteslarowlan
#116 decouple-comment-node-731724.helper.116.patch373.81 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 53,349 pass(es).
[ View ]
#115 decouple-comment-node-731724.helper.115.interdiff.txt3.16 KBlarowlan
#115 decouple-comment-node-731724.helper.115.patch373.81 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 53,313 pass(es), 0 fail(s), and 5 exception(s).
[ View ]
#114 decouple-comment-node-731724.114.interdiff.txt6.21 KBlarowlan
#114 decouple-comment-node-731724.114.patch373.7 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 53,353 pass(es).
[ View ]
#112 comment-views.txt3.96 KBandypost
#112 comment-views.patch380.87 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 53,428 pass(es), 0 fail(s), and 91 exception(s).
[ View ]
#110 sandbox-merge-110.patch379 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 53,301 pass(es), 0 fail(s), and 91 exception(s).
[ View ]
#108 sandbox-merge-108.patch375.44 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 53,453 pass(es), 3 fail(s), and 91 exception(s).
[ View ]
#106 sandbox-merge-106.patch375.51 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sandbox-merge-106.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#104 sandbox-merge-104.patch375.67 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 53,219 pass(es), 0 fail(s), and 10 exception(s).
[ View ]
#102 sandbox-merge-102.patch366.93 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 52,871 pass(es), 24 fail(s), and 20 exception(s).
[ View ]
#98 interdiff-98.txt1.88 KBandypost
#98 sandbox-98.patch375.93 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 52,817 pass(es).
[ View ]
#95 decouple-comment-node-731724.385.interdiff.txt3.02 KBlarowlan
#95 decouple-comment-node-731724.385.patch368.55 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#93 sandbox.merge_.patch375.72 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 52,556 pass(es).
[ View ]
#91 formatters6.patch387.76 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 52,482 pass(es), 43 fail(s), and 0 exception(s).
[ View ]
#89 formatters6.patch395.53 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 52,474 pass(es), 67 fail(s), and 0 exception(s).
[ View ]
#88 comment-minimize.txt17.98 KBandypost
#88 comment-minimize.patch374.95 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 52,545 pass(es).
[ View ]
#87 rename-nested-interdiff.txt30.29 KBandypost
#87 rename-nested-interdiff.patch382.28 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 52,564 pass(es).
[ View ]
#85 formatters5.txt13.39 KBandypost
#85 formatters5.patch389.83 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 52,492 pass(es), 51 fail(s), and 0 exception(s).
[ View ]
#83 formatters4.txt8.91 KBandypost
#83 formatters4.patch388.63 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 52,520 pass(es), 125 fail(s), and 0 exception(s).
[ View ]
#81 formatters3.txt578 bytesandypost
#81 formatters3.patch388.59 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 52,497 pass(es), 126 fail(s), and 1 exception(s).
[ View ]
#79 formatters2.txt3.78 KBandypost
#79 formatters2.patch388.57 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 52,417 pass(es), 127 fail(s), and 10 exception(s).
[ View ]
#77 formatters.txt42.43 KBandypost
#77 formatters.patch388.46 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 52,284 pass(es), 187 fail(s), and 47 exception(s).
[ View ]
#76 cleanup_default_value.patch6.74 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cleanup_default_value.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#76 sandbox+default.patch375.11 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 52,595 pass(es).
[ View ]
#73 sandbox-73.txt1.79 KBandypost
#73 sandbox-73.patch366.26 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 52,643 pass(es).
[ View ]
#71 sandbox-70pre.txt3.59 KBandypost
#71 sandbox-70pre.patch374.75 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 51,783 pass(es).
[ View ]
#68 sandbox-68.txt1.66 KBandypost
#68 sandbox-68.patch371.53 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 51,692 pass(es).
[ View ]
#66 sandbox-66.txt1.59 KBandypost
#66 sandbox-66.patch373.1 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 51,692 pass(es), 3 fail(s), and 6 exception(s).
[ View ]
#64 sandbox-64.txt634 bytesandypost
#64 sandbox-64.patch372.13 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 51,595 pass(es), 4 fail(s), and 8 exception(s).
[ View ]
#63 sandbox-63.patch372.48 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 51,650 pass(es), 3 fail(s), and 6 exception(s).
[ View ]
#58 sandbox-58.patch371.51 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 51,723 pass(es), 2 fail(s), and 4 exception(s).
[ View ]
#56 sandbox-56.patch374.97 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 51,508 pass(es), 2 fail(s), and 4 exception(s).
[ View ]
#54 sandbox-54.txt7.57 KBandypost
#54 sandbox-54.patch375.69 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 48,047 pass(es), 60 fail(s), and 21 exception(s).
[ View ]
#52 sandbox.patch360.03 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sandbox_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#50 731724-interdiff-339.txt2.35 KBandypost
#50 731724-comment-decouple-339m.patch360.03 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 51,141 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#50 731724-comment-decouple-339.patch369.13 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 51,113 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#49 sandbox.patch369.13 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 51,109 pass(es).
[ View ]
#47 731724-comment-decouple-338.patch360.03 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,190 pass(es), 17 fail(s), and 0 exception(s).
[ View ]
#46 46.interdif.txt4.82 KBandypost
#46 46.patch362.84 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 51,166 pass(es).
[ View ]
#42 interdiff.txt35 KBandypost
#42 44.patch367.74 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,606 pass(es), 15 fail(s), and 31 exception(s).
[ View ]
#41 41-field-interdiff.txt30.57 KBandypost
#41 41.patch367.85 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,328 pass(es), 106 fail(s), and 69 exception(s).
[ View ]
#39 39.interdiff.txt1.17 KBandypost
#39 39.patch371.9 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,834 pass(es), 3 fail(s), and 6 exception(s).
[ View ]
#38 upgrade.patch.txt13.44 KBandypost
#36 731724-comment-decouple-327.interdiff.txt961 bytesandypost
#36 731724-comment-decouple-327.patch371.71 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,877 pass(es), 3 fail(s), and 4 exception(s).
[ View ]
#34 731724-comment-decouple-326.patch370.04 KBdixon_
FAILED: [[SimpleTest]]: [MySQL] 50,632 pass(es), 13 fail(s), and 37 exception(s).
[ View ]
#33 sandbox-33.patch371.73 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 50,794 pass(es).
[ View ]
#31 comment-31.patch368.89 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,692 pass(es), 4 fail(s), and 8 exception(s).
[ View ]
#29 interdiff-29.txt2.56 KBandypost
#29 comment-upgrade-config-29.patch369.08 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 50,857 pass(es).
[ View ]
#27 interdiff-27.txt1.86 KBandypost
#27 comment-upgrade-config.patch368.89 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,755 pass(es), 2 fail(s), and 12 exception(s).
[ View ]
#25 sandbox.patch368.44 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,569 pass(es), 7 fail(s), and 7 exception(s).
[ View ]
#24 sandbox-upgrade-plugins.patch368.44 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,562 pass(es), 8 fail(s), and 8 exception(s).
[ View ]
#23 db.interdiff.txt2.46 KBandypost
#23 23.patch389.53 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,837 pass(es), 2 fail(s), and 4 exception(s).
[ View ]
#23 comment-formatter.patch392.47 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 50,814 pass(es).
[ View ]
#21 sandbox-staged.interdiff.txt32.54 KBandypost
#21 20.patch368.44 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,770 pass(es), 3 fail(s), and 3 exception(s).
[ View ]
#20 20.patch378.92 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,561 pass(es), 169 fail(s), and 38 exception(s).
[ View ]
#19 comment-dd4ca06.patch365.5 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,908 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#16 16-sandbox.patch364.99 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,550 pass(es), 3 fail(s), and 6 exception(s).
[ View ]
#14 interdiff.txt5.18 KBandypost
#14 comment-widget.patch378.95 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 49,990 pass(es), 173 fail(s), and 42 exception(s).
[ View ]
#12 comment-widget.patch377.3 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,133 pass(es), 182 fail(s), and 39 exception(s).
[ View ]
#10 8.patch368.85 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,108 pass(es), 5 fail(s), and 5 exception(s).
[ View ]
#8 7.patch377.81 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,147 pass(es), 151 fail(s), and 198 exception(s).
[ View ]
#6 5.patch378.29 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,028 pass(es), 145 fail(s), and 205 exception(s).
[ View ]
#3 3.patch378.29 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 48,553 pass(es), 319 fail(s), and 373 exception(s).
[ View ]
#1 1.patch377.15 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 48,499 pass(es), 395 fail(s), and 370 exception(s).
[ View ]

Comments

StatusFileSize
new377.15 KB
FAILED: [[SimpleTest]]: [MySQL] 48,499 pass(es), 395 fail(s), and 370 exception(s).
[ View ]

Assigned:Unassigned» andypost
Status:Active» Needs review
StatusFileSize
new378.29 KB
FAILED: [[SimpleTest]]: [MySQL] 48,553 pass(es), 319 fail(s), and 373 exception(s).
[ View ]

Fixed some tests

StatusFileSize
new378.29 KB
FAILED: [[SimpleTest]]: [MySQL] 50,028 pass(es), 145 fail(s), and 205 exception(s).
[ View ]

Another fixes

StatusFileSize
new377.81 KB
FAILED: [[SimpleTest]]: [MySQL] 50,147 pass(es), 151 fail(s), and 198 exception(s).
[ View ]

StatusFileSize
new368.85 KB
FAILED: [[SimpleTest]]: [MySQL] 50,108 pass(es), 5 fail(s), and 5 exception(s).
[ View ]

Main branch tests

StatusFileSize
new377.3 KB
FAILED: [[SimpleTest]]: [MySQL] 50,133 pass(es), 182 fail(s), and 39 exception(s).
[ View ]

Another round to move settings to widget

StatusFileSize
new378.95 KB
FAILED: [[SimpleTest]]: [MySQL] 49,990 pass(es), 173 fail(s), and 42 exception(s).
[ View ]
new5.18 KB

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

<?php
-    $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

StatusFileSize
new364.99 KB
FAILED: [[SimpleTest]]: [MySQL] 50,550 pass(es), 3 fail(s), and 6 exception(s).
[ View ]

Testing of sandbox code

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

Status:Needs work» Needs review
StatusFileSize
new365.5 KB
FAILED: [[SimpleTest]]: [MySQL] 50,908 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

Commited schema changes for proper length of keys

StatusFileSize
new378.92 KB
FAILED: [[SimpleTest]]: [MySQL] 50,561 pass(es), 169 fail(s), and 38 exception(s).
[ View ]

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

StatusFileSize
new368.44 KB
FAILED: [[SimpleTest]]: [MySQL] 50,770 pass(es), 3 fail(s), and 3 exception(s).
[ View ]
new32.54 KB

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

StatusFileSize
new392.47 KB
PASSED: [[SimpleTest]]: [MySQL] 50,814 pass(es).
[ View ]
new389.53 KB
FAILED: [[SimpleTest]]: [MySQL] 50,837 pass(es), 2 fail(s), and 4 exception(s).
[ View ]
new2.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

StatusFileSize
new368.44 KB
FAILED: [[SimpleTest]]: [MySQL] 50,562 pass(es), 8 fail(s), and 8 exception(s).
[ View ]

Current sandbox after merge

StatusFileSize
new368.44 KB
FAILED: [[SimpleTest]]: [MySQL] 50,569 pass(es), 7 fail(s), and 7 exception(s).
[ View ]

Current sandbox code

StatusFileSize
new368.89 KB
FAILED: [[SimpleTest]]: [MySQL] 50,755 pass(es), 2 fail(s), and 12 exception(s).
[ View ]
new1.86 KB

Used a way like user_update_8011()

StatusFileSize
new369.08 KB
PASSED: [[SimpleTest]]: [MySQL] 50,857 pass(es).
[ View ]
new2.56 KB

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

StatusFileSize
new368.89 KB
FAILED: [[SimpleTest]]: [MySQL] 50,692 pass(es), 4 fail(s), and 8 exception(s).
[ View ]

Let's try

StatusFileSize
new371.73 KB
PASSED: [[SimpleTest]]: [MySQL] 50,794 pass(es).
[ View ]

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

StatusFileSize
new370.04 KB
FAILED: [[SimpleTest]]: [MySQL] 50,632 pass(es), 13 fail(s), and 37 exception(s).
[ View ]

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

StatusFileSize
new371.71 KB
FAILED: [[SimpleTest]]: [MySQL] 50,877 pass(es), 3 fail(s), and 4 exception(s).
[ View ]
new961 bytes

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

StatusFileSize
new13.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

StatusFileSize
new371.9 KB
FAILED: [[SimpleTest]]: [MySQL] 50,834 pass(es), 3 fail(s), and 6 exception(s).
[ View ]
new1.17 KB

StatusFileSize
new367.85 KB
FAILED: [[SimpleTest]]: [MySQL] 50,328 pass(es), 106 fail(s), and 69 exception(s).
[ View ]
new30.57 KB

StatusFileSize
new367.74 KB
FAILED: [[SimpleTest]]: [MySQL] 50,606 pass(es), 15 fail(s), and 31 exception(s).
[ View ]
new35 KB

current sandbox

StatusFileSize
new362.84 KB
PASSED: [[SimpleTest]]: [MySQL] 51,166 pass(es).
[ View ]
new4.82 KB

Fixed remains of ER convertion

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

StatusFileSize
new360.03 KB
FAILED: [[SimpleTest]]: [MySQL] 50,190 pass(es), 17 fail(s), and 0 exception(s).
[ View ]

Current sandbox.. minimized patch

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

StatusFileSize
new369.13 KB
PASSED: [[SimpleTest]]: [MySQL] 51,109 pass(es).
[ View ]

and again

StatusFileSize
new369.13 KB
FAILED: [[SimpleTest]]: [MySQL] 51,113 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
new360.03 KB
FAILED: [[SimpleTest]]: [MySQL] 51,141 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
new2.35 KB

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

StatusFileSize
new360.03 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sandbox_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

let's re-test sandbox

StatusFileSize
new375.69 KB
FAILED: [[SimpleTest]]: [MySQL] 48,047 pass(es), 60 fail(s), and 21 exception(s).
[ View ]
new7.57 KB

StatusFileSize
new374.97 KB
FAILED: [[SimpleTest]]: [MySQL] 51,508 pass(es), 2 fail(s), and 4 exception(s).
[ View ]

StatusFileSize
new371.51 KB
FAILED: [[SimpleTest]]: [MySQL] 51,723 pass(es), 2 fail(s), and 4 exception(s).
[ View ]

merged HEAD

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.

Issue summary:View changes

added related

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
                )
        )
)
'

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?

Status:Needs work» Needs review
StatusFileSize
new372.48 KB
FAILED: [[SimpleTest]]: [MySQL] 51,650 pass(es), 3 fail(s), and 6 exception(s).
[ View ]

StatusFileSize
new372.13 KB
FAILED: [[SimpleTest]]: [MySQL] 51,595 pass(es), 4 fail(s), and 8 exception(s).
[ View ]
new634 bytes

Let's see, seems data in upgrade broken

StatusFileSize
new373.1 KB
FAILED: [[SimpleTest]]: [MySQL] 51,692 pass(es), 3 fail(s), and 6 exception(s).
[ View ]
new1.59 KB

Both patches

StatusFileSize
new371.53 KB
PASSED: [[SimpleTest]]: [MySQL] 51,692 pass(es).
[ View ]
new1.66 KB

Let's see if get_t() removed

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

Great work here @andypost

StatusFileSize
new374.75 KB
PASSED: [[SimpleTest]]: [MySQL] 51,783 pass(es).
[ View ]
new3.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

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

StatusFileSize
new366.26 KB
PASSED: [[SimpleTest]]: [MySQL] 52,643 pass(es).
[ View ]
new1.79 KB

Merged HEAD

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

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

StatusFileSize
new375.11 KB
PASSED: [[SimpleTest]]: [MySQL] 52,595 pass(es).
[ View ]
new6.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cleanup_default_value.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

@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

StatusFileSize
new388.46 KB
FAILED: [[SimpleTest]]: [MySQL] 52,284 pass(es), 187 fail(s), and 47 exception(s).
[ View ]
new42.43 KB

Initial patch for formatters

StatusFileSize
new388.57 KB
FAILED: [[SimpleTest]]: [MySQL] 52,417 pass(es), 127 fail(s), and 10 exception(s).
[ View ]
new3.78 KB

small fixes

StatusFileSize
new388.59 KB
FAILED: [[SimpleTest]]: [MySQL] 52,497 pass(es), 126 fail(s), and 1 exception(s).
[ View ]
new578 bytes

fix upgrade path. Links still need some work

StatusFileSize
new388.63 KB
FAILED: [[SimpleTest]]: [MySQL] 52,520 pass(es), 125 fail(s), and 0 exception(s).
[ View ]
new8.91 KB

HEAD was broken

StatusFileSize
new389.83 KB
FAILED: [[SimpleTest]]: [MySQL] 52,492 pass(es), 51 fail(s), and 0 exception(s).
[ View ]
new13.39 KB

last one for today, should work except rdf and search

StatusFileSize
new382.28 KB
PASSED: [[SimpleTest]]: [MySQL] 52,564 pass(es).
[ View ]
new30.29 KB

Addressed #731724-341: Convert comment settings into a field to make them work with CMI and non-node entities
Moving instance settings from nested 'comment' to first level

StatusFileSize
new374.95 KB
PASSED: [[SimpleTest]]: [MySQL] 52,545 pass(es).
[ View ]
new17.98 KB

Some clean-up to minimize patch size

StatusFileSize
new395.53 KB
FAILED: [[SimpleTest]]: [MySQL] 52,474 pass(es), 67 fail(s), and 0 exception(s).
[ View ]

Another round on formatters

StatusFileSize
new387.76 KB
FAILED: [[SimpleTest]]: [MySQL] 52,482 pass(es), 43 fail(s), and 0 exception(s).
[ View ]

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

StatusFileSize
new375.72 KB
PASSED: [[SimpleTest]]: [MySQL] 52,556 pass(es).
[ View ]

merging head

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

Status:Needs work» Needs review
StatusFileSize
new368.55 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new3.02 KB

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

StatusFileSize
new375.93 KB
PASSED: [[SimpleTest]]: [MySQL] 52,817 pass(es).
[ View ]
new1.88 KB

Somehow Annotation now lives in Component

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

Green

StatusFileSize
new366.93 KB
FAILED: [[SimpleTest]]: [MySQL] 52,871 pass(es), 24 fail(s), and 20 exception(s).
[ View ]

testing changes before commit

StatusFileSize
new375.67 KB
FAILED: [[SimpleTest]]: [MySQL] 53,219 pass(es), 0 fail(s), and 10 exception(s).
[ View ]

Pushed this commit, testing sandbox

StatusFileSize
new375.51 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sandbox-merge-106.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

StatusFileSize
new375.44 KB
FAILED: [[SimpleTest]]: [MySQL] 53,453 pass(es), 3 fail(s), and 91 exception(s).
[ View ]

StatusFileSize
new379 KB
FAILED: [[SimpleTest]]: [MySQL] 53,301 pass(es), 0 fail(s), and 91 exception(s).
[ View ]

Fixed broken tests except views handlers

StatusFileSize
new380.87 KB
FAILED: [[SimpleTest]]: [MySQL] 53,428 pass(es), 0 fail(s), and 91 exception(s).
[ View ]
new3.96 KB

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

StatusFileSize
new373.7 KB
PASSED: [[SimpleTest]]: [MySQL] 53,353 pass(es).
[ View ]
new6.21 KB

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

StatusFileSize
new373.81 KB
FAILED: [[SimpleTest]]: [MySQL] 53,313 pass(es), 0 fail(s), and 5 exception(s).
[ View ]
new3.16 KB

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

StatusFileSize
new373.81 KB
PASSED: [[SimpleTest]]: [MySQL] 53,349 pass(es).
[ View ]
new604 bytes

Incorrect reference to $table in 115.

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

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.

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

StatusFileSize
new373.45 KB
PASSED: [[SimpleTest]]: [MySQL] 53,298 pass(es).
[ View ]
new2.31 KB

Chasing HEAD

StatusFileSize
new372.61 KB
PASSED: [[SimpleTest]]: [MySQL] 53,441 pass(es).
[ View ]

Chasing HEAD

StatusFileSize
new387.9 KB
FAILED: [[SimpleTest]]: [MySQL] 53,895 pass(es), 23 fail(s), and 137 exception(s).
[ View ]
new13.95 KB

New patch with upgrade path

StatusFileSize
new379.96 KB
FAILED: [[SimpleTest]]: [MySQL] 54,328 pass(es), 0 fail(s), and 126 exception(s).
[ View ]

Sanbox re-test

Status:Needs review» Needs work
StatusFileSize
new389.08 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
new381.14 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
new1.64 KB

Status:Needs work» Needs review

Pushed follow-up commit for

/**
- * Defines the 'test_field' entity field item.
+ * Defines the 'comment_field' entity field item.
  */

StatusFileSize
new381.12 KB
FAILED: [[SimpleTest]]: [MySQL] 54,263 pass(es), 2 fail(s), and 6 exception(s).
[ View ]
new389.06 KB
FAILED: [[SimpleTest]]: [MySQL] 53,781 pass(es), 26 fail(s), and 27 exception(s).
[ View ]

And now in proper place

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new381.31 KB
FAILED: [[SimpleTest]]: [MySQL] 54,304 pass(es), 1 fail(s), and 6 exception(s).
[ View ]
new4.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)

StatusFileSize
new376.09 KB
FAILED: [[SimpleTest]]: [MySQL] 54,135 pass(es), 0 fail(s), and 7 exception(s).
[ View ]

Does someone know why the interdiff fails?

StatusFileSize
new373.15 KB
FAILED: [[SimpleTest]]: [MySQL] 54,090 pass(es), 1 fail(s), and 4 exception(s).
[ View ]

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

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new372.63 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Let's see how much broken for last merge

StatusFileSize
new374.32 KB
FAILED: [[SimpleTest]]: [MySQL] 54,312 pass(es), 44 fail(s), and 12 exception(s).
[ View ]
new2.58 KB

Fixes plugin -> pluginid issues

StatusFileSize
new373.09 KB
FAILED: [[SimpleTest]]: [MySQL] 54,495 pass(es), 44 fail(s), and 10 exception(s).
[ View ]

Another merge

StatusFileSize
new374.12 KB
FAILED: [[SimpleTest]]: [MySQL] 54,653 pass(es), 4 fail(s), and 6 exception(s).
[ View ]
new1.93 KB

Fix some broken tests

StatusFileSize
new376.81 KB
FAILED: [[SimpleTest]]: [MySQL] 55,088 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

@dawehner++

StatusFileSize
new377.84 KB
FAILED: [[SimpleTest]]: [MySQL] 55,186 pass(es), 10 fail(s), and 1 exception(s).
[ View ]
new1.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

StatusFileSize
new982 bytes
new386.9 KB
FAILED: [[SimpleTest]]: [MySQL] 54,931 pass(es), 8 fail(s), and 7 exception(s).
[ View ]

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

StatusFileSize
new378.13 KB
FAILED: [[SimpleTest]]: [MySQL] 54,893 pass(es), 7 fail(s), and 7 exception(s).
[ View ]

Fix random failures

Status:Needs review» Needs work
StatusFileSize
new2.42 KB
new2.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

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

Status:Needs work» Needs review
StatusFileSize
new377.51 KB
FAILED: [[SimpleTest]]: [MySQL] 55,212 pass(es), 31 fail(s), and 32 exception(s).
[ View ]

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new378.6 KB
FAILED: [[SimpleTest]]: [MySQL] 55,169 pass(es), 74 fail(s), and 27 exception(s).
[ View ]
new2.69 KB

Adds the update deps from #154

StatusFileSize
new377.65 KB
FAILED: [[SimpleTest]]: [MySQL] 55,570 pass(es), 2 fail(s), and 77 exception(s).
[ View ]
new712 bytes

Makes dep optional

StatusFileSize
new377.35 KB
FAILED: [[SimpleTest]]: [MySQL] 55,662 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
new1.16 KB

Reverses logic
@andypost++

StatusFileSize
new376.3 KB
PASSED: [[SimpleTest]]: [MySQL] 55,951 pass(es).
[ View ]
new385.12 KB
PASSED: [[SimpleTest]]: [MySQL] 55,670 pass(es).
[ View ]
new5.63 KB

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

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

StatusFileSize
new17.87 KB
new408.34 KB
FAILED: [[SimpleTest]]: [MySQL] 55,889 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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

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

StatusFileSize
new377.93 KB
PASSED: [[SimpleTest]]: [MySQL] 56,213 pass(es).
[ View ]
new823 bytes

Reverting 4ec8332 broke file access

StatusFileSize
new383.87 KB
FAILED: [[SimpleTest]]: [MySQL] 55,337 pass(es), 25 fail(s), and 25 exception(s).
[ View ]
new18.4 KB

StatusFileSize
new381.87 KB
FAILED: [[SimpleTest]]: [MySQL] 55,361 pass(es), 26 fail(s), and 25 exception(s).
[ View ]

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.
  */

StatusFileSize
new381.95 KB
FAILED: [[SimpleTest]]: [MySQL] 56,358 pass(es), 27 fail(s), and 18 exception(s).
[ View ]
new4.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();

StatusFileSize
new381.98 KB
PASSED: [[SimpleTest]]: [MySQL] 55,970 pass(es).
[ View ]
new2.37 KB

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

StatusFileSize
new383.69 KB
PASSED: [[SimpleTest]]: [MySQL] 56,400 pass(es).
[ View ]
new5.18 KB

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

+++ 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()

StatusFileSize
new385.41 KB
FAILED: [[SimpleTest]]: [MySQL] 56,005 pass(es), 10 fail(s), and 2 exception(s).
[ View ]
new11.91 KB

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

StatusFileSize
new386.13 KB
FAILED: [[SimpleTest]]: [MySQL] 57,700 pass(es), 48 fail(s), and 95 exception(s).
[ View ]
new1.3 KB

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

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)

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

@andypost - I have this bit done locally

pushed
b08132e Move comment_node_redirect to a route to sandbox

Where will the module exist for nodes happen?

StatusFileSize
new388.38 KB
PASSED: [[SimpleTest]]: [MySQL] 56,175 pass(es).
[ View ]
new2.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,

StatusFileSize
new393.28 KB
PASSED: [[SimpleTest]]: [MySQL] 56,238 pass(es).
[ View ]
new7.95 KB

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?

StatusFileSize
new935 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

StatusFileSize
new1.04 KB
new389.36 KB
PASSED: [[SimpleTest]]: [MySQL] 56,222 pass(es).
[ View ]

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')): ?>

StatusFileSize
new390.38 KB
PASSED: [[SimpleTest]]: [MySQL] 56,190 pass(es).
[ View ]
new1.02 KB

Converted

StatusFileSize
new391.01 KB
FAILED: [[SimpleTest]]: [MySQL] 55,137 pass(es), 143 fail(s), and 5,200 exception(s).
[ View ]

Another merge patch

StatusFileSize
new391.28 KB
PASSED: [[SimpleTest]]: [MySQL] 55,849 pass(es).
[ View ]
new11.61 KB

merge for language

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

StatusFileSize
new395.51 KB
FAILED: [[SimpleTest]]: [MySQL] 44,831 pass(es), 549 fail(s), and 403 exception(s).
[ View ]

StatusFileSize
new394.33 KB
FAILED: [[SimpleTest]]: [MySQL] 57,365 pass(es), 10 fail(s), and 7 exception(s).
[ View ]
new887 bytes

should fix most of tests

StatusFileSize
new389.57 KB
FAILED: [[SimpleTest]]: [MySQL] 56,292 pass(es), 0 fail(s), and 4 exception(s).
[ View ]
new6.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()

<?php
 
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()

StatusFileSize
new389.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sandbox-merge-205.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

only a ModulesDisabledUpgradePathTest.php because of forum_enable() broken

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?

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

StatusFileSize
new2.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

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

Status:Needs work» Needs review
StatusFileSize
new389.67 KB
PASSED: [[SimpleTest]]: [MySQL] 56,411 pass(es).
[ View ]
new1018 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

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

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

yay, green!

StatusFileSize
new389.13 KB
PASSED: [[SimpleTest]]: [MySQL] 55,531 pass(es).
[ View ]

StatusFileSize
new389.13 KB
PASSED: [[SimpleTest]]: [MySQL] 55,983 pass(es).
[ View ]

and again

StatusFileSize
new389.19 KB
PASSED: [[SimpleTest]]: [MySQL] 55,690 pass(es).
[ View ]

Another merge

StatusFileSize
new385.76 KB
FAILED: [[SimpleTest]]: [MySQL] 57,314 pass(es), 44 fail(s), and 49 exception(s).
[ View ]

Another merge

StatusFileSize
new385.25 KB
FAILED: [[SimpleTest]]: [MySQL] 57,941 pass(es), 12 fail(s), and 13 exception(s).
[ View ]
new4.23 KB

Fix upgrade path

StatusFileSize
new385.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sandbox-merge-223.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new403.12 KB

fix some notices after UserNG
Comment preview still broken

StatusFileSize
new385.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sandbox-merge-224.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new594 bytes

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

StatusFileSize
new385.34 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sandbox-merge-225.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new2.39 KB

Finally fix reply notices

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

StatusFileSize
new385.37 KB
PASSED: [[SimpleTest]]: [MySQL] 56,306 pass(es).
[ View ]

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

StatusFileSize
new386.2 KB
PASSED: [[SimpleTest]]: [MySQL] 56,481 pass(es).
[ View ]

test

StatusFileSize
new386.27 KB
PASSED: [[SimpleTest]]: [MySQL] 57,908 pass(es).
[ View ]
new2.26 KB

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

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

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

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.

StatusFileSize
new386.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-731724.helper.236.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

StatusFileSize
new388.78 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-731724.238.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new3.21 KB

Missed the interface changes

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.

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

Status:Needs work» Needs review
StatusFileSize
new387.8 KB
FAILED: [[SimpleTest]]: [MySQL] 56,551 pass(es), 422 fail(s), and 1,403 exception(s).
[ View ]

We're getting close to the 300 mark here too

StatusFileSize
new386.02 KB
FAILED: [[SimpleTest]]: [MySQL] 57,366 pass(es), 16 fail(s), and 151 exception(s).
[ View ]
new8.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 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.)

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

StatusFileSize
new385.74 KB
PASSED: [[SimpleTest]]: [MySQL] 58,010 pass(es).
[ View ]
new679 bytes

Other comment tests fixed

StatusFileSize
new387.66 KB
FAILED: [[SimpleTest]]: [MySQL] 58,261 pass(es), 17 fail(s), and 0 exception(s).
[ View ]

re-roll against HEAD

StatusFileSize
new1.3 KB
new388.7 KB
FAILED: [[SimpleTest]]: [MySQL] 58,770 pass(es), 1 fail(s), and 3 exception(s).
[ View ]

Lets see if this fixes the fails

StatusFileSize
new389.75 KB
PASSED: [[SimpleTest]]: [MySQL] 58,320 pass(es).
[ View ]
new1.04 KB

Fixes views tests

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.

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?

StatusFileSize
new387.72 KB
FAILED: [[SimpleTest]]: [MySQL] 58,342 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new616 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

StatusFileSize
new390.07 KB
PASSED: [[SimpleTest]]: [MySQL] 58,867 pass(es).
[ View ]
new832 bytes
new2.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

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?

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

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

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.

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?

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.

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.

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.

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.

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.

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.

  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?

+++ 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!

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?

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

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

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

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

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

StatusFileSize
new387.53 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Merged HEAD

StatusFileSize
new389.83 KB
FAILED: [[SimpleTest]]: [MySQL] 58,122 pass(es), 39 fail(s), and 100 exception(s).
[ View ]
new13.28 KB

StatusFileSize
new390.11 KB
FAILED: [[SimpleTest]]: [MySQL] 58,386 pass(es), 35 fail(s), and 162 exception(s).
[ View ]
new3.39 KB

Fixes use of default values and left-over from #2024867: Rename translation_entity to content_translation
Plus chases HEAD

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

Status:Needs work» Needs review
StatusFileSize
new388.38 KB
FAILED: [[SimpleTest]]: [MySQL] 56,898 pass(es), 34 fail(s), and 112 exception(s).
[ View ]
new3.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

Status:Needs review» Needs work
StatusFileSize
new388.89 KB
FAILED: [[SimpleTest]]: [MySQL] 56,819 pass(es), 124 fail(s), and 210 exception(s).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new388.98 KB
FAILED: [[SimpleTest]]: [MySQL] 56,634 pass(es), 27 fail(s), and 126 exception(s).
[ View ]

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

StatusFileSize
new389.87 KB
FAILED: [[SimpleTest]]: [MySQL] 56,643 pass(es), 27 fail(s), and 3 exception(s).
[ View ]
new5.03 KB

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

StatusFileSize
new385.98 KB
FAILED: [[SimpleTest]]: [MySQL] 56,855 pass(es), 26 fail(s), and 3 exception(s).
[ View ]
new3.42 KB

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

PS: need some sleep

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.

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

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.

Status:Needs work» Needs review
StatusFileSize
new389.19 KB
FAILED: [[SimpleTest]]: [MySQL] 56,955 pass(es), 1 fail(s), and 3 exception(s).
[ View ]

StatusFileSize
new397.22 KB
FAILED: [[SimpleTest]]: [MySQL] 57,219 pass(es), 4 fail(s), and 1 exception(s).
[ View ]
new6.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

StatusFileSize
new396.82 KB
FAILED: [[SimpleTest]]: [MySQL] 56,986 pass(es), 3 fail(s), and 3 exception(s).
[ View ]
new6.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

StatusFileSize
new397.78 KB
FAILED: [[SimpleTest]]: [MySQL] 57,332 pass(es), 22 fail(s), and 2 exception(s).
[ View ]
new3.91 KB

Added CommentManager

StatusFileSize
new405.77 KB
FAILED: [[SimpleTest]]: [MySQL] 56,752 pass(es), 13 fail(s), and 1 exception(s).
[ View ]
new3.91 KB

Another set of fixed tests

StatusFileSize
new404.11 KB
FAILED: [[SimpleTest]]: [MySQL] 56,778 pass(es), 10 fail(s), and 3 exception(s).
[ View ]

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'];

StatusFileSize
new404.25 KB
FAILED: [[SimpleTest]]: [MySQL] 57,221 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
new1.84 KB

Proper fix and rdf now works

StatusFileSize
new406.8 KB
FAILED: [[SimpleTest]]: [MySQL] 57,574 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
new3 KB

A bit more fixed tests

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?

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

Status:Needs work» Needs review
StatusFileSize
new1.58 KB
new807.07 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to clear checkout directory.
[ View ]

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.

Status:Needs review» Needs work

thanks @linclark, will refactor this into the sandbox

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

Status:Needs work» Needs review
StatusFileSize
new407.26 KB
FAILED: [[SimpleTest]]: [MySQL] 56,571 pass(es), 637 fail(s), and 265 exception(s).
[ View ]
new791 bytes

Merges head and attempts to fix the EnableDisable test

StatusFileSize
new407.37 KB
FAILED: [[SimpleTest]]: [MySQL] 57,535 pass(es), 7 fail(s), and 2 exception(s).
[ View ]
new3.55 KB

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

StatusFileSize
new408.31 KB
FAILED: [[SimpleTest]]: [MySQL] 57,563 pass(es), 0 fail(s), and 33 exception(s).
[ View ]
new4.34 KB

Fixes the failing tests locally.

StatusFileSize
new408.47 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new1.27 KB

This time?

StatusFileSize
new408.46 KB
PASSED: [[SimpleTest]]: [MySQL] 57,709 pass(es).
[ View ]
new871 bytes

Indent fail

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

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

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

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

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.

StatusFileSize
new408.92 KB
PASSED: [[SimpleTest]]: [MySQL] 57,832 pass(es).
[ View ]
new1.47 KB

Addresses #337 comment 2
Rerolls against HEAD

StatusFileSize
new408.73 KB
FAILED: [[SimpleTest]]: [MySQL] 55,545 pass(es), 1,027 fail(s), and 634 exception(s).
[ View ]

StatusFileSize
new408.95 KB
FAILED: [[SimpleTest]]: [MySQL] 58,101 pass(es), 3 fail(s), and 5 exception(s).
[ View ]
new2.54 KB

Some typehint fails

Status:Needs review» Needs work

working on the fails

Status:Needs work» Needs review
StatusFileSize
new409.89 KB
FAILED: [[SimpleTest]]: [MySQL] 58,236 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new5.02 KB

Failing tests pass locally, merges HEAD

StatusFileSize
new409.9 KB
PASSED: [[SimpleTest]]: [MySQL] 58,196 pass(es).
[ View ]
new711 bytes

stupid copy paste fail

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.

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?

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.

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

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

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:

<?php
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.

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.

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.

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.

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

<