Ignore this one for reviews, this is just to post new periodic patches for the conversion of comment to field
Original issue #731724: Convert comment settings into a field to make them work with CMI and non-node entities
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.
Comment | File | Size | Author |
---|---|---|---|
#386 | interdiff.txt | 6.24 KB | andypost |
#386 | sandbox-merge-386.patch | 419.06 KB | andypost |
#383 | interdiff_type.txt | 3.8 KB | andypost |
#383 | interdiff.txt | 32.02 KB | andypost |
#383 | sandbox-merge-383.patch | 419.35 KB | andypost |
Comments
Comment #1
andypostComment #3
andypostFixed some tests
Comment #6
andypostAnother fixes
Comment #8
andypostComment #10
andypostMain branch tests
Comment #12
andypostAnother round to move settings to widget
Comment #14
andypostFixed some tests, just moved other settings to widget in tests
can't reproduce upgrade tests failures locally so probably depends on php version
Comment #16
andypostTesting of sandbox code
Comment #18
andypostFiled #1916556-1: Random test failures - The string cannot be saved with current sandbox code, but patch passed the tests
Comment #19
andypostCommited schema changes for proper length of keys
Comment #20
andypostmy patch with formatters. fixed formatter and re-roll for HEAD
Comment #21
andypostPatch against sandbox - adds formatter and widget + upgrade path for entity display
Comment #23
andypostSo 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
Comment #24
andypostCurrent sandbox after merge
Comment #25
andypostCurrent sandbox code
Comment #27
andypostUsed a way like
user_update_8011()
Comment #29
andypostComment #30
andypostNext round should be:
- fix views (required for RTBC)
- Move instance settings to formatter settings
- get rid of hook_entity_view()
Not sure it's needed
Comment #31
andypostLet's try
Comment #33
andypost- fix joins for the last_comment_name field/sort
- try to get field_view_field working with entityNG (possible not the best workaround)
Comment #34
dixon_New patch based on #33 with updates from Berdir's latest review in the main issue.
Comment #36
andypostReverted 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
Comment #38
andypostStarted 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
Comment #39
andypostAddress #731724-327: Convert comment settings into a field to make them work with CMI and non-node entities
Comment #41
andypostchanges for #731724-333: Convert comment settings into a field to make them work with CMI and non-node entities
Comment #42
andypostcurrent sandbox
Comment #46
andypostFixed remains of ER convertion
current diffstat
105 files changed, 3276 insertions(+), 1425 deletions(-)
Comment #47
andypostCurrent sandbox.. minimized patch
105 files changed, 3243 insertions(+), 1416 deletions(-)
Comment #49
andypostand again
Comment #50
andypostI bit of more clean-up in touched doc-blocks
Also testing "m" minimized (-M25%) git option
Comment #52
andypostlet's re-test sandbox
Comment #54
andypostFixes for #731724-348: Convert comment settings into a field to make them work with CMI and non-node entities
Comment #56
andypostComment #58
andypostmerged HEAD
Comment #60
jthorson CreditAttribution: jthorson commentedOn 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\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.
Comment #60.0
jthorson CreditAttribution: jthorson commentedadded related
Comment #61
jthorson CreditAttribution: jthorson commentedCaptured the string(s) tripping the StringStorageException in StringDatabaseStorage.php line 513:
Comment #62
jthorson CreditAttribution: jthorson commentedFrom 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?
Comment #63
andypostLet'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
Comment #64
andypostLet's see, seems data in upgrade broken
Comment #66
andypostBoth patches
Comment #68
andypostLet's see if
get_t()
removedComment #69
yched CreditAttribution: yched commentedThe comment needs to be updated accordingly :-p. Same in node.tpl.php
Comment #70
larowlanGreat work here @andypost
Comment #71
andypost@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
Comment #72
larowlanwoot!
Moving over to main issue at @andpost's request
Comment #73
andypostMerged HEAD
Comment #74
podarokfew tests with #73
15 comments without tree
10 comments per page
second page for testing
cache disabled
ab third run for drupal warm-up
without patch
looks like we have no performance regression
Comment #75
andypostSee 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
Comment #76
andypost@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
Comment #77
andypostInitial patch for formatters
Comment #79
andypostsmall fixes
Comment #81
andypostfix upgrade path. Links still need some work
Comment #83
andypostHEAD was broken
Comment #85
andypostlast one for today, should work except rdf and search
Comment #87
andypostAddressed #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
Comment #88
andypostSome clean-up to minimize patch size
Comment #89
andypostAnother round on formatters
Comment #91
andypostReverted back comment_node_view() for rss, changed rdf and merged current sandbox
Comment #93
andypostmerging head
Comment #94
podarokentity_get_info()
deprecated. Usedrupal_container()->get('plugin.manager.entity')->getDefinitions()
http://drupal.org/node/1929006
All other looks good
Comment #95
larowlanMerges HEAD, fixes comment #731724-384: Convert comment settings into a field to make them work with CMI and non-node entities from @berdir and #94
Comment #96
larowlan@andypost - patch at #95 based on sandbox HEAD - is that up to date? Size of patch has dropped.
Comment #98
andypostSomehow Annotation now lives in Component
Comment #100
andypostLocally the tests are passed
#98: sandbox-98.patch queued for re-testing.
Comment #101
larowlanGreen
Comment #102
andyposttesting changes before commit
Comment #104
andypostPushed this commit, testing sandbox
Comment #106
andypostChanged
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
Comment #108
andypostMerge after #1818556: Convert nodes to the new Entity Field API
Comment #110
andypostFixed broken tests except views handlers
Comment #112
andypostThis fixes 'entity_id', 'entity_type' and 'field_name' but NodeNewComments still broken - views wont join statistics table
Comment #114
larowlanThis 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
Comment #115
larowlanCommitted 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
Comment #116
larowlanIncorrect reference to $table in 115.
Comment #117
andypostAwesome! @larowlan++
Suppose this change should be filed as separate issue. As increased DX
Comment #118
dawehnerIf 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.
Comment #119
larowlanPatch at 116 pushed to sandbox, will file new issue for views changes
Comment #120
larowlanFollow up is here #1942660: Add a method to allow modules to clear the \Drupal\views\ViewsDataCache->storage during run-time
Comment #121
larowlanChasing HEAD
Comment #122
larowlanChasing HEAD
Comment #123
andypostNew patch with upgrade path
Comment #125
andypostSanbox re-test
Comment #127
andypostPushed changes according #1732730: [meta] Implement the new entity field API for all field types
Comment #128
andypostPushed follow-up commit for
Comment #130
andypostAnd now in proper place
Comment #132
andypostLooks like comment_entity_load() is broken, so no comment_statistics attached and no Hidden option availiable
Comment #133
andypostHere'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)
Comment #134
dawehnerDoes someone know why the interdiff fails?
Comment #135
andyposthttp://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()
Comment #137
andypostNeeds re-roll after #1793696: views_preprocess_node check for the wrong row_plugin
Comment #138
andypostLet's see how much broken for last merge
Comment #140
larowlanFixes plugin -> pluginid issues
Comment #142
andypostAnother merge
Comment #144
andypostFix some broken tests
Comment #146
andypost@dawehner++
Comment #148
andypostShould be green. Upgrade tests are fixed
Commits list
Interdiff to show last commit (debug does not included - just to point why === changed to ==)
$mode always returns as string value
Comment #150
andypostAs fields now entities - the easiest way to add right dependency
Comment #152
andypostFix random failures
Comment #154
andypostCurrent 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
Comment #155
fagoWhy are you overriding getValue() in CommentEntityItem ? It's not supposed to be computed, is it?
Comment #156
larowlanChases head, still to-do's as per #154 and any test fails
Comment #158
larowlanSo looks like same fails as above plus the upgrade issues identified at #154
Comment #159
larowlanAdds the update deps from #154
Comment #161
larowlanMakes dep optional
Comment #162
larowlanReverses logic
@andypost++
Comment #164
andypostFix broken tests
git show 5a22142..73ed66a > interdiff.txt
PS decouple-comment-node-731724.164_.patch just same patch with -M25%
Comment #165
larowlanReverts 4ec8332
Also xhprof output is attached:
Before:
After:
Comment #166
larowlanNote xhprof output is for node view with 100 comments and per-page set to 150 (there is no 100 option)
Comment #169
larowlanReverting 4ec8332 broke file access
Comment #170
larowlanFixes for #1900132-16: [Meta] Comment as field
Comment #171
andypostMerge for Issue #1043198: Convert view modes to ConfigEntity
also commited
Comment #173
andypostFix routing with stab in hook_menu()
Normalize path to common
comments/manage/{bundle}
Changed deprecated ControllerInterface
Also fix upgrade path tests
Comment #175
andypostFix the rest broken tests. I think /manage is great improvement so allowing contrib to attach own tabs to comment bundles
Comment #176
andypostAnother clean-ups
- form alter
- bundle stub-page
Comment #177
alexpottModule_exists is deprecated... use Drupal::moduleHandler() instead
This has happened see #1696660: Add an entity access API for single entity access
yay!
get only accepts a key...
should be something like
$maintain_entity_statistics = state()->get('comment.maintain_entity_statistics') ?: TRUE;
Shouldn't we be using the config system here...
deprecated in favour of the module handler if the entity module has been enabled at this point
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...This is still referenced in node.api.php
Why? This comment could do with some more meat... I would have thought other entity type might want to benefit from the functionality...
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?
Should be a route... and return a redirect response
Generic controller has landed and the needs more comments if it stays in like this..
Do we really need the strict type checking here?
... note to self... I've got to comment_new_page_count()
Comment #178
andypostFixed part of 177, also changed type-hitting to CommentInterface
Comment #180
andypostmerged head and reverted access check - node_access() works different then $node->access('view')
Comment #181
larowlanthis 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)
Comment #183
larowlan@andypost - I have this bit done locally
Comment #184
larowlanpushed
b08132e Move comment_node_redirect to a route
to sandboxComment #185
aspilicious CreditAttribution: aspilicious commentedWhere will the module exist for nodes happen?
Comment #186
andypostSuppose 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
Comment #187
larowlanPushed to sandbox
@alexpott can we get some more reviews?
Comment #188
andypostpushed 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
Comment #189
andyposttest merge after twig, still not fixed hunk for comment-wrapper
Comment #190
andypostConverted
Comment #193
andypostAnother merge patch
Comment #195
andypostmerge for language
Comment #199
andypostpatch is rtbc, could be re-rolled to primary issue
Comment #200
larowlanMerges after #1498674: Refactor node properties to multilingual went in
Comment #202
andypostshould fix most of tests
Comment #203
andypostFix block and minimize patch size
EDIT
Sandbox code now
24a23188 Fix SearchCommentTest
Currently only 2 tests are broken
HandlerAllTest.php
And ModulesDisabledUpgradePathTest.php because of
forum_enable()
throws
Comment #205
andypostonly a
ModulesDisabledUpgradePathTest.php
because offorum_enable()
brokenComment #207
larowlanWe 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?
Comment #208
andypost@larowlan now it's different - field_data_comment... table exists but no inactive field definition could be found
Comment #209
andypostThere'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
Comment #210
larowlanI'm offline for weekend but can't we use forum/config for the comment field instead of field_create_field
Comment #211
andypostIt 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
Comment #212
larowlanah Drupal commandment 6
Interdiff looks good to me, will move to main issue once bot returns
Comment #213
larowlanah Drupal commandment 6
Interdiff looks good to me, will move to main issue once bot returns
Comment #214
andypostyay, green!
Comment #215
andypostMerga after #1999322: Remove node_save() and node_delete()/node_delete_multiple() in favor of $node->save()/$node->delete()
Comment #218
andypostand again
Comment #219
andypostAnother merge
Comment #220
andypostAnother merge
Comment #221
andypostFix upgrade path
Comment #223
andypostfix some notices after UserNG
Comment preview still broken
Comment #224
andypostpreview now works, but
CommentUserTest
still throws some notice inuser_user_view()
Comment #225
andypostFinally fix reply notices
Comment #227
andypostMerge after #1999328: Remove comment_save() and comment_delete()/comment_delete_multiple() in favor of $comment->save()/$comment->delete()
Comment #228
andypostNode 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
Comment #229
andypostMerge after #1970360-94: Entities should define URI templates and standard links
I see no way to skip loading of the entity
Comment #230
andyposttest
Comment #231
andypostfiled #2020429: Cleanup comment views handlers docs
Comment #232
xjmAlright, 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. :)
Comment #233
xjmWhy isn't #1920042: Upgrade path changes part of the main issue?
Comment #234
andypostUpgrade 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 issueComment #235
xjmPersonally, 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.
Comment #236
larowlanRe-roll after #1893772: Move entity-type specific storage logic into entity classes
Comment #239
larowlanMissed the interface changes
Comment #240
xjmEdit: 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.
So, has this been removed entirely from
drupalCreateNode()
? Note to self: Check when we get there.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
.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:
New:
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.This seems like it should be a method on... something. Followup?
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.
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.)Previously, it wasn't necessary to require this relationship because there were no comments without the relationship. Makes sense.
I believe the latter two should be changed to match the first rather than using static calls?
Interesting that this test has all these dependencies. Also, why are we adding text as a module here?
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?
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.
Comment #241
larowlanthe 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
Comment #244
larowlanWe're getting close to the 300 mark here too
Comment #246
andypostFixed 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 sensefunction 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 back10. views integration was changed a lot - thanx to @dawehner
Comment #248
xjmWhat 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.)
Comment #249
xjmThe 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
Comment #250
andypostOther comment tests fixed
Comment #253
larowlanre-roll against HEAD
Comment #255
larowlanLets see if this fixes the fails
Comment #257
larowlanFixes views tests
Comment #258
xjmOut-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.
Comment #259
xjmSqueezing in a bit more review in DTW. Up through simpletest.
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?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.
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.
I wonder, should we file change notices against contrib Views for stuff like this?
Comment #260
andypostNew 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
Comment #262
andypostFix 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
Comment #263
mgiffordWow, 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?
Comment #264
andypost@mgifford Thanx a lot for review! Patch was RTBC many times but for now we are in process of getting technical reviews about code.
Comment #265
xjm@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...
Comment #266
xjmUp through node. There were actually way fewer changes in node itself than I expected. (Reminder, these are just notes for myself.)
Yay.
API change that should go in the change notice.
Out of scope: What the heckity heck is all that? BC stuff?
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.
Weird that this is defined in one views test... it's not even overriding a parent method or anything.
What is the point of this change? Merge artifact?
Out of scope: Single underscore separator? Is that a BC thing or by design?
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.)Hooray.
This should use a constant for promoted rather than a magic integer.
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.
Thank you, entity API.
Note to self: what is
Node::comment_statistics
and how is it different fromNode::comment_count
? Presumably as array or something since there can be multiple comment fields? Edit: Yes, that looks to be exactly the case.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.
Hmm, what's with the comment prefix and double-underscore separator here? Hopefully I'll find that somewhere in comment.module.
Nitpick: These comments needs to be rewrapped; the second line of the first and the second both look to be 82 characters or so.
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. :)
Wha, since when is this a field-level setting? Hunh.
Note to self to look at this function when I get up there. Looks like Yet Another Procedural Wrapper™ at first glance...
Makes sense; search only cares about nodes by default, and previously it was implicit that these were nodes.
Comment #267
xjmDone reading everything other than comment module itself. Yay.
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.
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. :)Why are we un-defining
$text_file
? This seems to have nothing to do with the patch.Why do we need to do this in
hook_install()
? The field presumably isn't defined incomment.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:Since forum (presumably) has comment as a dependency, comment should already be installed and available here.
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.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.Oy. We can't use a getter or something here?
Comment #268
xjmUp through
UserCommentTest
, wherein I got a little concerned about the testing situation: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.
Out of scope:
$modules
doesn't seem to be documented onWebTestBase
. Prolly we could@inheritdoc
all these in all the tests everywhere if it were...Our standards specify that
getInfo()
should not have a docblock. (Yeah, it's dumb and inconsistent.) See https://drupal.org/node/325974.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).
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.
Isn't this somewhat redundant with the
deleteComment()
method?Nitpick: Maybe this should be
getUnapprovedCommentBySubject()
?Nitpick: ID, not id. Also, articles. It should be "The comment ID."
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:
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.
Nitpicks: "Unpublish the comment", "Publish the comment", "Delete the comment".
The extra blank line in the middle of this (and the similar checks below) can probably be removed for better readability.
Nitpick: "Check that the comment was found."
The inline comment here is kind of useless and also misleading. Maybe "Deny anonymous users access to comments"?
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).
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).
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.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.)
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.
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?
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.
Nitpick: The assertion message here is both confusing and unnecessary. I'd just remove it; the default
assertText()
message is more specific and informative.Out of scope: This test class name should end in
Test
.This should use
$this->container
.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.
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:But, we're also setting a default here, initializing
$propertyDefinitions['status']
to beCOMMENT_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.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.
So, I need to double check in
template_preprocess_comment()
that these are correct.Comment #269
xjmRe: 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.
Comment #270
xjmRe:
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.Comment #271
xjmOkay this is whatever I wrote last night but did not save.
Should we be using
$this->container->get(field.info)
here instead offield_info_instance()
?...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 thecreateEntity()
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.
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.
Comment #272
xjmSo, 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 documentCommentItem
thoroughly.Comment #273
xjmReviewed the rest of the test changes. Yeagh.
Note to self: Re-review this test after testing the patch manually.
Ditto re:
FieldInfo
.What? Why? As far as I can tell reading
CommentTestBase
andCommentFieldsTest
, neither this method norCommentTestBase::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?Ah, I am glad we added this; that would be a lot of broken links. +1.
Mmm, why is the numbering changing here? I'll need to read this whole test in context too.
And why is the pager changing here?
Heh. I asked for upgrade path tests for this, but it sounds like we don't have complete coverage for it, period?
Again, is it possible to use methods here?
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.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.
Comment #274
xjmNitpick: This should start with a verb. ("Defines a...") http://drupal.org/node/1354#functions
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 aRouteSubscriberInterface
and/or aRouteSubscriberBase
and/or something along those lines to codify this pattern (e.g.,getSubscribedEvents()
seems to be the same for each of them, defining to theroutes()
or a similarly named callback.Edit: Plus, we should update the WSCCI conversion guide to cover this.
Nitpick: Should end with a period.
I don't think it's the entity type manager? Copypasta error?
This docblock is missing an
@param
, and is also a little thin. E.g., most of the routes for comment are already defined statically inhook_menu()
--here, we're fetching the existing ones adn then adding in an additional one if node is enabled. Correct?Comment #276
xjmAha. 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!Comment #277
xjmReviewed 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.
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.
Why
comment_default
? What's the "default" part for?Nitpick: I think per our standards this should be "Comment list" (sentence case).
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?
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.I don't understand why P implies Q.
So this actually seems more like the place that we are about the
search_result
andsearch_index
view modes... Aha, wait. Both this and the previous conditional are excluding those two view modes. So, rather than having to subsequentif
statements that check the view modes are not one of these two, let's nest them both in a separateif
checking the view modes. I think that can also go around theif (!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 followingin_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:
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.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?
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.
I hope this underscore-delimited magic is documented in our templates.
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?
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.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.
I don't like arbitrary weights, especially when there are no other weights specified in the widget form here. What's 30? Why 30?
Comment #278
xjmI wonder, should this path come from a method rather than us concatenating it together?
I wonder, do we need an
@throws
in this docblock forInvalidArgumentException
? I mean any function throws that exception. Or should we use a more specific exception?How about:
Load all the entities that have comments attached.
Comma splice. Just change the comma to a period and capitalize "This." :) Also, "per type" should not be hyphenated.
This should totally be a method. I guess this can be a followup.
"Use the created date of the entity if it's set, or default to REQUEST_TIME."
"Refactor" should be capitalized. :)
"Get the user ID from the entity if it's set, or default to the currently logged in user."
Nitpick: this needs rewrapping; the first line is 81 characters.
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. :)Missing period. Also,
AdminController
is a very vague and confusing class name. https://drupal.org/node/608152#namingAlso, 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.
Articlesssssss. the the the the the :)
What? No it doesn't.
This docblock would be better if it explained what the purpose of said overview was.
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."
"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.
Inline comment for this hunk would be great, e.g.:
Fetch a list of all comment fields.
Better inline commenting would be better here as well so I don't have to parse all of this in my brain.
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.
These should be sentence-cased (capital M).
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.A renderable array containing...
(and describe what it contains).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.
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...
Good, we have
FieldInfo
injected here. That reinforces to me that we should do so elsewhere as well (as per my earlier reviews).Nitpick: add an article.
Redirects legacy node links to the new path.
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:
NodeInterface
instead?The node object identified by the legacy URL.
Let's flesh out this comment a little, shall we? How about:
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 toEntityInterface
finally? In either case, that's irrelevant here. Just a note for myself. :)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.
Comments aren't attached to entity types, either. ;)
The entity type of the entity to which this comment is attached.
I guess the comment is actually attached to a field so I'll leave this one. ;)
Maybe: "...if this is a reply to another comment." (I realize this line already existed.
I realize this
@todo
was pre-existing, but is there an issue for it?I need to check again why we're unsetting all this stuff.
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. ;)Comment #279
xjm#1969728: Implement Field API "field types" as TypedData Plugins landed so there's rerolly stuff to be done here.
Comment #280
xjmI'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. :)So I'm thinking maybe we want a followup to revisit the way this works.
"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.
Remove should be capitalized.
Nitpick: This should end with parens.
So it used to be a variable and is now a per-instance setting? That's nice.
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?
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?What does that even mean?
Does this have test coverage, particularly for non-node entity types?
Hmmm.
Is there any reason this can't be a method on
Comment
?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.
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.
I guess we probably need test coverage to make sure the things that the cleanup from these hook implementations actually happens?
Should these be using (at least)
Drupal::cache()
? Should we actually be deleting stuff here, or just invalidating it?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.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.)
I guess it's out of scope for this patch to avoid making it even bigger, but
drupal_set_breadcrumb()
is deprecated.Nitpick: Needs to be wrapped.
Should we still be doing this?
This needs an
@throws
.Ah, so this is what those methods are for--they're page callbacks. Let's update the method documentation to clarify that.
"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.
Edit: Ah, here we go. So let's reword this comment to:
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 incomment_token_info()
?This change is both out of scope and wrong.
Pre-existing and out of scope, but is this really the correct case for this method name?
Entity ID. :)
Comment #281
xjmI've kind of reached my tolerance for this patch for now, so I'll revisit it more another day. :)
Comment #282
andypostCurrently 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]() callsAssigning 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
Comment #283
andypostMerged HEAD
Comment #285
larowlanMassive refactor after #1969728: Implement Field API "field types" as TypedData Plugins
Comment #287
larowlanFixes use of default values and left-over from #2024867: Rename translation_entity to content_translation
Plus chases HEAD
Comment #289
andypostHere'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
Comment #290
andypostFix some bugs.
Also after removing a comment field from entity and visiting
/admin/reports/fields
there's still noticescomment1(2) was fields added and later removed
I think we should drop fieldset wrapper around settings
Comment #292
andypostAnother round on sandbox code, merged all tremendous changes, Asked about default value in #2031203-4: Configurable fields should use applyDefaultValue()
Comment #295
andypostA bit more fixes, primary questionable is #2031203-4: Configurable fields should use applyDefaultValue()
Comment #297
andypostIntroduced
_update_comment_get_comment_fields()
to be used in updatesReplaced more deprecated calls to field-functions
Comment #299
andypostreverted changed test - see interdiff.txt
minimized noise in patch and more fixes for deprecated functions (in sandbox)
PS: need some sleep
Comment #301
Anonymous (not verified) CreditAttribution: Anonymous commentedWas just reviewing this when I was helping in debugging the RDF test, and I saw this:
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.
Comment #302
Anonymous (not verified) CreditAttribution: Anonymous commentedActually, I'm just realizing that I probably don't understand how comment statistics works... so my last comment can probably be ignored.
Comment #303
larowlanOpened 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.
Comment #304
andypostFixed a bunch of nitpicks. Tests should still fail waiting for #2031707: field_sync_field_status() does not enable fields defined without hook_field_info()
http://privatepaste.com/8d7eb58bfd to debug the upgrade test failures
So we have 2 critical bugs:
- CommentFieldsTest #2031707: field_sync_field_status() does not enable fields defined without hook_field_info()
- ModulesDisabledUpgradePathTest #2032369: _update_8003_field_create_field() relies on field schemas never changing and plugins being available - @berdir suggested #1958050-251: IGNORE: Patch testing issue ✌✌ (see
comment_install()
) approachComment #306
andypostSo 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 neededComment #308
andypostAnother 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
Comment #310
andypostAdded CommentManager
Comment #312
andypostAnother set of fixed tests
Comment #314
andypostRevert changes and test current state
Comment #316
andypostProper fix and rdf now works
Comment #317
andypostA bit more fixed tests
Comment #319
andypostIn #317 I drop unused
core/profiles/standard/config/rdf.mapping.comment.comment_node_page.yml
Currently we ship standard profile with
image
andtags
fields that added with config yml files - so the question:Should we drop this in favour of config yml files?
Comment #320
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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 :)
Comment #321
Anonymous (not verified) CreditAttribution: Anonymous commentedThis 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.
Comment #323
larowlanthanks @linclark, will refactor this into the sandbox
Comment #324
larowlanNote 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
Comment #325
larowlanMerges head and attempts to fix the EnableDisable test
Comment #327
larowlanSome $account->uid/$user->uid calls.
Expecting a few more, but lets see what the bot says.
Comment #329
larowlanFixes the failing tests locally.
Comment #331
larowlanThis time?
Comment #333
larowlanIndent fail
Comment #334
dsnopekHuzzah! 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:
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. :-)
Comment #335
micbar CreditAttribution: micbar commentedWow!
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
Comment #336
dsnopek@larowlan: Maybe it's time to take this patch back to the main issue for wider review and, hopefully, to be committed?
Comment #337
yched CreditAttribution: yched commentedNo 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().
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 ?
Comment #338
larowlanOk 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
Agreed, will sort during next re-roll.
Comment #339
larowlanAddresses #337 comment 2
Rerolls against HEAD
Comment #340
larowlanMerges HEAD and reverts chunk 1 of #337 now that #2048833: Prepare cache called on field items without checking if PrepareCacheInterface is implemented is in
Comment #342
larowlanSome typehint fails
Comment #344
larowlanworking on the fails
Comment #345
larowlanFailing tests pass locally, merges HEAD
Comment #347
larowlanstupid copy paste fail
Comment #348
xjmFirst reviewing the interdiffs, starting way back in #285. Editing the same comment as I go to save thread space and not lose any review.
Man, our terminology is so confusing... Not in scope. Just, man.
I don't quite understand this comment?
This needs to have a few more words to become an English sentence. ;)
Another comment that could use a little work. I'd say:
However, let's also explain why we need this safe-for-maintenance-mode function.
Nit: Extra blank line.
Issue is in; will double-check that this is removed in a later reroll.
Why are we arbitrarily using a magic number 10 here? Maybe this is just moved code? Did I comment on this already elsewhere?
Why in D9? Can we clarify? File a 9.x issue? ;)
Hm, why reverting this?
Followup issue?
Post an explicit followup and link it on #1939994: Complete conversion of nodes to the new Entity Field API?
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.
Comment #349
xjmDown 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.Why is the new JS so much smaller than the old JS?
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.
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.
db_select()
.Just the form? Not created entities? If so the hook name is kind of unintuitive.
Nit: There should probably be parens after
{%entity_type}_access()
.Nit: logged-in should be hyphenated.
These return values are a little confusing; why isn't there a constant for COMMENT_ACCESS_IGNORE or something?
This is in.
This was kind of mind-bending as an example; maybe add an inline comment that says:
Or something.
This is a pretty useless comment; let's replace with:
WOOO YEAH \m/
"...and its fields are inactive."
Okay, so this is one use of the update helper that has nothing to do with DB updates. Is there a better function name?
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.
comment_entity_statistics is a table name right? If so it should be in curlies. Also, the word "the" can be removed.
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.
Kinda useless comment. Better would be something like:
And better yet would be a little more explanation of what that means and/or an
@see
.Whatever I said above about these docs etc.
Yeah, the function name is misleading. This wouldn't be usable during the upgrade path because of the reliance on Entity API here.
Grammar nitpick: Comments are not replies to entity IDs or entity types. Try:
This is a little confusing too. Does it mean: "The field_name of the field that was used to add this comment."?
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).
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?
We're getting close to when these changes won't be allowed anymore (Beta 1), but not there yet.
Nitpick: We say "Update" instead of "Updates" because these docblocks are end-user-facing on
update.php
.These are some seriously heavy-duty upgrade hooks. I hope we have some upgrade path tests?
#1856972: config() isn't safe to use during minor updates if there's a change to configuration storage
Is this automatically doing the thing now where the module handler is the upgrade-path-safe one?
+1, this is the correct way to specify these. :)
Note to self: find this guy.
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.That's a tad vague. ;) What exisitng values does it update and how? Also, same nitpick about the silly s.
We've already migrated this data, right? Can we reference the hook that migrated it?
Comment #350
larowlanYeah 10 was pre-existing and is also used by taxonomy.
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.
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.
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.
Comment #351
xjmMmm, but #2018319: Remove field_read_field(s)() and field_read_instance(s) in favor of entity_load() and entity_load_multiple_by_properties().
Comment #352
larowlanMost 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.
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.
Agreed, this was on of our action points and will reflect so in the summary.
Yes
Everything else makes sense, will add to the spreadsheet
Comment #353
BerdirFocusing on NG stuff.
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.
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:
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.
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 ;)
Same as above.
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.
Comment #355
xjmIs there an open issue around this?
If we want to do that, let's do it in a separate patch.
Also sounds like we need some tests based on #353.
Comment #356
BerdirWe 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.
Comment #357
xjmThat 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.
Comment #358
BerdirAlso came up in a different issue, opened #2057999: Add argument to AccountInterface::getRoles() that allows to exclude the anonymous/authenticated roles to make that nicer.
Comment #359
alexpottI'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.
Should use update_variable_del()
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.
This new hook appears to be untested.
This function does not appear to be called
Is this still to do?
Misspelt compatability should be compatibility
Maybe we need to refactor this text some more.
user_access is deprecated
I can't find query_entity_access_alter() - does this actually do anything at this point?
not => no
What can we do to not use \StdClass?
We shouldn't be using global $user anymore
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.
Why? and this is very difficult to grok as an explanation.
Looking at the code for the function $values can sometime be an object.
field_info_instance is deprecated - lets inject the field.info service and whilst we're at it we can inject the entity manager too.
This is an integer
Needs to be type hinting on FieldInterface
user_access is deprecated so shouldn't be used in new code.
I think this line is unnecessary
It might be nice to adjust some the D7 comment variables so they are not all the default values
Deprecated
This review is half done but I'm on the move and don't want to lose where I am.
Comment #360
alexpottI think we should take the opportunity swap this out for entity_load_multiple to have one less wrapper.
module_exists is deprecated
Do we need to make this decision here? Or is this part of the history for any entity issue? Also "other entities" is technically incorrect since if the history module is not enabled this will include node too.
I think change this to
DEPRECATED: The node the comment was posted to.
because we plan to remove this in D9 and it has to stand out.The second comment is now redundant given the first.
So what are we going to do?
Let's link to the issue that is doing this.
Let's inject the entity manager here... we'll need to implement the EntityControllerInterface
And we can convert both of these to use the injected services too
Should have a linked issue.
Let's inject the entity manager too
SHould be using the already injected path generator service.
Should be $this->getFieldSettings();
Deprecated user_access
Let's inject the request by implementing ContainerFactoryPluginInterface and also inject the entity manager
Let's inject the entity manager - if possible.
field_info_field is deprecated
Don't we still have to test the node: to ensure the BC stuff works?
Very ugly. Disabled modules must die.
Unnecessary double quotes
node_comment_statistics is now entity_comment_statistics
In Drupal\Core\Entity\Annotation\EntityType. I don't think this is true anymore
Comment #361
alexpottEdit: removing duplicate
Comment #362
alexpottAnd last but not least.
In manual testing the issue I discovered the following functionality.
How to recreate:
This scenario was not really present before. The closest you could get to this was creating content and then enabling the comment module. And because the node's comment status defaults to 0 commenting is closed on all existing content. This would also occur if you only enabled comment after a while and then added a comment field to a node type. In the past regardless of the default value no old node would get a comment form. Now they do. Perhaps the desired behaviour would be to default to the the default value selected in the field settings.
There is some help text in
comment_help()
on this "(changes to the settings on existing content must be done manually)". Not sure it is sufficient.EDIT: and massive apologies if there is duplication with xjm's reviews above but I did this review offline and without access to here work.
Comment #363
larowlanI think we can make these a property of the commentitem fielditem. Eg $entity->some_comment_field->count.
Comment #364
larowlanif only we could track the review comments here https://github.com/drupal/drupal/pull/12/files
Comment #365
andyposttesting current merged sandbox
EDIT
still can't push
Also patch makes some fixes to track node and field module changes
Comment #367
andypostMerged and fixed most of all broken tests
Comment #369
BerdirThe search fails should be fixable by replacing $node->type in comment_node_update_index() with $node->getType(). The file field test should be somewhat similar, replace a node field with a method.
Comment #370
larowlanAdded #2078387: Add an EntityOwnerInterface in reference to #353
Comment #371
andypostOnly upgrade tests still broken, trying to fix
Added
CommentManager::getParentEntityUri()
helper to properly fix mergedDeleteForm
for comment (see interdiff_merge.txt)EDIT Fixed #353 in 79c8b0e1
Comment #373
andypostSet of changes
- removed
_comment_get_default_status()
in favour of entity field api methods (added @todo - probably we need to compare values not array of values)- optimized
comment_entity_view()
to run only on entity fields- fixed upgrade path
- use
$instance->getFieldSetting()
for field settingsPS: out of scope - MyIsam throws error
Failed: Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42000]: Syntax error or access violation: 1071 Specified key was too long; max key length is 1000 bytes: ALTER TABLE {file_usage} CHANGE `id` `id` VARCHAR(64) NOT NULL DEFAULT '' COMMENT 'The primary key of the object using the file.'; Array ( ) in Drupal\Core\Database\Connection->query() (line 571 of /www/core/lib/Drupal/Core/Database/Connection.php).
Comment #375
larowlanDoes this fit better on Drupal\comment\CommentInterface and therefore Drupal\comment\Entity\Comment - thoughts?
Or did you put it here because it needs injection?
Comment #376
andypostFixed regression,
ModulesDisabledUpgradePathTest
still broken - seems needs schema fix as user module doesEDIT I think comment manager is a better place for the function because it could be re-usable,
But probably
CommentInterface
is better place... no ideaComment #378
andypostI'm starting new branch to re-work the approach after #1497374: Switch from Field-based storage to Entity-based storage
Now we can't use field name as bundle for comment entity, so will merge in separate branch and update summary accordingly
Comment #379
andypostPushed initial code to comment-fieldapi5 branch
The main issue with bundle for comment entity - we can't use the field name now so will try menu_link approach with virtual bundle
Comment #380
larowlanWorking on the merge post #1497374: Switch from Field-based storage to Entity-based storage over here: #2079093: Patch testing issue for Comment-Field
Comment #381
larowlanJust pasting the re-roll from #2079093: Patch testing issue for Comment-Field where we tracked the refactor post #1497374: Switch from Field-based storage to Entity-based storage
Interdiffs are there - but in summary - comment bundles are no longer the fieldname, instead {entity_type}__{fieldname}
Now onto the reviews above.
Comment #382
yched CreditAttribution: yched commentedReview of the @FieldType class.
Based on the last patch in the main issue (#731724-460: Convert comment settings into a field to make them work with CMI and non-node entities), but posting it here to avoid derailing even more :-/
Not needed anymore.
$settings = $this->getFieldSettings();
'#field_name' property name is not accurate anymore.
Besides, _comment_field_instance_settings_form_process() is a bit beyond me, but it uses $element['#field_name'] as the second parameter for content_translation_enable_widget() which means it's supposed to be a bundle name - in which case $field->id() doesn't fit. This should be $entity_type__$field_name now, right ?
$entity_type = $this->getParent()->entityType();
$field_name = $this->getFieldDefinition()->getFieldName(); // or even $this->name I think
'#bundle' => "{$entity_type}__{$field_name}
And then you don't need the $field & $instance variables in there. We do really try to avoid FieldItem classes having code that refer to "configurable fields" definition structures (field / instance), and instead do all of the work only using the single getFieldDefinition().
Also, _comment_field_instance_settings_form_process() would be better off as a static protected method within the class (then,
'#process' => array(array(get_class($this), '_comment_field_instance_settings_form_process')),
Some code commented out ?
Minor, but would be better with $entity = $items->getParent();
Comment #383
andypostMerged HEAD and addressed #382, @yched thanx a lot!
Also get rid of
comment_add_default_comment_field()
in favour ofCommentManager::addDefaultField()
Comment #384
andypostWhat we should do:
hook_comment_access()
+COMMENT_ACCESS_DENY
and re-use entity field access forcomment_file_download_access()
, probably checkcomment_reply_access()
Comment #385
yched CreditAttribution: yched commentedThanks @andypost.
Oh, forgot to add that the function name should be renamed / camelCased OO style :-)
Comment #386
andypostRenamed to
processSettingsElement()
Also cleaned-up some deprecated calls after #2030151: Convert entity_get_bundles to a method on the entity manager
Comment #388
andypost#386: sandbox-merge-386.patch queued for re-testing.
Comment #389
micbar CreditAttribution: micbar commentedI tested the patch from #386 on simplytest.me.
If i add comments to a taxonomy vocabulary, it throws following error saving the field settings
Fatal error: Call to undefined method Drupal\taxonomy\Entity\Term::getType() in /home/s4f69e6ad382ad79/www/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.php on line 85
If i add comments to users, it throws following error saving the field settings
Fatal error: Call to undefined method Drupal\user\Entity\User::getType() in /home/s4f69e6ad382ad79/www/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.php on line 85
Comment #390
Anonymous (not verified) CreditAttribution: Anonymous commentedDoes this need a reroll? I tried testing with simplytest.me, but install resulted in an error.
Comment #391
dsnopek@linclark: The latest version of this patch can be found here #2079093: Patch testing issue for Comment-Field
Comment #392
larowlan/me updates issue summary on #731724: Convert comment settings into a field to make them work with CMI and non-node entities to reference #2079093: Patch testing issue for Comment-Field
Comment #393
larowlanNew patch up at #2079093: Patch testing issue for Comment-Field with a massive interdiff addressing most of the review points.
Still some to-dos in https://docs.google.com/a/previousnext.com.au/spreadsheet/ccc?key=0AlTpa... but we're getting close.
Comment #394
larowlanAll of these review items except the changes to CommentUserTest have been rectified, lastest patch is at #2079093: Patch testing issue for Comment-Field
We have 4 fails because of shortcomings in entity-render cache in HEAD.
Keep the reviews coming - lets get this in during Drupalcon!
Comment #395
larowlanAll reviews from here addressed.
Please post new reviews on #2079093: Patch testing issue for Comment-Field
Comment #395.0
larowlanUpdated issue summary.