Convert node SQL queries to the Entity Query API

I don't see any sql queries in the menu_link module (except the \Drupal\menu_link\MenuLinkStorageController of course).

So I think this issue should be closed.

Sorry, previous comment landed on the wrong issue...

Here is a partial solution, changes only in the node.module file. It also contains some todos where are queries to change in this issue.

I'm also created a new function (node_mass_update_user_nodes()) to eliminate some code duplication.

New patch its doesn't touch tracker or statistics yet but I am also thinking they likely don't need to be ported initially as after all if I was using mongo for storage I most definently would not be using either of those modules!

1. +++ b/core/modules/node/lib/Drupal/node/NodeStorageControllerInterface.php
   @@ -0,0 +1,53 @@
   +  public function adminNodeList(Array $header);
   

   +++ b/core/modules/node/lib/Drupal/node/NodeStorageController.php
   @@ -82,4 +82,60 @@ protected function postDelete($nodes) {
   +  public function adminNodeList(array $header) {
   

   +++ b/core/modules/node/node.admin.inc
   @@ -198,33 +198,7 @@ function node_admin_nodes() {
   +  $nids = \Drupal::entityManager()->getStorageController('node')->adminNodeList($header);
   

   This function is no longer necessary since substituted with default entity list.

2. +++ b/core/modules/node/lib/Drupal/node/NodeStorageControllerInterface.php
   @@ -0,0 +1,53 @@
   +  public function userRevisons(EntityInterface $account);
   

   +++ b/core/modules/node/node.module
   @@ -829,20 +817,35 @@ function node_user_cancel($edit, $account, $method) {
   +  $revisions = \Drupal::entityManager()->getStorageController('node')->userRevisons($account);
   

   +++ b/core/modules/node/lib/Drupal/node/NodeStorageController.php
   @@ -82,4 +82,60 @@ protected function postDelete($nodes) {
   +  public function userRevisons(EntityInterface $account) {
   

   Renamed the function as userRevisions.

10: node-entity-move-10.patch queued for re-testing.

(Merging "node system" and "node.module" components for 8.x; disregard.)

hi,

I can take this issue and try to create a new patch file. I rerolled the patch file from #10, also replaced the db_selects with entity queries in the test files. But some of them can't be made with entity queries. what do you recommend? shall I let them there, I don't think that one is the good solution.

I don't think we need to worry about tests?

Correct as far as I know no need to touch tests, none of the other related issues do.

ok, then I check again my patch file and push it here.

I used the patch file from #10 to continue to work on this.
I added a node storage controller, I replaced the SQL queries to entity queries where I was able to and if it wasn't possible I added methods in the storage controller to keep those features.

In lastChanged() you are missing an execute and fetchField() would be better also you can do

$query =$this->database->select('node_field_data')->condition('nid', $nid); // format this nicely
if (isset($langcode)) { // add the right langcode condition
return $query->execute->fetchField();

Also, please be mindful of \ No newline at end of file these git warnings and add a newline to the end of the new files. Thanks!

And thanks for working on this, really nice to see this moving forward.

Fixed the issues found by chx and also some found by me (renamed some variables, modified documentation).

fixed some issues, found another query to be converted, fixed that also.

28: node-entity-queries-2068333-28.patch queued for re-testing.

Here's a reroll changing some signatures of the account interfaces and node interfaces used, let's see what else is missing from here.

There were a couple of missing includes.

this seems ok, but personally I don't like the solution to call a procedural function from a storage controller. can't we find a solution to avoid this kind of code or do you think this kind of dependencies are ok?

+++ b/core/modules/node/lib/Drupal/node/NodeStorageController.php
@@ -0,0 +1,122 @@
+    module_load_include('inc', 'node', 'node.admin');
+    $nids = $this->userNodes($account);
+    node_mass_update($nids, array('status' => 0), NULL, TRUE);

Just to clarify that we're discussing about this piece of the code.

I don't like it either but I guess it should be handled in a followup, maybe making it a method of the storage controller from nodes and removing it from node.admin.inc?

Maybe related: #89181: Use queue API for node and comment, user, node multiple deletes

I do not see why do you need to move all the procedural code into the storage controller...? ->userNodes is already public so can't we just do:

$storage_controller = \Drupal::entityManager()->getStorageController('node');
module_load_include('inc', 'node', 'node.admin');
node_mass_update($storage_controller->userNodes($account), array('status' => 0), NULL, TRUE);

and

$storage_controller = \Drupal::entityManager()->getStorageController('node');
module_load_include('inc', 'node', 'node.admin');
node_mass_update($storage_controller->userNodes($account), array('uid' => 0), NULL, TRUE);
$storage_controller->anonymizeUserRevisions($account);

and then anonymizeUserRevisions would just be the update() in anonymizeUserNodes in the current patch. Already node_user_predelete() does something similar.

Agreed with @chx, here's a version implementing the changes proposed in #37

seems ok for me

Trying to review issues @ DevDays, and learn. Relatively new to D8 core contribution, so please bear with some wordiness and feel free to tell me, if doing/saying too much/little.

done:
- reviewed the code
- reviewed the node module for outstanding stuff, obviously didn't find anything
- the scope of this issue does not require checking against 'core gates' or anything else I can think of. (I did find something that left me wondering, but I'll ask in the related meta issue.)

I wouldn't RTBC even if I did only space changes, so it's still Needs Review. But I think anyone who's seen the previous version can quickly set RTBC. It was already reviewed before me, anyway.

...but I did minor changes:

- space/indent changes

- changed 'join' to 'innerJoin' in revisionsList, for clarity. (Another innerJoin is already in there too.)

- inserted extra comment in node_user_cancel(); if node_mass_update() confuses me, it may confuse someone else too...

Thanks for the review! I don't spot these things but can probably implement. Will do this before working further on #2068331, and take the above into account there.

Interdiff is between the rebased patch #41 and the new one; I assume that's what people do. The difference between #41 and #41-rebased is that the menu.inc patch has fallen away: #2199381: Add \Drupal\Core\Path\PathInterface moved menu_tree_check_access() to Drupal\menu_link\MenuTree::checkAccess() and rewrote it to not use a database query anymore.

---

re. #42:

point 1: obsoleted by the above - lucky me ;)

point 2-6 implemented; 7 not seen. Plus added some random comments in the new interface.

re. point 4:

• Added an extra argument to node_mass_update(). Not ideal, but I guess there isn't an ideal way. Comments welcome.
• Manual testing shows that in the 'user_cancel_reassign' case (where node_mass_update() now gets a list of node revisions to update to 0)... node_field_revision.uid gets set to 0 but node_revision.revision_uid keeps the id of the now-deleted user. This also happens before applying the patch; I can't see if it's an issue that should be filed, someone is working on deleting node_revision.revision_uid, or whatever else. So, assuming I shouldn't create an issue for that.

Tests didn't fail on my local machine, so I hope the failures go away now :/

In the meantime: rerolled after #2188613: Rename EntityStorageController to EntityStorage got committed. The comments from #45 still apply; there's just an extra renamed interface/class/file/method in the interdiff.

*cough*

(LOL. Maybe a 'use' statement helps before using Language::LANGCODE_NOT_SPECIFIED. Live and learn. Now just uploading and seeing what the testbot says.)

Again: comments from #45 still apply.

#1: d'oh! / #2: done.

revisionsList() has been replaced by revisionIds(). Points of note:

• Naming? (I didn't find any other existing functions that retrieve revisions, which seems a bit strange to me...)
• I nailed down the order in which revision IDs are returned, in the interface. Am I allowed to do that?
• node_revision_list() has pretty specific return arguments and actually only one caller (node_revision_overview()) uses those. So, as discussed, node_revision_list() has been removed; node_revision_overview() was changed to accomodate this, and other callers are now changed to use the revisionIds() method.

Those calls are inside simpletests.
My local environments is failing tests in mysterious (possibly unrelated) ways now :/ so I'll just see if the test bot is nice to me.

.

Oh; draft change record at [#2228895].

1. +++ b/core/modules/node/lib/Drupal/node/NodeStorage.php
   @@ -0,0 +1,64 @@
   +    return $this->database->select('node_revision', 'r')
   

   This and others don't really need to use ->select, could just be static queries no?

2. +++ b/core/modules/node/node.module
   @@ -926,32 +908,24 @@ function node_page_title(NodeInterface $node) {
   +    ->addTag('node_access')
   

   I don't understand this change. tbh I don't understand the entire function.

   This passes in an nid, so we could just load the node. But where this gets called, we already have the node.

   Also it is loading the nodes but not checking the translated value of 'changed'.

   NodeFormController::validate is one of only two places this is used:

   node_last_changed($node->id(), $this->getFormLangcode($form_state)) > $node->getChangedTime()))

   That results in $node->getChangedTime() > $node->getChangedTime() after all that work it looks like.

   Should just be a check on the original entity or something? We could likely remove node_last_changed() altogether.

+++ b/core/modules/node/node.pages.inc
@@ -134,45 +132,49 @@ function node_revision_overview($node) {
+    if ($revision = node_revision_load($vid)) {

Looks like a performance regression, but unavoidable really.

Yes, the last changed function looks wrong, that very explicitly looks for it with a raw query.

We have an EntityChanged constraint now, which uses loadUnchanged() instead.

#1
sure; will remember the convention.
(For consistency, I also made it explicit that userRevisions() returns a sorted list; comments still welcome.

#2
Thanks for your comments! Should have seen that this rewrite wasn't making sense / checked the actual function usage.

If I pieced all the info together correctly,

• You're right, we can remove node_last_changed()
• ...but not before #2002180-9: Entity forms skip validation of fields that are edited without widgets gets fixed.

So shall I just place a @todo to delete this function later (while ignoring the fact that we ignore the $langcode argument, for now)?

Still a noob, I know. (I guess this is the correct way to do update db_queries...)

.

alexpott		roderik: revisionIds vs userRevisions - both return a list of vids
alexpott		roderik: deleteRevisionsLanguage is not deleting anything

1. +++ b/core/modules/node/lib/Drupal/node/NodeStorage.php
   @@ -0,0 +1,64 @@
   +  public function revisionIds(NodeInterface $node) {
   +    return $this->database->query(
   +      'SELECT vid FROM {node_revision} WHERE nid=:nid ORDER BY vid',
   +      array(':nid' => $node->id())
   +    )->fetchCol();
   +  }
   ...
   +  public function userRevisions(AccountInterface $account) {
   +    return $this->database->query(
   +      'SELECT vid FROM {node_field_revision} WHERE uid = :uid ORDER BY vid',
   +      array(':uid' => $account->id())
   +    )->fetchCol();
   +  }
   ...
   +  public function deleteRevisionsLanguage($language) {
   

   The names of these functions are inconsistent - both return a list of revision IDs

2. +++ b/core/modules/node/lib/Drupal/node/NodeStorage.php
   @@ -0,0 +1,64 @@
   +  public function updateType($old_id, $new_id) {
   +    return $this->database->query(
   +      'UPDATE {node} SET type=:new WHERE type=:old',
   +      array(':new' => $new_id, ':old' => $old_id),
   +      array('return' => Database::RETURN_AFFECTED)
   +    );
   +  }
   

   how about old_type, new_type instead of _id

3. +++ b/core/modules/node/lib/Drupal/node/NodeStorage.php
   @@ -0,0 +1,64 @@
   +  public function deleteRevisionsLanguage($language) {
   +    return $this->database->query(
   +      'UPDATE {node_revision} SET langcode=:ns WHERE langcode=:lc',
   +      array(':ns' => Language::LANGCODE_NOT_SPECIFIED, ':lc' => $language->id),
   +      array('return' => Database::RETURN_AFFECTED)
   +    );
   +  }
   

   This function does not delete anything

Thanks. Changed . (Between resetRevisionsLanguage, unsetRevisionsLanguage and clearRevisionsLanguage, I chose the latter.)

Not changing assignee...idk...

Ehm, yeah :)
Fixed, plus some more comment details.

Also reassessed the removal of node_revisions_list() (as a result of thinking about #2068331) and still concluding it should go. Draft change notice still at https://drupal.org/node/2228895.

Oh, meh.

But... why not? It's the recommended notation! :p

*slaps self* Thanks. Also for the Interface comment - better to get the details right.

+  public function updateType($old_type, $new_type) {
+    return $this->database->query(
+      'UPDATE {node} SET type=:new WHERE type=:old',
+      array(':new' => $new_type, ':old' => $old_type),
+      array('return' => Database::RETURN_AFFECTED)
+    );
+  }

+public function clearRevisionsLanguage($language) {
+    return $this->database->query(
+      'UPDATE {node_revision} SET langcode=:ns WHERE langcode=:lc',
+      array(':ns' => Language::LANGCODE_NOT_SPECIFIED, ':lc' => $language->id),
+      array('return' => Database::RETURN_AFFECTED)
+    );

Any reason why these aren't a database->update() query. I understand all the selects being static queries but I thought updates should always use ->update to deal with the differences between db drivers.

Yes only static select queries should use $connection->query(), rest have to stay dynamic.

Ah.

Looks good

Committed/pushed to 8.x, thanks!