Problem/Motivation

If we save a link field just with the uri then the default options are set to NULL instead of [].
In core shortcuts are saved this way see standard_install().
It's major because it causes a bug in a major task #2502151: Convert shortcut block to view.

Proposed resolution

If default options value is not set in LinkItem::setValue() then set it to [].

Remaining tasks

Finalize the fix.
Review it.
Commit it.

User interface changes

None.

API changes

None

CommentFileSizeAuthor
#19 link_field_default-2502913-19.patch3 KBjibran
#19 link_field_default-2502913-19-test-only.patch1.89 KBjibran
#19 interdiff.txt3.45 KBjibran
#15 link_field_default-2502913-15-test-only.patch928 bytesjibran
#15 link_field_default-2502913-15.patch1.73 KBjibran
#15 interdiff.txt1.89 KBjibran
#7 link_field_default-2502913-7.patch1.31 KBjibran
#7 interdiff.txt1.28 KBjibran
#3 link_field_default-2502913-3.patch1.9 KBjibran
#3 interdiff.txt627 bytesjibran
linkitem-default-options-test-only.patch689 bytesjibran
linkitem-default-options.patch1.28 KBjibran
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, linkitem-default-options-test-only.patch, failed testing.

The last submitted patch, linkitem-default-options.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff.txt627 bytes
link_field_default-2502913-3.patch1.9 KB

Nice found one more issue.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan as a volunteer commented
Status: Needs review » Reviewed & tested by the community
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan as a volunteer commented

Manually tested too

Screenshot https://www.dropbox.com/s/2y5y35qeihx32kv/Screenshot%202015-06-10%2007.4...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/system/src/Tests/System/TrustedHostsTest.php
@@ -97,7 +97,7 @@ public function testShortcut() {
-      'link' => 'internal:/admin/reports/status',
+      'link' => ['uri' => 'internal:/admin/reports/status'],

Why is this change necessary - isn't uri the main property?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
FileSize
interdiff.txt1.28 KB
link_field_default-2502913-7.patch1.31 KB

How about now?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan as a volunteer commented
Status: Needs review » Reviewed & tested by the community

back again

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: link_field_default-2502913-7.patch, failed testing.

Status: Needs work » Needs review

jibran queued 7: link_field_default-2502913-7.patch for re-testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/link/src/Plugin/Field/FieldType/LinkItem.php
@@ -184,6 +184,9 @@ public function setValue($values, $notify = TRUE) {
+    if (!is_string($values) && !isset($values['options'])) {
+      $values['options'] = [];
+    }

I'm not sure that the !is_string($values) is actually necessary. If it is a string $values['options'] will be set.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Status: Needs work » Needs review

@alexpott: So you are proposing to rely on PHP's behavior that $foo = 'abc'; isset($foo['bar']); returns TRUE? I have to strongly disagree, I think it makes the code wildly less readable. When you read isset($foo['bar']) no one would expect $foo to be a string. I'm fine with changing the !isset() to is_array() if people prefer that, but...

Marking "needs review" to get some more opinions on this.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan as a volunteer commented

Agree with @tstoeckler

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
FileSize
interdiff.txt1.89 KB
link_field_default-2502913-15.patch1.73 KB
link_field_default-2502913-15-test-only.patch928 bytes

Thanks for the suggestion @alexpott and @tstoeckler. Thank you @larowlan for helping me understand the issue.

  1. Added another test case.
  2. Updated the if condition.
  3. Added default values.

Status: Needs review » Needs work

The last submitted patch, 15: link_field_default-2502913-15-test-only.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Issue summary: View changes
Status: Needs work » Needs review
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs review » Needs work
  1. I like the new approach much cleaner.
  2. +++ b/core/modules/link/src/Plugin/Field/FieldType/LinkItem.php
@@ -177,6 +177,15 @@ public function getUrl() {
+      $values = ['uri' => $values];

    I think we should use $this->mainPropertyName() instead of hardcoding 'uri' here. 

  3. +++ b/core/modules/link/src/Plugin/Field/FieldType/LinkItem.php
@@ -177,6 +177,15 @@ public function getUrl() {
+      'title' => NULL,

    Why add title? It's optional? 

  4.     if (isset($values['options']) && is_string($values['options'])) {
      $values['options'] = unserialize($values['options']);
    }

    No longer necessary to check if $values['options'] is set - it will always be.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff.txt3.45 KB
link_field_default-2502913-19-test-only.patch1.89 KB
link_field_default-2502913-19.patch3 KB

Thanks for the review @alexpott. Fixed #18 added two more test cases. Updated docs.

Mac_Weber’s picture

Mac_Weber CreditAttribution: Mac_Weber as a volunteer commented
Status: Needs review » Reviewed & tested by the community

looks good

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed e5d87b1 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/link/src/Tests/LinkItemTest.php b/core/modules/link/src/Tests/LinkItemTest.php
index a476853..382a464 100644
--- a/core/modules/link/src/Tests/LinkItemTest.php
+++ b/core/modules/link/src/Tests/LinkItemTest.php
@@ -130,7 +130,6 @@ public function testLinkItem() {
     $entity->field_test[0] = NULL;
     $this->assertNull($entity->field_test[0]->getValue());
 
-
     // Test the generateSampleValue() method.
     $entity = entity_create('entity_test');
     $entity->field_test->generateSampleItems();

Removed unnecessary empty line on commit.

  • alexpott committed e5d87b1 on 8.0.x 
    Issue #2502913 by jibran, larowlan: Link field default options values...
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented

Thanks @alexpott for the commit. Thanks @Mac_Weber for RTBC.

Status: Fixed » Closed (fixed)

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