Remove those functions.

Replace all node_save($node) calls with $node->save(), the same for node_delete().

Remove node_delete_multiple($ids) with entity_delete_multiple('node', $ids).

Replace comment references with \Drupal\Core\Entity::save() and so on.

Files: 
CommentFileSizeAuthor
#39 replace_node_save_functions-1999322-39.patch25.75 KBalexander.ilivanov
PASSED: [[SimpleTest]]: [MySQL] 55,858 pass(es).
[ View ]
#28 1999322-28-node_save-and-node_delete-conversions.patch25.74 KBelvis2
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1999322-28-node_save-and-node_delete-conversions.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#26 1999322-26-take2-node_save-and-node_delete-conversions.patch25.82 KBelvis2
PASSED: [[SimpleTest]]: [MySQL] 55,884 pass(es).
[ View ]
#24 1999322-24-node_save-and-node_delete-conversions.patch24.13 KBelvis2
FAILED: [[SimpleTest]]: [MySQL] 55,912 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#18 1999322-18-convert-node_delete_multiple.patch4.94 KBelvis2
FAILED: [[SimpleTest]]: [MySQL] 56,540 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#16 1999322-15-converted-node_delete_multiple.patch4.92 KBelvis2
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/node.module.
[ View ]
#9 1999322-9-converted-node-save-functions.patch13.14 KBelvis2
FAILED: [[SimpleTest]]: [MySQL] 55,857 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#4 1999322-2-changed_node_save_functions.patch6.84 KBelvis2
FAILED: [[SimpleTest]]: [MySQL] 56,041 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#2 1999322-2-changed_node_save_functions.patch0 byteselvis2
PASSED: [[SimpleTest]]: [MySQL] 55,862 pass(es).
[ View ]

Comments

elvis2’s picture

Assigned:Unassigned» elvis2
elvis2’s picture

StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,862 pass(es).
[ View ]

This patch takes care of the node_save uses in all files except:
/drupal/core/modules/search/lib/Drupal/search/Tests/SearchCommentCountToggleTest.php

lines 91, 93, 101, 103

Looking purely at the code, it seems those lines would break since node_save is expecting the $node object, so I hesitated to touch this for the time being. I will come back to it this week.

At this point, should function node_save($node) in node.module be removed or should this be left in place for contrib modules to have a fallback on upgrading from d7 to d8?

I am doing the node_delete fixes separately.

elvis2’s picture

Status:Active» Needs review
elvis2’s picture

StatusFileSize
new6.84 KB
FAILED: [[SimpleTest]]: [MySQL] 56,041 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Let's try that again...

Berdir’s picture

Thanks for working on this.

Yes, the functions should be removed.

You should be able to convert those in that test as well. If you can call node_save($whatever['thing']), then $whatever['think'] is a node object, however it might be named, so you can call ->save() on that and it should work.

elvis2’s picture

@Berdir node_save is referenced from a page callback in:
/drupal/core/modules/system/tests/modules/menu_test/menu_test.module

within menu_test_menu.

How should that be handled?

Status:Needs review» Needs work

The last submitted patch, 1999322-2-changed_node_save_functions.patch, failed testing.

Berdir’s picture

Weird. I can't find a request that actually uses that in the tests, only one that loads it. Try to change it to menu_test_callback and then let's see if anything breaks.

elvis2’s picture

Status:Needs work» Needs review
StatusFileSize
new13.14 KB
FAILED: [[SimpleTest]]: [MySQL] 55,857 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Ok, all node_save() have been converted and documentation in node.api.php updated.

@Berdir I changed the menu callback as you suggested.

Status:Needs review» Needs work
Issue tags:-Novice, -Entity Field API

The last submitted patch, 1999322-9-converted-node-save-functions.patch, failed testing.

elvis2’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Novice, +Entity Field API

The last submitted patch, 1999322-9-converted-node-save-functions.patch, failed testing.

Berdir’s picture

index 0000000..5080fa9
--- /dev/null

--- /dev/null
+++ b/.gitignoreundefined

+++ b/.gitignoreundefined
@@ -0,0 +1,2 @@

