Posted by andypost on February 5, 2013 at 12:32am
20 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | comment.module |
| Category: | task |
| Priority: | normal |
| Assigned: | andypost |
| Status: | needs work |
Issue Summary
Ignore this one for reviews, this is just to post new periodic patches for the conversion of comment to field
Original issue #731724: Decouple comment.module from node by turning comment settings into a field
Related issues:
#1900132: [Meta] Comment as field
#1901110: Improve the UX for comment bundle pages and comment field settings
#1029708: History table for any entity
#1919002: Upgrade to D8 broken when D7 has more then one language enabled
#1922034: UpgradePathTestBase does not detect "An unrecoverable error has occurred" install errors.
Comments
#1
#2
#3
Fixed some tests
#5
The last submitted patch, 3.patch, failed testing.
#6
Another fixes
#7
The last submitted patch, 5.patch, failed testing.
#8
#9
The last submitted patch, 7.patch, failed testing.
#10
Main branch tests
#11
The last submitted patch, 8.patch, failed testing.
#12
Another round to move settings to widget
#13
The last submitted patch, comment-widget.patch, failed testing.
#14
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
#15
The last submitted patch, comment-widget.patch, failed testing.
#16
Testing of sandbox code
#17
The last submitted patch, 16-sandbox.patch, failed testing.
#18
Filed #1916556-1: Random test failures - The string cannot be saved with current sandbox code, but patch passed the tests
#19
Commited schema changes for proper length of keys
#20
my patch with formatters. fixed formatter and re-roll for HEAD
#21
Patch against sandbox - adds formatter and widget + upgrade path for entity display
#22
The last submitted patch, 20.patch, failed testing.
#23
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
#24
Current sandbox after merge
#25
Current sandbox code
#26
The last submitted patch, sandbox.patch, failed testing.
#27
Used a way like
user_update_8011()#28
The last submitted patch, comment-upgrade-config.patch, failed testing.
#29
#30
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
#31
Let's try
#32
The last submitted patch, comment-31.patch, failed testing.
#33
- fix joins for the last_comment_name field/sort
- try to get field_view_field working with entityNG (possible not the best workaround)
#34
New patch based on #33 with updates from Berdir's latest review in the main issue.
#35
The last submitted patch, 731724-comment-decouple-326.patch, failed testing.
#36
Reverted back result of
comment_field_name_load()used for field_ui to return a bundle name stringEDIT 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
#37
The last submitted patch, 731724-comment-decouple-327.patch, failed testing.
#38
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
#39
Address #731724-327: Decouple comment.module from node by turning comment settings into a field
#40
The last submitted patch, 39.patch, failed testing.
#41
changes for #731724-333: Decouple comment.module from node by turning comment settings into a field
#42
current sandbox
#43
The last submitted patch, 44.patch, failed testing.
#44
#42: 44.patch queued for re-testing.
#45
The last submitted patch, 44.patch, failed testing.
#46
Fixed remains of ER convertion
current diffstat
105 files changed, 3276 insertions(+), 1425 deletions(-)#47
Current sandbox.. minimized patch
105 files changed, 3243 insertions(+), 1416 deletions(-)#48
The last submitted patch, 731724-comment-decouple-338.patch, failed testing.
#49
and again
#50
I bit of more clean-up in touched doc-blocks
Also testing "m" minimized (-M25%) git option
#51
The last submitted patch, 731724-comment-decouple-339.patch, failed testing.
#52
let's re-test sandbox
#53
The last submitted patch, sandbox.patch, failed testing.
#54
Fixes for #731724-348: Decouple comment.module from node by turning comment settings into a field
#55
The last submitted patch, sandbox-54.patch, failed testing.
#56
#57
The last submitted patch, sandbox-56.patch, failed testing.
#58
merged HEAD
#59
The last submitted patch, sandbox-58.patch, failed testing.
#60
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 pageHowever, 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\DatabaseExceptionWrapperThe 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.
#61
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
)
)
)
'
#62
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?
#63
Let's test the patch with included hunk from #1922034: UpgradePathTestBase does not detect "An unrecoverable error has occurred" install errors.
I still sure that we affected by #1919002: Upgrade to D8 broken when D7 has more then one language enabled
#64
Let's see, seems data in upgrade broken
#65
The last submitted patch, sandbox-64.patch, failed testing.
#66
Both patches
#67
The last submitted patch, sandbox-66.patch, failed testing.
#68
Let's see if
get_t()removed#69
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
#70
Great work here @andypost
#71
@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
#72
woot!
Moving over to main issue at @andpost's request
#73
Merged HEAD
#74
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
#75
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
#76
@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
#77
Initial patch for formatters
#78
The last submitted patch, formatters.patch, failed testing.
#79
small fixes
#80
The last submitted patch, formatters2.patch, failed testing.
#81
fix upgrade path. Links still need some work
#82
The last submitted patch, formatters3.patch, failed testing.
#83
HEAD was broken
#84
The last submitted patch, formatters4.patch, failed testing.
#85
last one for today, should work except rdf and search
#86
The last submitted patch, formatters5.patch, failed testing.
#87
Addressed #731724-341: Decouple comment.module from node by turning comment settings into a field
Moving instance settings from nested 'comment' to first level
#88
Some clean-up to minimize patch size
#89
Another round on formatters
#90
The last submitted patch, formatters6.patch, failed testing.
#91
Reverted back comment_node_view() for rss, changed rdf and merged current sandbox
#92
The last submitted patch, formatters6.patch, failed testing.
#93
merging head
#94
+++ 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. Usedrupal_container()->get('plugin.manager.entity')->getDefinitions()http://drupal.org/node/1929006
All other looks good
#95
Merges HEAD, fixes comment #731724-384: Decouple comment.module from node by turning comment settings into a field from @berdir and #94
#96
@andypost - patch at #95 based on sandbox HEAD - is that up to date? Size of patch has dropped.
#97
The last submitted patch, decouple-comment-node-731724.385.patch, failed testing.
#98
Somehow Annotation now lives in Component
#99
The last submitted patch, sandbox-98.patch, failed testing.
#100
Locally the tests are passed
#98: sandbox-98.patch queued for re-testing.
#101
Green
#102
testing changes before commit
#103
The last submitted patch, sandbox-merge-102.patch, failed testing.
#104
Pushed this commit, testing sandbox
#105
The last submitted patch, sandbox-merge-104.patch, failed testing.
#106
Changed
entity_get_info()to useDrupal::service()notation also removed some unused callsFailed 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.phpinterdiff http://pastebin.com/pa1jz0BW
#107
The last submitted patch, sandbox-merge-106.patch, failed testing.
#108
Merge after #1818556: Convert nodes to the new Entity Field API
#109
The last submitted patch, sandbox-merge-108.patch, failed testing.
#110
Fixed broken tests except views handlers
#111
The last submitted patch, sandbox-merge-110.patch, failed testing.
#112
This fixes 'entity_id', 'entity_type' and 'field_name' but NodeNewComments still broken - views wont join statistics table
#113
The last submitted patch, comment-views.patch, failed testing.
#114
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
#115
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
#116
Incorrect reference to $table in 115.
#117
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
#118
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.
#119
Patch at 116 pushed to sandbox, will file new issue for views changes
#120
Follow up is here #1942660: Add a method to allow modules to clear the \Drupal\views\ViewsDataCache->storage during run-time
#121
Chasing HEAD
#122
Chasing HEAD
#123
New patch with upgrade path
#124
The last submitted patch, 731724-upgrade-path.patch, failed testing.
#125
Sanbox re-test
#126
The last submitted patch, sandbox-merge-125.patch, failed testing.
#127
Pushed changes according #1732730: [meta] Implement the new entity field API for all field types
#128
Pushed follow-up commit for
/**- * Defines the 'test_field' entity field item.
+ * Defines the 'comment_field' entity field item.
*/
#129
The last submitted patch, 731724-upgrade-path.patch, failed testing.
#130
And now in proper place
#131
The last submitted patch, sandbox-merge-130.patch, failed testing.
#132
Looks like comment_entity_load() is broken, so no comment_statistics attached and no Hidden option availiable
#133
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)
#134
Does someone know why the interdiff fails?
#135
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()andcomment_entity_insert()#136
The last submitted patch, sandbox-merge-M25-135.patch, failed testing.
#137
Needs re-roll after #1793696: views_preprocess_node check for the wrong row_plugin
#138
Let's see how much broken for last merge
#139
The last submitted patch, sandbox-merge-138.patch, failed testing.
#140
Fixes plugin -> pluginid issues
#141
The last submitted patch, decouple-comment-node-731724.helper.140.patch, failed testing.
#142
Another merge
#143
The last submitted patch, sandbox-merge-142.patch, failed testing.
#144
Fix some broken tests
#145
The last submitted patch, sandbox-merge-144.patch, failed testing.
#146
@dawehner++
#147
The last submitted patch, sandbox-merge-146.patch, failed testing.
#148
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
#149
The last submitted patch, sandbox-merge-148.patch, failed testing.
#150
As fields now entities - the easiest way to add right dependency
#151
The last submitted patch, sandbox-merge-150.patch, failed testing.
#152
Fix random failures
#153
The last submitted patch, sandbox-merge-151.patch, failed testing.
#154
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.phpLooks similar to 2) but needs to remove node relation
#155
Why are you overriding getValue() in CommentEntityItem ? It's not supposed to be computed, is it?
#156
Chases head, still to-do's as per #154 and any test fails
#157
The last submitted patch, decouple-comment-node-731724.helper.156.patch, failed testing.
#158
So looks like same fails as above plus the upgrade issues identified at #154
#159
Adds the update deps from #154
#160
The last submitted patch, decouple-comment-node-731724.helper.159.patch, failed testing.
#161
Makes dep optional
#162
Reverses logic
@andypost++
#163
The last submitted patch, decouple-comment-node-731724.162.patch, failed testing.
#164
Fix broken tests
git show 5a22142..73ed66a > interdiff.txtPS decouple-comment-node-731724.164_.patch just same patch with -M25%
#165
Reverts 4ec8332
Also xhprof output is attached:
Before:
Total Incl. Wall Time (microsec): 5,834,274 microsecsTotal 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 microsecsTotal 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
#166
Note xhprof output is for node view with 100 comments and per-page set to 150 (there is no 100 option)
#167
The last submitted patch, decouple-comment-node-731724.helper.165.patch, failed testing.
#168
#165: decouple-comment-node-731724.helper.165.patch queued for re-testing.
#169
Reverting 4ec8332 broke file access
#170
Fixes for #1900132-16: [Meta] Comment as field
#171
Merge for Issue #1043198: Change notice: 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.
*/
#172
The last submitted patch, sandbox-merge-171.patch, failed testing.
#173
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();
#174
The last submitted patch, sandbox-merge-173.patch, failed testing.
#175
Fix the rest broken tests. I think /manage is great improvement so allowing contrib to attach own tabs to comment bundles
#176
Another clean-ups
- form alter
- bundle stub-page
#177
+++ 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()
#178
Fixed part of 177, also changed type-hitting to CommentInterface
#179
The last submitted patch, sandbox-merge-178.patch, failed testing.