On testEditNonNodeEntityLink, the $node variable is initialized, but never used.

-    $node = $this->drupalCreateNode(['wrong link target']);
-

That variable needs to be removed from the code and one extra space.

Comments

shaktik created an issue. See original summary.

shaktik’s picture

shaktik’s picture

Status: Active » Needs review
shaktik’s picture

Title: Remove Unused variable $node from LinkFieldTest.php file » Remove Unused variable $node from link module
longwave’s picture

Title: Remove Unused variable $node from link module » Remove unused variable $node from link module
Status: Needs review » Reviewed & tested by the community
lauriii’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

Hardik_Patel_12’s picture

Assigned: Unassigned » Hardik_Patel_12
Hardik_Patel_12’s picture

Assigned: Hardik_Patel_12 » Unassigned
Status: Needs work » Needs review
FileSize
810 bytes
662 bytes

Kindly review a patch.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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.

Berdir’s picture

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.

mradcliffe’s picture

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".

ultrabob’s picture

Status: Needs work » Needs review
FileSize
13.64 KB
811 bytes

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.

ultrabob’s picture

FileSize
831 bytes

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

nijolawrence’s picture

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

antojose’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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']);

kiamlaluno’s picture

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.)

ultrabob’s picture

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

ultrabob’s picture

Status: Needs work » Needs review
FileSize
838 bytes

Here's a patch addressing #18 and 19

nijolawrence’s picture

Status: Needs review » Needs work

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.
ultrabob’s picture

Status: Needs work » Needs review
FileSize
846 bytes

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.

nijolawrence’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs issue summary update

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.

  • alexpott committed f884044 on 9.1.x
    Issue #3157919 by ultrabob, Hardik_Patel_12, shaktik, nijolawrence,...

  • alexpott committed f83bd89 on 9.0.x
    Issue #3157919 by ultrabob, Hardik_Patel_12, shaktik, nijolawrence,...

  • alexpott committed 1b823bc on 8.9.x
    Issue #3157919 by ultrabob, Hardik_Patel_12, shaktik, nijolawrence,...

Status: Fixed » Closed (fixed)

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