Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
As part of #2068331: Convert comment SQL queries to the Entity Query API we're moving all storage dependent code for comment entity into storage controller. We should do the same with comment_new_page_count().
Proposed resolution
- deprecate comment_new_page_count()
- move it's functionality into Comment storage
API changes
- comment_new_page_count() becomes deprecated and is removed in 8.0
- comment mode constants move; see https://www.drupal.org/node/2295487 for change record.
Comment | File | Size | Author |
---|---|---|---|
#51 | interdiff-2281475-48-50.txt | 729 bytes | roderik |
#51 | 2281475_50.patch | 16.26 KB | roderik |
#43 | 2281475_43.patch | 29.31 KB | slashrsm |
#40 | diffdiff-2281475-37-40.txt | 2.25 KB | roderik |
#40 | 2281475-40.patch | 29.33 KB | roderik |
Comments
Comment #1
slashrsm CreditAttribution: slashrsm commentedComment #2
roderikcoming up. I wasn't planning on separating that out but totally works for me :)
Comment #3
andypostSuppose proper tag
Comment #4
roderikActually, thanks for separating this. Because I had not thought of that in #2068331. comment_new_page_count() was not deprecated. Instead, I only split out the threaded new-page-nr into a new method on CommentStorage (because that was the only thing containing SQL.
This is the split-out & rerolled part of #2068331, if only for my own overview.
From there you could theoretically go in two directions:
You said the former, and I'll assume the latter is indeed overkill. Feel free to set back to Needs Work if that changes somehow.
followup patch coming up.
Comment #5
roderik...after I actually post the first patch...
Comment #6
slashrsm CreditAttribution: slashrsm commentedTestbot.
Comment #8
slashrsm CreditAttribution: slashrsm commentedAll storage-specific code (DB queries) go to storage. Non DB-specific logic can live in manager.
Comment #9
roderikAt the risk of being confusing I'm going to try -only once- to fix the above test error separately, in 5a.
--
Indeed. But is it actually worth creating two functions out of comment_new_page_count()? I don't know; the non DB-specific logic isn't much code.
(See the 5a-9 interdiff for comment_new_page_count(): that piece is what could be moved into a function in manager.)
Below patch assumes "No".
More comments:
* This patch treads on #2262407: Deprecate comment_get_display_page/ordinal()'s turf because it also moves the COMMENT_MODE_FLAT definition into the interface. So one should be rerolled after the other gets committed (or we should wait RTBC'ing this one, to not annoy the core committers?)
* Some more misc class/argument renames in unrelated functions were necessary...
* I didn't want to have getNewCommentPageNr() start returning an array|NULL. It still returns a nice and consistent int. Even though it feels like unremoving boilerplate code to its callers...
* still a bit new in this forest of interfaces, I'm not sure if I should inject the EntityManager into the NodeNewComments class. I didn't know where to get it and saw another \Drupal:: being used inside the class... So I didn't.
(Same with the tests.)
Comment #11
roderikobviously should have stolen more of slashrsm's code from #2262407: Deprecate comment_get_display_page/ordinal(). I think this is enough.
Comment #13
roderikassertIdentical(5,5) fails. Obviously.
Is there a better arithmetic expression than this one? *shrug* I just want to see this tested now.
Comment #14
slashrsm CreditAttribution: slashrsm commentedLooks good. Just one minor thing.
"The entity that comments belong to."?
Comment #15
roderikOK. (Tried to see whether unifying the lines above there to your change worked... but it didn't speak to me.)
Also changed my last fix - since (int) always rounds down.
Comment #16
slashrsm CreditAttribution: slashrsm commentedThanks! Looks good.
Comment #18
roderikTemporary glitch? It worked locally and I could step through the test being executed without seeing a hint of what would be the problem.
Comment #19
roderik15: 2281475-15-comment-new-page-count.patch queued for re-testing.
Comment #21
slashrsm CreditAttribution: slashrsm commentedReroll.
Comment #22
slashrsm CreditAttribution: slashrsm commentedWas very simple re-roll.
Comment #24
roderikReroll after #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form.
If someone cares: (Non-unified) diff of patches attached as a record that the only 'changes to the changes' are $field_id -> $field_name stuff.
Comment #26
roderikd'oh, incomplete reroll
Comment #28
roderiksrsly d'oh
Comment #29
roderikDespite me taking 3 uploads *sigh*, all that happened since #21 was a
s/field_id/field_name/
so, still RTBCComment #30
marcingy CreditAttribution: marcingy commentedRTBC seconded
Comment #31
webchickJust two small things. I'd change them myself but they happen enough that there's a chance I'd miss one and screw up testbot.
As long as we're fixing the grammar of the one below this,
"The entity to which the comments belong."
Let's spell out "Number" in the method name and the holding variables ($page_nr => $page_number). We don't typically abbreviate words, and when we do we definitely don't do "num" on some places and "nr" in others. ;)
Comment #32
slashrsm CreditAttribution: slashrsm commentedFixed.
Comment #33
marcingy CreditAttribution: marcingy commentedLooks good
Comment #34
andypostAlso summary should be updated about removal of constants, suppose this API change (maybe better to file new issue or change record)
should be fixed
$page_no at least
Comment #35
andypost32: 2281475_32.patch queued for re-testing.
Comment #37
roderikRerolled (was trivial) and renamed variables according to #31-2/#34-2. (And renamed $total_comments_number to $total_comments, for consistency with $new_comments.)
As for #34-1: short change record created at https://www.drupal.org/node/2295487. I can see the sense in splitting this out to a separate issue (this constants change is actually in several RTBC issues now)... but since webchick already almost-committed this and other issues are waiting for this... I'd rather not split things further.
Comment #38
slashrsm CreditAttribution: slashrsm commentedLooks good.
Comment #40
roderikReroll after #2097123: Deprecate comment_num_new() in favour of method on CommentManager.
Diff-between-diffs included to prove that there's nothing to re-review. (The change from 'comment_node_forum' to 'comment_forum' is not our business; #2097123-13: Deprecate comment_num_new() in favour of method on CommentManager at bottom says it's cool.
Comment #41
slashrsm CreditAttribution: slashrsm commentedComment #42
alexpottComment #43
slashrsm CreditAttribution: slashrsm commentedReroll.
Comment #45
slashrsm CreditAttribution: slashrsm commentedComment #46
slashrsm CreditAttribution: slashrsm commentedWas a simple reroll.
Comment #47
alexpottI think the docblock on the interface is incorrect.
Unnecessary use statement
Comment #48
slashrsm CreditAttribution: slashrsm commentedGood catch! Thanks!
Comment #49
andypostback to rtbc
Comment #50
alexpott#2262407: Deprecate comment_get_display_page/ordinal() landed which has the same constant change in.
Comment #51
roderikRerolled, and much smaller now all constant changes are already in. The only 'misc' stuff remaining is some class/argument renames in unrelated functions which were necessary.
Interdiff is small change to the @return comment, to say its 0-based.
You'll see a reference to a second change notice (https://www.drupal.org/node/2299799) appear shortly. Will set its status to Draft again, because it will contain documentation for this and other uncommitted changes.
Comment #52
marcingy CreditAttribution: marcingy commentedLooks good :)
Comment #53
alexpottCommitted d8a4d54 and pushed to 8.x. Thanks!