Support from Acquia helps fund testing for Drupal Acquia logo

Comments

artreaktor created an issue. See original summary.

artreaktor’s picture

Status: Active » Needs review
FileSize
615 bytes
dawehner’s picture

Issue tags: +Needs tests

Thank you for the fix! For getting this committed we need some additional tests though.
For the additional tests, I think extending \Drupal\Tests\link\Functional\LinkFieldTest would be the right place.

borisson_’s picture

Status: Needs review » Needs work
Issue tags: -link

Setting to needs work based on #3

mayurjadhav’s picture

Issue tags: +DrupalMumbaiCodeSprint

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dhirendra.mishra’s picture

Assigned: Unassigned » dhirendra.mishra
dhirendra.mishra’s picture

Assigned: dhirendra.mishra » Unassigned
chiranjeeb2410’s picture

Status: Needs work » Needs review
FileSize
916 bytes

Extended \Drupal\Tests\link\Functional\LinkFieldTest as mentioned in #2. Hope for a green.
Please review.

cilefen’s picture

Status: Needs review » Needs work

It looks as though you imported a test class but we need a test.

chiranjeeb2410’s picture

@cilefen,

Any pointers on implementing that. Could use some advice.

cilefen’s picture

#3 has some advice. You can modify that test class to cover that line.

ankitjain28may’s picture

@cilefen Can you please let me know what tests are required to write in this \Drupal\Tests\link\Functional\LinkFieldTest to validate, Should i create another function to validate or include this in this testURLValidation by adding more key/value pairs to this array valid_internal_entries ?

ankitjain28may’s picture

  1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -7,6 +7,7 @@
    +use Drupal\Tests\link\Functional\LinkFieldTest;
    

    We need to include new tests in that file to validate the proposed solution to the problem, not to include the file.

  2. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -100,6 +101,7 @@ protected static function getUriAsDisplayableString($uri) {
    +    $string = trim($string);
    

    We need not an extra statement, we can add trim function directly as--
    $uri = trim($string);

The last submitted patch, 15: 2927723-15-test-only.patch, failed testing. View results

ankitjain28may’s picture

The patch in #15 looks good to me.

artreaktor’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests, -DrupalMumbaiCodeSprint
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed b98cdcad61 to 8.6.x and 4c944ba64e to 8.5.x. Thanks!

Backported to 8.5.x because this is a bugfix with no BC implications.

  • alexpott committed b98cdca on 8.6.x
    Issue #2927723 by longwave, artreaktor, chiranjeeb2410, ankitjain28may,...

  • alexpott committed 4c944ba on 8.5.x
    Issue #2927723 by longwave, artreaktor, chiranjeeb2410, ankitjain28may,...
Wim Leers’s picture

Issue tags: +Usability

Nice usability improvement 👌 Thanks!

Status: Fixed » Closed (fixed)

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