Updated: Comment #0
Problem/Motivation
#731724: Convert comment settings into a field to make them work with CMI and non-node entities Will add a new CommentManager service. comment_num_new() belongs on that service.
Proposed resolution
Move to a method and deprecate the original function.
Remaining tasks
Wait for #731724: Convert comment settings into a field to make them work with CMI and non-node entities
Patch
User interface changes
None
API changes
comment_num_new() is deprecated.
Related Issues
Follow-up from #731724: Convert comment settings into a field to make them work with CMI and non-node entities.
#2090139: Warning: Illegal string offset 'callable' in rdf_rdfa_attributes() (line 181 of core/modules/rdf/rdf.module).
Comment | File | Size | Author |
---|---|---|---|
#104 | interdiff.2097123.100.104.txt | 1.56 KB | YesCT |
#104 | 2097123-comment_num_new-104.patch | 18.78 KB | YesCT |
#100 | 2097123-comment_num_new-100.patch | 18.75 KB | andypost |
#100 | interdiff.txt | 738 bytes | andypost |
#99 | 1.patch | 18.31 KB | andypost |
Comments
Comment #1
andypostComment #2
das-peter CreditAttribution: das-peter commentedFirst attempt.
There are still calls to procedural code in the method, is it in the scope of this issue to change this as well?
And I don't know if
$user
as parameter is the right approach.So the whole thing doesn't feel sound yet.
Comment #3
andypostLet's test the patch
Please @deprecated
$user makes no sense as first argument
Comment #5
das-peter CreditAttribution: das-peter commentedAddressed feedback in #3.
$user
parameter, really doesn't make sense as long as we don't have a way to pass it on.Btw. I don't get how
ForumManager::numberNew()
ever worked, it looks like this method calledcomment_num_new()
always with wrong parameters.Comment #7
andypost@das-peter Awesome finding about forum!
Not sure why tokens fail, will check tonight
yep, seems we have no test for that.
Also forum adds own comment field so it makes sense to use the name here
Comment #8
andypost#5: d8-deprecate-comment_num_new-2097123-5.patch queued for re-testing.
Comment #10
andypostI can't reproduce failures locally...
Also filed related
#2101155: Add CommentManagerInterface
#2101183: Move {comment_entity_statistics} to proper service
Comment #11
andypostComment #12
andypostFound 2 occurrences of broken forum field usage, now tests should be fine
Comment #13
larowlanI think this should take an array of entity ids, like we did for history_read_multiple(). There are several instances where we're calling this method in a foreach loop which could be more performant in a single query.
This should be injected
So should module handler
Do we have a history service yet? If not we should put history_read behind a method so we have ability to unit-test
We should inject the database connection
Good catch. So far this is the only issue from the 450kb comment-field patch. I can live with that.
Comment #14
larowlan'for the current user' might push it past 80 chars, so 'for a user'
Remove this line
Comment #16
andypostPatch fixes injections, multiple IDs needs a bit more work,
Also found
Warning: call_user_func() expects parameter 1 to be a valid callback, function 'd' not found or invalid function name in rdf_rdfa_attributes() (line 183 of core/modules/rdf/rdf.module).
but that's not patch related, thrown on forum pageDone, but still not sure about injection of common services.
No, that's for
@todo Remove once http://drupal.org/node/1029708 lands.
Comment #18
andypostlet's debug bot
Comment #20
andypostCurrent user is request scope limited so we can't pass it to services
Comment #22
andypostProper fix for tests:
1)
ForumManagerTest
test needs to mock comment manager2)
CommentTokenReplaceTest
should check new comments for user that never read commentsComment #24
andypostWe need to fix simpletest #2102539: Current user service should be updated in drupalLogin() and drupalLogout()
Comment #25
larowlanDisregard the comment regarding fetching in a single query (aka taking array of ids), this can't be done because the $timestamp is different in each case.
Can this be injected?
Other than that looks good to go
Comment #26
andypostLet's name it right, also includes hunk from #2102539-2: Current user service should be updated in drupalLogin() and drupalLogout()
Comment #27
andypostAnd last hunk
Comment #28
dawehnerIs there a specific reason why we don't inject the currentUser?
It would be maybe helpful to have swap the order of the arguments so we kind of have similar signature as with things like entity_load in D7.
Comment #29
andypostOrder of arguments makes a lot of sense!
We pass
$account
as argument so calling code should pass it right, it makes no sense to make manager depends on service that will be limited with request scope #2102027: Add back the request scope to the current user serviceComment #30
dawehnerWell, then we maybe should require the account variable?
Comment #31
pfrenssenI don't think requiring the account is a good idea DX-wise, since most of the time this is called it will be for the current account, and it would put additional burden on the developer to always have to retrieve the current user.
I couldn't make sense of the current user / request scope issue, but looking at the bootstrap, the request is initialized right after the DRUPAL_BOOTSTRAP_CODE phase, which means it is the very first thing happening after the system becomes usable. I can't imagine many cases where it would be necessary to retrieve a comment count earlier than this bootstrap phase :) Trying to do so will throw fatal errors anyway.
Did some work on this:
Comment #32
pfrenssenHere's the interdiff.
Comment #33
andypostProbably we should implement this method in storage controller or postpone on #2068331: Convert comment SQL queries to the Entity Query API
Comment #34
tim.plunkettCan someone explain why this is a major beta blocker? Also, postponing on #2068331: Convert comment SQL queries to the Entity Query API sounds reasonable.
Comment #34.0
tim.plunkettUpdated issue summary.
Comment #35
catchYes I don't think this is a beta blocker, not sure it should block release either since there's no API change necessary.
Comment #36
mgifford31: drupal8.comment-module.2097123-31.patch queued for re-testing.
Comment #38
andypostthis query should be converted to EntityQuery
Comment #39
andypostRe-roll with EntityQuery and @todo reference for #2081585: [META] Modernize the History module (no reason to inject module handler in favor of history manager
Comment #40
andypost39: 2097123-newComments-39.patch queued for re-testing.
Comment #42
andypostre-roll
Comment #43
larowlanCan we get a url added for this todo?
Other than that, RTBC
Comment #44
andypost- filed #2169361: Convert COMMENT_HIDDEN & COMMENT_CLOSED & COMMENT_OPEN to a constant on the comment field interface that needs discussion
- added link
Comment #45
larowlanReady, thanks!
Comment #46
xjmAgreed that this is not major.
Comment #47
catchSorry one more question here. Why not just pass the $entity object instead of type/id?
Comment #48
andypostNew patch changes the argument to entity, also removes wrapper in forum manager that was a simple wrapper function and not defined in
ForumManagerInterface
so no API change here.Also @larowlan's feedback is welcome
Because this will need to change forum manager.
Comment #49
larowlanGreat cleanup to see the wrapper gone from ForumManager, getting there slowly :)
Comment #50
alexpottLet's replace \Drupal::currentUser() with $this->currentUser
What is the purpose of the $account parameter?
Comment #51
BerdirSee #2180109: Change the current_user service to a proxy and #2182055: Comment module causes Circular reference error on a REST request, Comment manager seems to get the current user but it is problematic...
Comment #52
andypostSeems this should be postponed on #2081585: [META] Modernize the History module
Comment #53
mgiffordre-roll... I tried to address $this->currentUser from #50.
Comment #55
mgiffordhmm...
Comment #57
mgiffordComment #58
willieseabrook CreditAttribution: willieseabrook commentedComment #59
ahmed25 CreditAttribution: ahmed25 commentedComment #60
ahmed25 CreditAttribution: ahmed25 commentedI just checked this it doesn't need a reroll.
Comment #61
andypostFix #50, $account was supposed to be passed, but there's a current user service for.
Also removed leftover COMMENT_OPEN constant
Comment #62
andypostAnd a bit more fixes
Comment #65
andypost62: 2097123-newComments-62.patch queued for re-testing.
Comment #66
andypostAnd one more place
Comment #67
dawehnerI guess we also want to inject the module handler.
Can't we just mock the interface?
Comment #68
andypost1) fixed, but module handler will not needed after #2081585: [META] Modernize the History module
2) fixed
Also I start to think- do new method needs access check for current user...
Comment #70
andypostmissed module handler
Comment #72
andypostfixed interdiff
Comment #73
andypostproper patch & interdiff
Comment #74
YesCT CreditAttribution: YesCT commentedI have a feeling we have a system for dealing with @deprecated things.
Is it, commit the patch like this, and then do a follow-up issue to actually remove it?
Comment #75
roderikYeah, the system is at #2158871: [Policy, no patch] Clearly denote when @deprecated code is slated to be removed which includes a followup-meta. Reworded @deprecated to fit this.
Also: needed reroll for #2182055: Comment module causes Circular reference error on a REST request and #2079797: Provide a trait for $this->t() and $this->formatPlural().
Unfortunately I don't know what to do with the 'currentUser' class variable which has been removed. Do we pass the current account into getCountNewComments() now?
Attached part of the before-after reroll diff FYI. (Not included there: comment.module. in there, the call to getCountNewComments() was changed.)
Comment #76
roderikCorrect version of what I wanted to upload FYI. (I guess it's not customary to do before-after-reroll diffs, they're confusing.)
Comment #78
roderikHah, brain freeze. Forgot to check existing calls to getCountNewComments() before I changed its signature.
Rephrase:
I do not know how to reroll this properly;
protected $currentUser
wasn't removed for nothing. I placed the reference to \Drupal::currentUser() back in the rerolled patch, to make tests pass, but I'm pretty sure this is not wanted :/(Maybe #2188287: Split CommentManager in two will make this easier? I don't know.)
Comment #79
slashrsm CreditAttribution: slashrsm commentedReroll after PSR-4.
Comment #80
slashrsm CreditAttribution: slashrsm commentedFixed few minor things.
Comment #83
slashrsm CreditAttribution: slashrsm commentedComment #84
marcingy CreditAttribution: marcingy commentedMore a comment than anything do we have an issue to fix up comment_new_page_count I am assuming yes!!
Comment #85
slashrsm CreditAttribution: slashrsm commentedHere we go: #2281475: Deprecate comment_new_page_count() and move it functionality into Comment storage controller
Comment #86
slashrsm CreditAttribution: slashrsm commentedActually.... this is not OK. DB code should go into CommentStorage.
Comment #87
roderikYou're right.
Comment #88
roderikActually, no. This uses an EntityQuery, which is storage agnostic, so it's fine in CommentManager.
I'm not setting back RTBC, only because I don't understand what changed between comment #20 and now. #20 says you can't inject current_user into services.
(If this has indeed changed, just set back RTBC.)
Comment #89
slashrsm CreditAttribution: slashrsm commentedHm... you're right. current_user is properly injected as of #80.
Comment #91
slashrsm CreditAttribution: slashrsm commentedReroll.
Comment #92
slashrsm CreditAttribution: slashrsm commentedThis was a straight reroll.
Comment #94
roderikReroll. (Effectively: one replacement from a field_id expression to field_name.)
Comment #95
slashrsm CreditAttribution: slashrsm commentedComment #96
webchickCommitted and pushed to 8.x. Thanks!
Comment #99
andyposttest reverted patch
Comment #100
andypostFix broken test
Comment #102
marcingy CreditAttribution: marcingy commentedLooks good
Comment #103
slashrsm CreditAttribution: slashrsm commented+1 for RTBC.
Comment #104
YesCT CreditAttribution: YesCT commentedread through this and noticed some nits. just fixed them. should still be rtbc.
also made #2296839: Remove deprecated comment_num_new()
#100 added comment to the list of module dependencies. It had to do this because #2287223: Use KernelTestBase for config schema tests where possible changed BlockConfigSchemaTest to be a kernel test and kernel tests do not auto enable dependencies of modules.
Comment #105
andypostAwesome! back to rtbc
Comment #106
YesCT CreditAttribution: YesCT commentedI dont think this changes anything related to accessibility. that tag probably was inherited from the parent issue.
Comment #107
alexpottThis should be injected no?
Comment #108
alexpottNope - it's a static function - thanks @slashrsm
Comment #109
alexpottCommitted 6e70062 and pushed to 8.x. Thanks!