@@ -0,0 +1,2 @@
+sites
+sites/*
\ No newline at end of file

Looks like you accidently added your .gitignore file to the patch. I suggest you add .gitignore to .gitignore, then it will ignore itself or add these rules to .git/info/exclude

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentLinksTest.phpundefined
@@ -168,7 +168,8 @@ function setEnvironment(array $info) {
-      node_save($this->node);
+      $node = $this->node;
+      $node->save();

You don't need to assign it to $node, you can simply call $this->node->save().

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeRevisionPermissionsTest.phpundefined
@@ -51,7 +51,8 @@ function setUp() {
-        node_save($revision);
+        $node = node_load($revision->nid);
+        $node->save();

Same here. Everything that's passed to node_save() must be a node object, even if it's not named like that, so $revision->save() should work fine.

+++ b/core/modules/node/node.api.phpundefined
@@ -39,7 +39,7 @@
- * - Creating a new node (calling node_save() on a new node):
+ * - Creating a new node (calling $node->save() on a new node):

This looks quite readable but I'm not sure if this is valid, will have to check with someone that knows these documentation rules better.

+++ b/core/modules/node/node.api.phpundefined
@@ -518,7 +518,7 @@ function hook_node_revision_delete(\Drupal\Core\Entity\EntityInterface $node) {
- * This hook is invoked from node_save() after the database query that will
+ * This hook is invoked from $node->save() after the database query that will

This is actually not really true anymore. It should simply say something like "from the storage controller after the node was saved". If you're not sure, leave this part for now and I'll try to re-work it later.
Same for the other hook documentations in node.api.php

Looks good otherwise, I guess you still need to convert the node_save()'s in that search comment test. If you click on view details and then expand the detail log at the end, you can see that it dies with a fatal error trying to call that function somewhere.

elvis2’s picture

Thanks for your tips Berdir. I will make those changes later this evening.

I am glad you brought up the doc stuff, as I have been questioning myself if that is the right way. Especially now that I am doing the same thing, pretty much, for node_delete documentation. I will add that patch this evening too.

Thanks for your help.

Berdir’s picture

Thanks for working on this, appreciated!

Note that you can upload the node_delete() changes as part of the same patch. Except we want to define that we should open a separate issue for it. Which might not be the dumbest idea (the only problem is that it could result in unnecessary conflicts between the two patches).

So if you want to, feel free to edit the original issue title and description, remove delete and create a separate issue for it. Your decision :)

elvis2’s picture

Status:Needs work» Needs review
StatusFileSize
new4.92 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/node.module.
[ View ]

Here is the patch for the conversion of node_delete_multiple to entity_delete_multiple

Status:Needs review» Needs work

The last submitted patch, 1999322-15-converted-node_delete_multiple.patch, failed testing.

elvis2’s picture

Status:Needs work» Needs review
StatusFileSize
new4.94 KB
FAILED: [[SimpleTest]]: [MySQL] 56,540 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Ok, let's try that again.

elvis2’s picture

Berdir, okay, the next patch I will combine both sets of changes.

Berdir’s picture

+++ b/core/modules/node/node.moduleundefined
@@ -1001,21 +1001,9 @@ function node_save(EntityInterface $node) {
function node_delete($nid) {
-  node_delete_multiple(array($nid));
+  entity_delete_multiple('node', array($nid));

You can remove this as well and call $node->delete() instead. As this does just receive the node id, you might have to add some node_load()'s but only where you don't have a node object already.

elvis2’s picture

@Berdir, so to be clear, where ever node_delete is called convert those to be $node->delete() ?

Berdir’s picture

Exactly, $node->delete() instead of node_delete($node->nid).

Status:Needs review» Needs work

The last submitted patch, 1999322-18-convert-node_delete_multiple.patch, failed testing.

elvis2’s picture

StatusFileSize
new24.13 KB
FAILED: [[SimpleTest]]: [MySQL] 55,912 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This patch has both conversions, node_save (to $node->save()) and node_delete (to $node->delete()).

I wanted to ask, do the test bots fail sometimes due to false positives? The last patch failed from the testbot but on my local box (after "cleaning environment" in simpletest) I had no errors, just two warnings.

The documentation needs attention. I am not familiar with the d8 documentation standards. For example, in d7 node_save($node) was mentioned in node.api.php. I think there is a more proper way to document the save() method and references. Can I get some help on that or point me to some examples?

Thanks berdir and timplunkett for your help.

elvis2’s picture

Status:Needs work» Needs review
elvis2’s picture

StatusFileSize
new25.82 KB
PASSED: [[SimpleTest]]: [MySQL] 55,884 pass(es).
[ View ]

Trying again. I missed a few node_save()'s...

Berdir’s picture

Status:Needs review» Needs work

Looks good, a few (like 3 I think) cases that can be simplified, see #13.

Will try to find someone tomorrow for the documentation thing, I'm not sure myself. The problem with $node->save(), although it makes kinda sense is that it's not referencable to anything. But maybe that's fine, or maybe we just want to make some proper english out of it, e.g. "while saving a node". But there are some references that are supposed to reference where this actually happens, but they stopped making sense a long time ago anyway (node_save() isn't where the fun stuff happens, that's in the storag controller but that's hard to reference).

elvis2’s picture

Status:Needs work» Needs review
StatusFileSize
new25.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1999322-28-node_save-and-node_delete-conversions.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thanks Berdir, I have simplified those 3.

Assuming this attached patch passes, afterwards, should I change the title of this issue? What is the proper protocol?

Status:Needs review» Needs work
Issue tags:-Novice, -Entity Field API

The last submitted patch, 1999322-28-node_save-and-node_delete-conversions.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1999322-28-node_save-and-node_delete-conversions.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
Issue tags:+Novice, +Entity Field API
Berdir’s picture

Status:Needs review» Reviewed & tested by the community

Ok, this looks good. Discussed this with a few people and this is good to go.

Improving the hook documentation there to something that makes sense should happen in #1824184: Remove and replace entity type specific API hook documentation with generic entity API hook documentation

elvis2’s picture

Good to hear Berdir. This was my first time patching for core. Thanks for all your help...

catch’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Novice, +Entity Field API

The last submitted patch, 1999322-28-node_save-and-node_delete-conversions.patch, failed testing.

elvis2’s picture

@catch I need to reroll this, core has changed enough that this patch fails... I will do this Wednesday evening PST.

alexander.ilivanov’s picture

Will be done today during Code Sprint UA.

alexander.ilivanov’s picture

Status:Needs work» Needs review
Issue tags:+CodeSprintUA
StatusFileSize
new25.75 KB
PASSED: [[SimpleTest]]: [MySQL] 55,858 pass(es).
[ View ]

Replaced node_save(), node_delete(), node_delete_multiple()
Patch attached.

elvis2’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-CodeSprintUA

@alexander.ilivanov, nice work!

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

podarok’s picture

Issue tags:+CodeSprintUA

back to tag
Thanks for Your work @alexander.ilivanov

Status:Fixed» Closed (fixed)
Issue tags:-CodeSprintUA

Automatically closed -- issue fixed for 2 weeks with no activity.

Gábor Hojtsy’s picture

Title:Remove node_save() and node_delete()/node_delete_multiple() in favor of $node->save()/$node->delete()» Change notice: Remove node_save() and node_delete()/node_delete_multiple() in favor of $node->save()/$node->delete()
Priority:Normal» Critical
Status:Closed (fixed)» Active
Issue tags:+Needs change record

No change notice was created for this issue.

Berdir’s picture

Title:Change notice: Remove node_save() and node_delete()/node_delete_multiple() in favor of $node->save()/$node->delete()» Remove node_save() and node_delete()/node_delete_multiple() in favor of $node->save()/$node->delete()
Priority:Critical» Normal

We already have https://drupal.org/node/1400186, added a list of related removed functions there.

Adding every issue that's related to that is going to be too much...

Berdir’s picture

Issue tags:-Needs change record

Removing tag.

Berdir’s picture

Status:Active» Fixed
Gábor Hojtsy’s picture

I think thats a great solution. However wiring up that change notice to this issue seems important for posterity, so (a) its evident this issue is covered (b) people find the change explained in context from the issue summary. Added this issue to the list of related issues there.

Automatically closed -- issue fixed for 2 weeks with no activity.