Problem/Motivation

Yet another bug unveiled by #2590993: Create stub entities with proper default values! I'm stubbing shortcut entities, which contain a 'link' field with link_type LinkItemInterface::LINK_INTERNAL. generateSampleValues() creates external links only (http://www.foo.info, e.g.), and validating the shortcut entity fails in LinkTypeConstraint because it knows an external URL when it sees one.

Proposed resolution

Fix generateSampleValues() to create a valid-looking internal link when $link_type & LinkItemInterface::LINK_EXTERNAL is FALSE.

Remaining tasks

Do it.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#15 interdiff.txt2.15 KBmikeryan
#15 2605254-15.patch4.87 KBmikeryan
#7 2605254-7.patch4.67 KBmikeryan
#7 2605254-7-FAIL.patch2.46 KBmikeryan
#6 FieldGenerateSampleValueTest.php_.txt2.21 KBmikeryan
#2 2605254-2.patch2.21 KBmikeryan

Comments

mikeryan created an issue. See original summary.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new 2605254-2.patch2.21 KB
mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Issue tags: +rc target triage

We really want this in so we don't have to work around it in #2590993: Create stub entities with proper default values.

dawehner’s picture

dawehner
Primary language German
commented

Seems reasonable, do we have some tests for any other of those random value functions?

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented

I started on a general unit test Friday for the field types that are validated by PrimitiveTypeConstraint, got hung up trying to mock everything though...

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
StatusFileSize
new FieldGenerateSampleValueTest.php_.txt2.21 KB

The test I started on...

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Issue tags: -Needs tests
StatusFileSize
new 2605254-7-FAIL.patch2.46 KB
new 2605254-7.patch4.67 KB

OK, never mind going out of scope with broad tests, let's focus on this issue here...

The last submitted patch, 7: 2605254-7-FAIL.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 7: 2605254-7.patch, failed testing.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs work » Needs review

/me misses the retest button...

The last submitted patch, 7: 2605254-7-FAIL.patch, failed testing.

quietone’s picture

quietone commented 
+++ b/core/modules/link/src/Plugin/Field/FieldType/LinkItem.php
@@ -121,25 +121,30 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
+      $values['uri'] = 'http://www.' . $random->word($domain_length) . '.' . $tlds[mt_rand(0, (sizeof($tlds) - 1))];

use count since sizeof is an alias of count?

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented

use count since sizeof is an alias of count?

The sizeof() is existing code, didn't see a need to change that.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented 
  1. +++ b/core/modules/link/src/Plugin/Field/FieldType/LinkItem.php
@@ -121,25 +121,30 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
+    }
+    else {
+      $values['uri'] = 'base:' . $random->name(mt_rand(1, 64));
     }

    I can see this causing all sorts of chaos if the generated URI is run through the path validation service. Ideally this should just point to a route we know exists, like or a random node or something like that. 

  2. +++ b/core/modules/link/src/Tests/LinkItemTest.php
@@ -39,6 +40,29 @@ protected function setUp() {
+    entity_create('field_storage_config', array(
+      'field_name' => 'field_test_external',
+      'entity_type' => 'entity_test',
+      'type' => 'link',
+    ))->save();
+    entity_create('field_config', array(
+      'entity_type' => 'entity_test',
+      'field_name' => 'field_test_external',
+      'bundle' => 'entity_test',
+      'settings' => ['link_type' => LinkItemInterface::LINK_EXTERNAL],
+    ))->save();
+    entity_create('field_storage_config', array(
+      'field_name' => 'field_test_internal',
+      'entity_type' => 'entity_test',
+      'type' => 'link',
+    ))->save();
+    entity_create('field_config', array(
+      'entity_type' => 'entity_test',
+      'field_name' => 'field_test_internal',
+      'bundle' => 'entity_test',
+      'settings' => ['link_type' => LinkItemInterface::LINK_INTERNAL],

    These should use FieldConfig::create(), not the deprecated entity_create().

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
StatusFileSize
new 2605254-15.patch4.87 KB
new interdiff.txt2.15 KB

I can see this causing all sorts of chaos if the generated URI is run through the path validation service. Ideally this should just point to a route we know exists, like or a random node or something like that.

The purpose of generateSampleValue() is to generate random content - the primary use case today is devel-generate. I see no chaos here - the results of $random->name() will be syntactically correct.

These should use FieldConfig::create(), not the deprecated entity_create().

Done.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Issue tags: -rc target triage +rc target

Talked this over with catch, he said he was fine with this being an rc target since it blocks a migrate critical and is a basic bug fix + tests. Tagging accordingly.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

I think it looks good. Nice and thorough.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed f95cc89 on 8.0.x 
    Issue #2605254 by mikeryan, phenaproxima: LinkItem::generateSamplevalue...

The last submitted patch, 7: 2605254-7-FAIL.patch, failed testing.

  • webchick committed f95cc89 on 8.1.x 
    Issue #2605254 by mikeryan, phenaproxima: LinkItem::generateSamplevalue...

Status: Fixed » Closed (fixed)

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