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.
Part of #2068325: [META] Convert entity SQL queries to the Entity Query API. See the parent issue for details.
This issue accumulated into a large patch tacked by many people over time, and then got split into lots of smaller issues which have been committed separately.
The queries that remained got swept up and discussed from #95 onwards.
Comment | File | Size | Author |
---|---|---|---|
#132 | interdiff-2068331-127-132.txt | 2.4 KB | roderik |
#132 | comment-sql-2068331-132.patch | 11.25 KB | roderik |
#114 | interdiff-2068331-112-114.txt | 1.42 KB | roderik |
Comments
Comment #1
slashrsm CreditAttribution: slashrsm commentedWorking on this.
Comment #2
slashrsm CreditAttribution: slashrsm commentedThis patch is not finished. It includes first batch of conversions. Posting here mostly for my reference and to check with testbot if agrees with it.
Comment #4
slashrsm CreditAttribution: slashrsm commented#2: 2068331_2.patch queued for re-testing.
Comment #6
slashrsm CreditAttribution: slashrsm commentedThis should fix tests. No new conversions yet.
Comment #7
slashrsm CreditAttribution: slashrsm commentedSome more conversions.
Comment #9
slashrsm CreditAttribution: slashrsm commentedComment #10
slashrsm CreditAttribution: slashrsm commentedComment #12
slashrsm CreditAttribution: slashrsm commentedJust confirming that I'm still working on this. I am almost there, just need to fix few more tests.
Comment #13
slashrsm CreditAttribution: slashrsm commentedWow... this was one hell of a re-reoll. Comments field patch created a total mess here.
Comment #15
slashrsm CreditAttribution: slashrsm commentedComment #16
slashrsm CreditAttribution: slashrsm commentedComment #18
slashrsm CreditAttribution: slashrsm commentedComment #20
slashrsm CreditAttribution: slashrsm commentedThis should finally make tests pass. Some more conversions needed.
Comment #21
slashrsm CreditAttribution: slashrsm commentedThis should be the last set of relevant conversions.
Comment #23
slashrsm CreditAttribution: slashrsm commentedComment #25
slashrsm CreditAttribution: slashrsm commentedComment #27
slashrsm CreditAttribution: slashrsm commented#25: 2068331_25.patch queued for re-testing.
Comment #29
slashrsm CreditAttribution: slashrsm commentedComment #30
slashrsm CreditAttribution: slashrsm commentedThis should now be ready for review.
Comment #31
andypostThe patch is awesome! there's a some questions about moving part of functions to storage controller, probably most of them should live in CommentManager - not sure which one better!?
Contrib could swap manager and storage to more performant implementation so:
1) once storage changed for example to mongo then better to override the storage controller but other places should be fixed anyway
2) once manager changed then some functionality could be changed (number of items for example)
So I think better to discuss the issue a bit more and maybe split to parts to sort implementations to proper places.
Also I linked related issues to changed parts
Related issue #2111357: Get rid of comment_count_unpublished() in favor of CommentStorage method
Related #2054993: Remove (untested, unused, broken) comment_get_recent()
Suppose the CommentManager is better place for that.
Let's deprecate|remove this one once the result is changed anyway
The related issues
#2101183: Move {comment_entity_statistics} to proper service
#148849: Refactor {comment_entity_statistics} into performant Field
Let's do not use the wrapper once have method on storage controller
is there other usage of the method?
Related #1059608: Fix comment_get_recent ordering
Comment #32
andypostDiscussed in IRC - msonnabaum> all database access should go in the storage class
https://gist.github.com/msonnabaum/b88c73dbd5a910ee1db3
Comment #33
plachThe patch no longer applies and we have the code reviews above to address.
Comment #34
vijaycs85Re-rolling... not sure anything need to be updated for #31.
Comment #35
plachComment #36
slashrsm CreditAttribution: slashrsm commented#31 seems to only list related issues, etc. I think this should be ready for review. Please correct me if I'm wrong.
Comment #37
marcingy CreditAttribution: marcingy commentedThis doesn't seem too touch comment_admin_overview this may be intentional as I am assuming there is a routing patch out there but at the moment it has
Comment #37
marcingy CreditAttribution: marcingy commentedThis doesn't seem too touch comment_admin_overview this may be intentional as I am assuming there is a routing patch out there but at the moment it has
Comment #38
slashrsm CreditAttribution: slashrsm commentedYes... I left it like this intentionally. Mainly since I assumed it will be replaced by a view. I can definitely add it if needed.
Comment #39
andypostThe fallback should be still there, related conversion is #1978904: Convert comment_admin() to a Controller
Comment #40
slashrsm CreditAttribution: slashrsm commentedConversion included. It will conflict once #1978904: Convert comment_admin() to a Controller is converted, but should be pretty easy to re-roll.
Comment #41
andyceo CreditAttribution: andyceo commented40: 2068331_40.patch queued for re-testing.
Comment #43
plachComment #44
piyuesh23 CreditAttribution: piyuesh23 commentedComment #45
piyuesh23 CreditAttribution: piyuesh23 at QED42 commentedRe-rolled the patch. Uploading the updated patch.
Comment #46
Noe_ CreditAttribution: Noe_ commented@piyuesh23 The tests failed.
This error only occurs after I patched Drupal to with your changes.
I don't know but I think your calls to getFieldSetting() should be getSetting().
PS: It was not the getFieldSetting() code because when I changed that it just continued, but with a lot of errors.
Comment #48
mradcliffeThis jumped out at me.
This should be loadCommentingUsers, right?
Comment #49
jibran45: 2068331_44.patch queued for re-testing.
Comment #51
mradcliffeComment #52
pcambraHere's a re roll taking into account #46 & #48 plus several other changes to make the patch apply
Comment #54
pcambraSome minor changes to fix (most?) of the tests.
Comment #56
sidharthapThanks for the patch @pcambra
This patch fails
error: patch failed: core/modules/comment/comment.module:408
Use \Drupal\comment\Controller\CommentController::adminPage() instead of comment_admin()
Use buildForm to construct the form instead old style.
Comment #57
pcambraThanks for noticing about the admin.inc file @sidharthap, I definitely think that's something coming from some merge conflict as the comment admin forms are in \Drupal\comment\Form\CommentAdminOverview. now.
Fixed some historical merge problems and removed that file, here's a new patch.
Comment #59
pcambraThis one should be better.
Comment #60
jibranCan we make these multi line wherever possible please? Readability++
more then 80 chars.
Comment #61
pcambraThanks @Jibran for the review
Here's a new patch with the formatting changes in #60 applied
Comment #62
k.dani CreditAttribution: k.dani commentedI checked the patch and realized that it uses
comment_get_thread()
incomment_node_update_index()
function. This function will be depricated according to the following issue: #2156089: Remove comment_get_thread() in favour of method on CommentStorageShould we make it posponed until the mentioned issue is fixed?
Comment #63
roderikReferring in part to #2156089-2: Remove comment_get_thread() in favour of method on CommentStorage: It looks to me like this method should be moved into CommentStorageController anyway, since there is no way this could be done in a database generic way.
In other words, the patch in this issue could solve #2156089 in one go.
I'm going to work a bit on this patch regardless, because I found some strange things (and pcambra is probably way busier than me, D8 Core beginner :) )
Comment #64
roderikNo need to look at this patch now.
Rerolled for #2101183: Move {comment_entity_statistics} to proper service and the renaming of storage controller interfaces. Also removed comment_get_recent() and theme_comment_block() which snuck back in by accident during a previous reroll.
Also removed some of the storage interface's methods which the previous patch introduced, and replaced them by EntityQuery where that made sense to me. I'm not done with that yet, though. To finish up, I'll have to sit down and think about what to do with some code in tracker.module. Probably move its remaining SQL queries into a service first -- much like #2101183 did for comment statistics.
So, more coming up in a next patch (and I'll then provide an interdiff from #61). But not for a few days.
Comment #65
plachAny news here? :)
Comment #66
roderik*cough* sorry, no. This weekend looks promising though. I'll update here by saturday.
Comment #67
roderikStatus update! Wut, it's not saturday (19th) already, is it? :p
(Excuse of the week: got sucked deeper with every step, into a swamp of fixing contrib modules for a live website. I should never do urgent ongoing Core work; stuff like this will keep happening.)
If urgent: tell me, it helps. If not: I will try to parse #2068333: Convert node SQL queries to the Entity Query API (back to CNW again) today and this issue tomorrow.
Comment #68
plachThe node issue should prbably be prioritized but obviously the sooner we get these two in, the better :)
Comment #69
roderikOK. Pick your interdiff at will. Remarks and questions in semi random order:
@deprecated Deprecated since Drupal 8.x-dev, to be removed in Drupal 8.0.
So that's what I did forComment #72
roderikOh, meh.
Comment #75
roderikOh. During code reshuffle, I deleted the existing CommentStatisticsInterface::updateEntityStatistics() but didn't delete the class method.
It may strictly not belong in this patch... but let's just delete this method here? There is only 'update' in here, not 'create', 'read' or 'delete'...
See above interdiffs and the comments in #69.
Comment #76
roderikChange in #74 was incomplete: we can/should get rid of the whole dependency to the comment.statistics service + overridden createInstance()...
Comment #79
roderikUnit test was failing because it missed the comment.statistics service. Should be fixed now. Also inserted/deleted some random comments.
Comment #80
roderikYou know what - this is just way too big.
I found a second already-existing issue (with an existing patch) so I am going to wait until some split-off parts are committed. Also, note I should just open separate issues for
Comment #81
roderik.
Comment #82
roderikComment #83
roderikComment #84
chx CreditAttribution: chx commentedCan this be unpostponed now?
Comment #85
roderikYou can unpostpone if you want to review this whole thing in one go, but I figured it would be easier to do it in parts.
Getting each of the 3 "Related issues" committed will decrease the size and complication of this patch. I will go reroll those now. If you have some junior-contributors-like-me walking around, maybe they will want to review those?
I'm working / in Europe, but will be mostly available to reroll (and shrink) this one when I see activity in the other issues.
If, on the other hand, you want me to reroll this big one because you think getting it reviewed/committed in one go is the better way to approach this, please tell me.
Comment #86
slashrsm CreditAttribution: slashrsm commentedAgree. Fixing this in parts seems easier. Let's fix related issues ASAP and then finish last part in here.
Comment #87
roderikNice.
Some summary (I'm off till tomorrow, after this):
#2156089: Remove comment_get_thread() in favour of method on CommentStorage I rerolled, needs review
#2262407: Deprecate comment_get_display_page/ordinal() you rerolled - waiting for comments I guess? I can RTBC your patch but not my own logic without someone saying they looked at my patch in detail?
#2097123: Deprecate comment_num_new() in favour of method on CommentManager you rerolled - myeah, I didn't really work on this, only tried to do a reroll which was imperfect, don't know the code yet. Will review when green, I guess.
(About the last fail: while rerolling something else, my eye spotted a FieldInfo class that was replaced by an EntityManager class recently? I don't know what it did, just making an offhand uninformed remark :)...)
Just opened #2280861: CommentStatistics service followup because I had been planning to do that. Should be review-able.
Comment #88
roderikComment #89
larowlanDo we have a sub-issue for comment_new_page_count()?
Comment #90
marcingy CreditAttribution: marcingy commentedI couldn't find one #2290155: Deprecate comment_new_page_count in favour of commentManager/commentStorage created, I have assigned to myself and will look at in the next day or so but feel free to grab.
Comment #91
roderik#2281475: Deprecate comment_new_page_count() and move it functionality into Comment storage controller is for comment_new_page_count, RTBC, mentioned in left sidebar.
Comment #92
andypost@larowlan suppose once we move direct queries to entity queries we should separate storage dependent methods to
CommentStorage
and independent toCommentManager
Comment #93
roderik(if people are checking this issue now, with 2 sprints going on: I will check this stuff and weed out all committed code, maybe leading to closing the issue, before saturday.)
Comment #94
andypostLooks all child issues are closed
Comment #95
roderik...which leads to the following leftovers from the previous patches:
Note there is more to rewrite about tracker module, but I stopped once the {comment} sql was gone. tracker.page.inc conversion is for #2280861: CommentStatistics service followup, and the one {node_field_data} query remaining in tracker_cron(). is for... the future TrackerRepository service (no ticket opened yet), imho.
Comment #96
roderikDoing Node::load() in one place and not the other, is weird.
Comment #99
roderikWrong 'reroll'.
(Also, now formatting the SQL just like a CommentStorage::getNewCommentPageNumber() did above.)
Comment #100
roderikOh by the way! This patch 'depends' on #2280861: CommentStatistics service followup at the moment!
The 3rd argument from
...has no meaning at the moment; it's converting the 'target = replica' option that will be honored if #2280861 gets accepted.
Comment #102
roderikdum da dum see #95 & #100
Comment #103
andypostYes, we need to wait for #2280861: CommentStatistics service followup
The patch looks good but I disagree to add new method to storage that will require a test
This is not strait conversion, also I see no reason in this method in storage because it has only one usage.
And the tracker should be replaced with views! #1941830: Convert tracker listings to a view
Not sure that approach right, but default_langcode makes no sense for EQ.
Let's get @plach here to mention!
Seems the test a bit outdated, but that out of scope
Comment #104
slashrsm CreditAttribution: slashrsm commentedIs there any better way to convert? One of the reasons why we're converting is easy storage swapability. In order to achive that we need all DB-specific code in one place (== storage).
Comment #105
andypostThe better way is just simply replace db_select with the equivalent EQ, but as I pointed - queries are totally different
Comment #106
chx CreditAttribution: chx commentedFor the sake of simplicity; for now; we should move that query from tracker into comment 1:1 and leave a
// @todo remove when https://www.drupal.org/node/1941830
is in.Comment #107
roderikIn this interdiff:
if ($node->isPublished()) {
I kind-of disagree, though it's not apparent at first sight.
The old SQL query gets all user IDs from comments (except the node->uid), and then inserts a number of rows into {tracker_user}: each user ID & 'static' values for all other columns.
INSERT INTO {tracker_user} SELECT uid, [static values] FROM {comment_field_data} WHERE [conditions]
The converted SQL query still does the same, but...
The intention of what we are changing now (if I am correct) is to remove the explicit dependency on {comment_field_data}, so that's what is done by packaging the {comment_field_data} query into a CommentStorage method.
This implies that the new query must be converted to a
INSERT INTO {tracker_user} VALUES (uid1, [static values]), (uid2, [static values]), ...
. And that the [conditions] are moved outside the query (into code deciding which uids to insert).IMHO it's as straight a conversion as I can make it, given the assumption we need to get rid of {comment_field_data}.
I bounced back and forth on this. Is there another way in which we can get 'all users in a thread of comments'? This is a pretty generic need (theoretically, though nothing else uses it). I can use loadThread() because we don't really care about efficiency anyway... but then I'm a bit scared of the mandatory 'comment_filter' tag in that query.
...all of the above goes away if we make all this stuff into a TrackerRepository service and we don't care about that service using {comment_field_data} directly. But I think we do care - decoupling of thingies?
Cool. We're lucky; #1941830: Convert tracker listings to a view does not coincide with changes here. (Only with #2280861: CommentStatistics service followup.)
@chx: That implies "no can do" on #106. If you want to, though, I'm fine with an explicit decision to not convert SQL from tracker.module yet, and split the tracker.module part (and above discussion) into a new issue so we can call comment.module converted at least.
(It's gonna be added to #2068325: [META] Convert entity SQL queries to the Entity Query API though ;) )
Comment #108
roderikI was typing while you were discussing :)
My point in a shorter reply:
@andypost
But it's not a 'SELECT' query. It's an 'INSERT INTO {tracker_user} SELECT ...' query. How to convert that?
Comment #110
roderikComment #112
roderikstill fixing silly minor things in EQ
Comment #113
andypostOverall great!
tricky, comments can not work with non-int IDs, but not sure that's needed here. Probably for postgres only, and sometimes entities returns ID as string ("123")
should wait for #2280861: CommentStatistics service followup
previously I refer to this place, that is not properly converted, but now I see that "changed" is not used and calculated latter
Node::load($nid) but I'd preserve comment, there's issue somewhere in d8mi about negotiation...
Comment #114
roderik114.1
I really didn't know what to do there :) don't know the implementation of this in the lower DB layers. I got it from comment_entity_predelete().
But now I see other places that don't do (int). So I will delete this. (If there's ever a bug, we will see it.)
114.4.1
Please teach me (I don't know where to find out except typing a novel on IRC):
I like Node::load($nid) better too, but doesn't it create a hard dependency on \Drupal\node\Node while we want this class to be swappable? Or don't we care about swappability in the case of 'base entity classes' or is there something I am not seeing?
114.4.2
The comment (generic-clone comment inserted ~30 times for every {node_field_data} occurrence during working on #1498674) makes no sense to me when {node_field_data} disappears. I inserted a function-level comment, does it make sense?
Comment #115
roderikOh and re. #103, I need guidance on writing tests for that CommentStorage::getCommentingUserIds();
Not the code specifics, but the overall picture. (Maybe guidance can be simply "put it in X.php".) Because I don't see yet if
Comment #116
roderikRe 114.4.1 oh never mind -- it's a static function, and I should have checked its definition. Still having a hard time keeping up to date with all these newfangled modern OO inventions ;-)
Only #115 outstanding (if you agree with the comment in #114)
Comment #118
roderikComment #119
roderikComment #125
Sharique CreditAttribution: Sharique commentedFixing patch apply, lets see how many test fails.
Comment #126
Sharique CreditAttribution: Sharique commentedChanging status to need review.
Comment #127
roderikchx suggested using QueryAggregate instead of the new interface function, which works wonders.
(I had to think for a minute, whether an SQL 'group by' query without an aggregate field would be legal everywhere. But indeed, why wouldn't it?)
The $do_insert in the interdiff is fixing a bug that had been in this patch forever.
Comment #128
andypostOnly one thing left imo...
reset() is not needed here because of range()
Comment #129
slashrsm CreditAttribution: slashrsm commentedReally? Return will still be an array so it is either loadMultiple() or reset().
Comment #130
chx CreditAttribution: chx commentedI concur. reset is simpler.
Comment #131
webchickOverall this looks great, one question though...
Why did this code get indented into a if ($node->isPublished()) block? That didn't seem to be there before.
Comment #132
roderikWell that was a bit silly. Now that the need for a 'generalized' method has been replaced with a QueryAggregate, we can actually do a straight conversion of the existing query.
(Not to mention that the if ($node->isPublished()) block was a mistake to begin with...)
Comment #133
chx CreditAttribution: chx commentedIf #127 didn't fail despite #131 then we need more tests.
Comment #134
roderikAbsolutely. But since
...could we put the test into a followup which tackles the rest of the tracker stuff/tests?
Created #2330577: Introduce TrackerRepository service.
Comment #135
roderikComment #136
andypostI'm fine with follow-up, because tests are out of scope for conversion
Comment #137
alexpottOkay let's have a followup to add the missing test coverage.
Committed 8d594b5 and pushed to 8.0.x. Thanks!