Remove unused variable $node from link module

+++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
@@ -727,8 +727,6 @@ public function testEditNonNodeEntityLink() {
-    $node = $this->drupalCreateNode(['wrong link target']);
-

Since $this->drupalCreateNode() creates a node as a side effect, we should probably keep the call in place even though we remove the variable.

Kindly review a patch.

I am not quite sure the test is testing what it thinks it is testing, if it still works with the node creation step removed entirely, but still, the unused variable is gone.

See #2804391: Resaving menu links that points to a non-node entity changes the type to node and breaks the link - this is test what it thinks it is testing. However the existence of the node has no impact on whether the test fails if you revert the logic in core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php to what is was before - therefore I vote for removing the node creation as it is irrelevant.

However that would mean fixing the comment too...

    // Create a node and a test entity to have a possibly valid reference for
    // both. Create another test entity that references the first test entity.

I'll ping @Berdir for an opinion.

Hm. Unsure. Agreed that the test does in fact fail without creating that node (and the argument we pass in is bogus, missing the title key), There are some slight differences though in the way it is broken. What I'm surprised about right now is that "foo (1)" still accepts and stores it as a entity:node/1 even if there is no node 1. An entity reference field would have a validation fail if you try to same. So it's kind of uncovered an additional bug, that we don't have the same valid reference validation on link-widget-with-an-entity-route as an actual entity reference field.

My gut reaction is to keep the node, maybe with an extra comment that explains that we ensure having a node with the same ID. And fix the wrong key for the node *and* also have a separate name for the two entity_test entities that we create, which both declare themself as "correct link target" now.

I think that this issue still could be a Novice issue because there's enough information to act on. The first step is to update the issue summary based on recent comments, and then make sure we're creating a patch that is "testing what we think we're testing".

I sought advice from @berdir about what is needed to move this ticket forward.

we can remove $node, but we should also add a comment that explains we we create it. That we create a node with the same ID to make sure it doesn't end up matching that

Given that I've added a comment to the previous patch to indicate why that node is being created.

I messed up the patch the first time, re-uploading.

Tested the patch. Patch applies without any issues. The changes have been made as per @berdir comments.

nijolawrence missed to update the status as 'Reviewed & tested By the community', while working on this as part of the mentored contribution sprints post DrupalCon Global 2020.

+++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
@@ -727,7 +727,8 @@ public function testEditNonNodeEntityLink() {
+    $this->drupalCreateNode(['wrong link target']);

As per @Berdir let's add the title key... this should be $this->drupalCreateNode(['title' => 'wrong link target']);

Just nitpicking: The comment should end with a period and it should not be longer than 80 characters; id should be ID; in to ensure that, that is not necessary; it's not clear what is that in the link doesn't match that.

-    $node = $this->drupalCreateNode(['wrong link target']);
+    // Create a node that should have the same ID as the test entity to ensure
      // the link doesn't match it.
+    $this->drupalCreateNode(['title' => 'wrong link target']);

(It still confusing, as it could refer to ID or test entity. It would be better to make that more explicit.)

Thanks for the feedback @alexpott and @kiamlaluno. I'll submit a fix shortly.

Here's a patch addressing #18 and 19

Tested the patch. Patch applies without any issues.

Changes mentioned by @alexpott had been corrected.

Two more changes have to be made as per @kiamlaluno
1. The comment should not be longer than 80 characters.
2. id should be ID

// Create a node that should have the same ID as the test entity to ensure
// the link doesn't match it.

That's embarrassing. I assumed my IDE was set up to show me the 80 character width, but that setting was off. Here is a patch that actually addresses @kiamlaluno's comment.

Tested the patch. Patch applies without any issues. The changes have been made as per @alexpott and @kiamlaluno comments.

Committed and pushed f884044d46 to 9.1.x and f83bd898fd to 9.0.x and 1b823bcd8c to 8.9.x. Thanks!

Backported to 8.9.x as this is a test only change.