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.
This comes from http://drupal.org/node/1805996#comment-6622836
Comment | File | Size | Author |
---|---|---|---|
#28 | 1817672-28.patch | 17.07 KB | jibran |
#28 | interdiff.txt | 962 bytes | jibran |
#26 | 1817672-26.patch | 17.03 KB | jibran |
#26 | interdiff.txt | 10.69 KB | jibran |
#25 | 1817672-25.patch | 19.86 KB | jibran |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedComment #2
dawehnerOh this is embarrassing.
It's funny as we replaced them earlier with dbtng code and there are probably other places #1748168: Convert or document all usages of db_query()
Could it be that we should actually basically revert that patch?
Comment #3
xjmClosed #1817608: replace the query in NodeNewComments with a static one as a duplicate of this issue. See the comments from DamZ and chx there.
We'll want to split the aggregator change off and move it into the patch to add that integration (once it's posted).
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commented(copied from the other issue)
This query (both versions of it) doesn't seem to make any sense. At the very very minimum, it is missing a constraint on
h.uid
.I don't think there is a way to do this query properly (we want to count the number of comments posted to a list of nodes after the time the user last seen each node), other then with an ugly correlated scalar subquery:
So I just suggest we do like the forum module does in
forum_get_topics()
.Comment #5
dawehnerOh right, the new version (dbtng) is missing the constraint, the old
one had it.
This patch basically reverts the changes done in #1748168: Convert or document all usages of db_query() for every
place where it has been helpful.
Comment #6
dawehner#5: drupal-1817672-5.patch queued for re-testing.
Comment #8
dawehnerRerolled now.
Comment #9
dcam CreditAttribution: dcam commentedI found a few issues, two of them just code style problems. They seem like good things for a Novice to fix.
There's a double space at the end of the query argument, right after "GROUP BY n.nid".
It looks like this one was copied and pasted from another patch, probably the issue mentioned in #2 that converted queries from db_query to db_select. After the second line was pasted, however, the "- " wasn't removed.
The second "in" isn't uppercase.
Comment #10
bdgreen CreditAttribution: bdgreen commentedIssues corrected ...
Comment #11
bdgreen CreditAttribution: bdgreen commentedComment #12
dcam CreditAttribution: dcam commentedI'm sorry, I wasn't very clear. The extra spaces in front of the array argument need to be deleted too, not just the '-'. It should be indented two spaces from the start of the previous line.
#10 also adds several extra code style changes. Another more experienced contributor might want to comment on it, but I think we generally try to keep patches on topic and not add extra items, even if they seem necessary. The reasons why are that it can make patches harder to review by confusing the issue and if they did end up being committed it increases the chances that other patches will need to be rerolled. I would open up another issue to address them.
Comment #13
bdgreen CreditAttribution: bdgreen commented@dcam acknowledged ;) Rerolled from #8 and added changes (only) from #9, updated with #12 -- hope that's OK?
Comment #14
dcam CreditAttribution: dcam commented#13 looks good to me! It reverts several queries that were originally changed to db_select() in #1748168: Convert or document all usages of db_query(). Thanks, bdgreen!
Comment #15
alexpottNeeds reroll
Comment #16
jibranConflict
Fixed
Comment #17
dawehnerIn a perfect world all this classes would get the database connection injected via the create() method.
Comment #18
jibranso should we wait for #1863816: Allow plugins to have services injected.
Comment #19
dawehner@jibran
This is already in, so why do we have to wait?
Comment #20
jibranTrying to make a perfect world.
db_or()
.Comment #21
jibranSame patch for testing.
Comment #23
jibranForgot
somea lot off use statements.Comment #24
dawehnerThat's nearly ready to fly now!
Let's document the new parameter as well.
Let's remove the double quotes.
Comment #25
jibranThanks @dawehner for the review. Fixed #24. Added doxygen for create method as well.
Comment #26
jibranRemoved doxygen for
create
beacuse of\Drupal\Core\Plugin\ContainerFactoryPluginInterface
.Comment #27
dawehnerAwesome!!
We seriously should do foreach ($account->getRoles() as $rid) instead.
Comment #28
jibranDone.
Comment #29
dawehnerNice! One less db query needed in the code.
Comment #30
damiankloip CreditAttribution: damiankloip commentedLooks good. thanks jibran!
Changing the title to move with the times.
Comment #31
alexpottCommitted db0104f and pushed to 8.x. Thanks!