Updated: Comment #64

Problem/Motivation

The contrib link modules Link, URL Field and CKEditor Link supports both internal and external URLs and a lot of people use the module for internal links. Right now, the storage for the core link field data and the default widget only supports external URLs.

Steps to reproduce the issue:

  1. Install Drupal 8.x
  2. Enable the field type 'Link' in /admin/modules
  3. Goto manage fields for one of the content types in /admin/structure/types
  4. Add a new field with existing field type 'Link' and save
  5. In edit tab scroll down to see the bug

Below is the attached screenshot :
before

Proposed resolution

Implement plan C, described below.

Updated: After screenshot implementing plan C (provided on #64)
after

Remaining tasks

None.

User interface changes

A new option appears in the link field instance edit page. See screenshot from comment #64.

API changes

- both internal and external URLs can be stored (and edited in the default widget) by the link field
- the field type plugin receives its own interface (LinkItemInterface), which contains an isExternal() helper method

Original report by @Dave Reid

Current summary from #1778858: Decide on built-in support for internal URLs which was filed against the URL module issue queue:

Plan A:

  • Decide we only want to support external URLs in the widget and change nothing. Leave internal URL widget for contrib.
  • Pros: Lowest complexity
  • Cons: Users may find out-of-the-box solution limiting.

Plan B:

  • Rename the current widget from 'URL field' to 'External URL field' and add a new 'Internal URL field' widget.
  • Pros: we support both, not too much extra code
  • Cons: this makes a specific instance only able to use one or the other and not mix.

Plan C:

  • Add an option on the field instance to have the administrator say if this field supports internal only, external only, or both. The default widget will change it's validation based on the options.
  • Pros: Able to mix/match. Seems better as a field instance setting, not a widget setting.
  • Cons: Most amount of complexity to add. Widget will not be able to support HTML5 'url' FAPI type if option is not 'external only' since the url field by nature only supports external fields.

Plan D:

  • Always use the FAPI 'url' type, and attempt to resolve what are 'local' URLs to un-aliased internal Drupal URLs.
  • Pros: Simplicity. We are able to handle both types with the same widget and use the new HTML5 FAPI type always.
  • Cons: Medium complexity.
  • How does this work for sites that have multiple domains (multisite or domain access) or SSL? Would we still want to add additional logic for 'Allow the following types of URLs: [X] Internal [X] External'?
CommentFileSizeAuthor
#116 interdiff.txt1.14 KBdawehner
#116 internal_url-2054011-116.patch49.7 KBdawehner
#113 interdiff.txt1.32 KBdawehner
#113 internal_url-2054011-113.patch49.7 KBdawehner
#112 interdiff.txt1.62 KBdawehner
#112 internal_url-2054011-112.patch49.56 KBdawehner
#107 interdiff.txt1.53 KBtim.plunkett
#107 internal_url-2054011-107.patch49.51 KBtim.plunkett
#105 interdiff.txt6.65 KBdawehner
#105 internal_url-2054011-105.patch48.3 KBdawehner
#101 interdiff.txt1.64 KBdawehner
#101 internal_url-2054011-101.patch51.55 KBdawehner
#100 internal-url-2054011-100.patch51.45 KBpwolanin
#100 increment.txt8.01 KBpwolanin
#93 internal-url-2054011-93.patch51.16 KBpwolanin
#87 internal-url-2054011-87.patch48.87 KBpwolanin
#87 increment.txt3.52 KBpwolanin
#83 internal-url-2054011-83.patch45.59 KBpwolanin
#83 increment.txt741 bytespwolanin
#82 internal-url-2054011-82.patch45.53 KBpwolanin
#82 increment.txt1.2 KBpwolanin
#80 internal-url-2054011-80.patch44.32 KBpwolanin
#80 increment.txt5.31 KBpwolanin
#76 internal-url-2054011-76.patch42.15 KBpwolanin
#76 increment.txt12.34 KBpwolanin
#71 before2054011.png20.11 KBJayeshSolanki
#70 interdiff.txt5.51 KBblueminds
#70 link-internal-url-support-2054011-70.patch38.71 KBblueminds
#64 link-instance-option.png28.11 KBamateescu
#64 interdiff.txt739 bytesamateescu
#64 link-internal-url-support-2054011-64.patch36.82 KBamateescu
#62 interdiff.txt4.1 KBamateescu
#62 url-2054011-62.patch36.88 KBamateescu
#60 url-2054011-60.patch35.82 KBeffulgentsia
#58 interdiff.txt1.12 KBdawehner
#58 url-2054011-58.patch35.82 KBdawehner
#57 interdiff-2054011-55-57.txt1.77 KBYesCT
#57 2054011-link_internal_url_support-57.patch35.72 KBYesCT
#55 interdiff-2054011-53-55.txt3.98 KBYesCT
#55 2054011-link_internal_url_support-55.patch34.55 KBYesCT
#53 2054011-link_internal_url_support-53-interdiff.txt8.33 KBBerdir
#53 2054011-link_internal_url_support-53.patch34.44 KBBerdir
#49 interdiff.txt2.75 KBamateescu
#49 2054011-link_internal_url_support-49.patch34.32 KBamateescu
#43 2054011-link_internal_url_support-43.patch34.25 KBamateescu
#38 interdiff.txt8.66 KBamateescu
#38 2054011-link_internal_url_support-38.patch34.85 KBamateescu
#35 interdiff-2054011-32-35-do-not-test.diff574 bytesdas-peter
#35 2054011-link_internal_url_support-35.patch29.55 KBdas-peter
#32 interdiff.txt7.73 KBamateescu
#32 2054011-link_internal_url_support-32.patch29.5 KBamateescu
#26 interdiff.txt10.55 KBamateescu
#26 2054011-link_internal_url_support-26.patch26 KBamateescu
#23 2054011-link_internal_url_support.patch20.52 KBamateescu
#3 link-2054011-3.patch6.83 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

Here's a very good reason why we really need to do this: #2051889-7: Add route parameters property to menu links

klonos’s picture

Issue summary: View changes

..link to the link module ;)

dawehner’s picture

Issue tags: +MenuSystemRevamp

.

So to sump up the other issue:

Plan D

  • Store route name and route parameters
  • Use the url generator/link generator to generate finally the link based upon the two route
  • On the actual UI level you can still both specify the external/internal link and in case of an internal link, convert the path to route name/parameters.
dawehner’s picture

FileSize
6.83 KB

@amateescu
What are your plans to move this forward?

Personally the only option from A to D seems C for me. Given the problem that html5 validation is problematic anyway I think it is fine to drop support it for non-external URLs. As it uses the FAPI type url, you need to hack around for internal URLs in quite some places.

PS: Please ignore me previous comment ...
PS2: This is just a rough outline for the route/route_parameter part.... happy to discuss how to do it properly.

amateescu’s picture

@dawehner, I don't have any plans, not anymore.

dawehner’s picture

Does that mean that we should better move the route parameter logic just into MenuLink, so it is easier to get it in at the end of the day?

amateescu’s picture

If you think that's easier, sure. If you're asking what I'd prefer, it's still fixing this issue :)

amateescu’s picture

Issue summary: View changes

...not its issue queue.

amateescu’s picture

Assigned: Unassigned » amateescu
Issue summary: View changes
Issue tags: +Entity Field API, +sprint

This is the first step needed to fix the performance problems from #1842858: [PP-1] Convert menu links to the new Entity Field API. If we add internal url support for the link field, we would be able to group 5 columns (which are now 5 individual base fields) from the menu_links table into one base field.

anavarre’s picture

Issue summary: View changes
anavarre’s picture

Issue summary: View changes
anavarre’s picture

Added https://drupal.org/project/ckeditor_link which supports internal links as well.

benjy’s picture

Priority: Normal » Critical
Issue tags: +beta blocker

Adding beta blocker tag and bumping to critical since this is blocking: #1842858: [PP-1] Convert menu links to the new Entity Field API

effulgentsia’s picture

If we add internal url support for the link field, we would be able to group 5 columns (which are now 5 individual base fields) from the menu_links table into one base field.

Are all plans A-D compatible with that goal? What would be each plan's impact to the menu link code, especially with regards to this quote from #2051889-7: Add route parameters property to menu links:

This was also one my original goals in the menu link entity conversion, to be able to leverage the field system and drop a lot of custom code.

Crell’s picture

At least in concept, options B or C seem the most viable to me. The unknowns and mismatches around D worry me, and we do need to have internal links in core so that rules out A.

The main question is whether forcing internal and external links to be separate fields poses a sufficient problem to justify the extra work of merging them.

However... if menu entities use a link field, they already support both internal and external links. That means we have a use case in core that demands a single field, because it would be a regression otherwise.

So I think that leaves us with C.

amateescu’s picture

I also think C is our best / only option if we want to be able to use this field for menu links. I'll start working on this next week, when I come back from vacation :)

amateescu’s picture

Sorry that I forgot to mention (I said it so many times in IRC that I thought I did it here as well), I'm waiting for #2153891: Add a Url value object before moving forward here.

klonos’s picture

Status: Active » Postponed
Related issues: +#2153891: Add a Url value object

...better status then ;)

...though I guess it won't take long since the other issue is RTBC.

xjm’s picture

Status: Postponed » Active

Unblocked. :)

timodwhit’s picture

+1 to Option C, FWIW. :)

lee20’s picture

+1 to Option C

rymcveigh’s picture

+1 to Option C

chrisolof’s picture

Issue summary: View changes

Added Link to the list of modules providing this functionality in the contrib space.

amateescu’s picture

Title: Decide on built-in support for internal URLs » Implement built-in support for internal URLs
Status: Active » Needs review
Issue tags: +Needs tests

Here's an initial patch that implements option C. No tests yet as I'd prefer to get an approval for this direction and code reviews before writing them.

amateescu’s picture

FileSize
20.52 KB

And the patch :)

amateescu’s picture

Status: Needs work » Needs review
FileSize
26 KB
10.55 KB

Impressive test coverage!

das-peter’s picture

@amateescu Damn, I was working on that too because I couldn't reach you in IRC :D But I'm glad to see that you've made the same changes I made.

One addition I made was to add support for schema / protocol relative URL's (http://tools.ietf.org/html/rfc3986#section-4.2 / http://url.spec.whatwg.org/#concept-scheme-relative-url). I talked to Crell about that and he agreed that it's a good idea to support that. However, I think I'll create an extra issue for that so I can review this patch.

amateescu’s picture

Ugh, sorry about that, lots of non-Drupal stuff to do these days so I can't stay in IRC all the time. Having support for schema-relative URLs sounds like a useful addition and I agree that we should open a new issue for it.

das-peter’s picture

  1. +++ b/core/lib/Drupal/Core/Url.php
    @@ -322,9 +331,9 @@ public function toString() {
       public function toArray() {
         return array(
    -      'route_name' => $this->getRouteName(),
    -      'route_parameters' => $this->getRouteParameters(),
    -      'options' => $this->getOptions(),
    +      'route_name' => $this->routeName,
    +      'route_parameters' => $this->routeParameters,
    +      'options' => $this->options,
         );
    

    Shouldn't we return another set of information if it's an external url? Similar to toString() / toRenderArray()?
    (As far as I've seen this is used in LinkWidget::massageFormValues() and I'm not familiar with that)

  2. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -31,14 +34,42 @@ class LinkWidget extends WidgetBase {
    +    // If the field is configured to allow both internal and external paths,
    +    // show a useful description
    

    Inline comments must end in full-stops, exclamation marks, or question marks.

  3. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -125,17 +156,58 @@ public function settingsSummary() {
    +        if ($url->isExternal() && !UrlHelper::isValid($element['url']['#value'], TRUE)) {
    +          form_error($element['url'], $form_state, t('The URL %url is not valid.', array('%url' => $element['url']['#value'])));
    +        }
    ...
    +      catch (\Exception $e) {
    +        form_error($element['url'], $form_state, t('The URL %url is not valid.', array('%url' => $element['url']['#value'])));
    +      }
    

    Shouldn't we use \Drupal::formBuilder()->setError() for new code?

That's it, patch looks pretty good to me - I'm tempted to RTBC it but I think someone else should have a look too.

das-peter’s picture

Btw. #2195983: Support scheme-relative URLs is green by now, reviews appreciated :)

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Url.php
    @@ -335,11 +344,19 @@ public function toArray() {
       public function toRenderArray() {
    

    I wonder whether we should also take the link_title into account? This is maybe more for a followup.

  2. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldType/LinkItem.php
    @@ -117,4 +172,12 @@ public function isEmpty() {
    +    // External URLs don't have a route_name value.
    +    return empty($this->route_name);
    

    This seems to be a fragile logic. Should we not store the external property additional?

  3. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -31,14 +34,42 @@ class LinkWidget extends WidgetBase {
    +      $element['url']['#field_prefix'] = \Drupal::url('<front>', array(), array('absolute' => TRUE));
    

    using an external dependency would have a nice semantic: generating a link requires an url

  4. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -31,14 +34,42 @@ class LinkWidget extends WidgetBase {
    +      $element['url']['#description'] = t('This can be an internal Drupal path such as %add-node or an external URL such as %drupal. Enter %front to link to the front page.', array('%front' => '<front>', '%add-node' => 'node/add', '%drupal' => 'http://drupal.org'));
    
    @@ -125,17 +156,58 @@ public function settingsSummary() {
    +          form_error($element['url'], $form_state, t('The URL %url is not valid.', array('%url' => $element['url']['#value'])));
    ...
    +        form_error($element['url'], $form_state, t('The URL %url is not valid.', array('%url' => $element['url']['#value'])));
    

    Can we use $this->t() directly? Note: form_error is also a deprecated function already.

  5. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -125,17 +156,58 @@ public function settingsSummary() {
    +        catch (\Exception $e) {
    +          // Nothing to do here, validateUrl() already emitted a form validation
    +          // error.
    +        }
    

    Should we rather just match for MatchingRouteNotFoundException, given that there might be exceptions below you don't want to catch if not needed? Url::createFromPath also tells you what it throws.

amateescu’s picture

FileSize
29.5 KB
7.73 KB

Thanks for the reviews!

Re #29:

  1. I was thinking the same thing at some point, I guess I was just lazy. Done.
  2. Fixed.
  3. Fixed.

Re #31:

  1. Drupal/Core/Url has no knowledge of a 'title' property and I'm not sure we want to add it. But a followup to discuss it is fine :)
  2. I prefer to think of it as a clever trick rather than fragile logic. And I did it this way specifically to eliminate the need to store an 'external' property.
  3. Not sure what you mean there..
  4. Fixed.
  5. Sure, I changed that code to catch only the two exceptions we know we want to handle.
das-peter’s picture

Adjusted test to validate with the new return for external URL's of Url::toArray().

das-peter’s picture

Status: Needs work » Needs review
klonos’s picture

...remember to hide old patches/files when uploading new ones ;)

amateescu’s picture

Issue tags: -Needs tests
FileSize
34.85 KB
8.66 KB

So the approach seems fine, let's get some nice tests before RTBC.

dawehner’s picture

using an external dependency would have a nice semantic: generating a link requires an url

Aka. inject the thing.

amateescu’s picture

You mean inject the url generator in LinkWidget? Because widgets are not (yet) set up for having things injected, afaik.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Url.php
    @@ -291,6 +291,15 @@ public function setOption($name, $value) {
       /**
    +   * Returns the path of the URL if it is external.
    +   *
    +   * @return string
    +   */
    +  public function getPath() {
    +    return $this->path;
    +  }
    

    This needs to throw an exception if it's not external (and have an @expectedException test for it!)

  2. +++ b/core/lib/Drupal/Core/Url.php
    @@ -308,10 +317,10 @@ public function setAbsolute($absolute = TRUE) {
    -      return $this->urlGenerator()->generateFromPath($this->path, $this->getOptions());
    +      return $this->urlGenerator()->generateFromPath($this->path, $this->options);
         }
     
    -    return $this->urlGenerator()->generateFromRoute($this->getRouteName(), $this->getRouteParameters(), $this->getOptions());
    +    return $this->urlGenerator()->generateFromRoute($this->routeName, $this->routeParameters, $this->options);
       }
    
    @@ -321,11 +330,19 @@ public function toString() {
    -    return array(
    -      'route_name' => $this->getRouteName(),
    -      'route_parameters' => $this->getRouteParameters(),
    -      'options' => $this->getOptions(),
    -    );
    ...
    +      return array(
    +        'route_name' => $this->routeName,
    +        'route_parameters' => $this->routeParameters,
    +        'options' => $this->options,
    +      );
    
    @@ -335,11 +352,19 @@ public function toArray() {
    -      '#route_name' => $this->getRouteName(),
    -      '#route_parameters' => $this->getRouteParameters(),
    -      '#options' => $this->getOptions(),
    ...
    +        '#route_name' => $this->routeName,
    +        '#route_parameters' => $this->routeParameters,
    +        '#options' => $this->options,
    

    I don't understand these changes at all.

  3. +++ b/core/lib/Drupal/Core/Url.php
    @@ -321,11 +330,19 @@ public function toString() {
        *   An associative array containing all the properties of the route.
    ...
    +    if ($this->external) {
    +      return array(
    +        'path' => $this->path,
    +        'options' => $this->options,
    +      );
    +    }
    

    I really feel uneasy about this. The docblock and the assumptions made by calling code are that this is a route (meaning, internal). If we make this change, we'll be forced to either check isset($whatever['route_name']) or $foo->isExternal() every single time...

  4. +++ b/core/lib/Drupal/Core/Url.php
    @@ -335,11 +352,19 @@ public function toArray() {
    +    if ($this->external) {
    +      return array(
    +        '#href' => $this->path,
    +        '#options' => $this->options,
    +      );
    +    }
    +    else {
    +      return array(
    +        '#route_name' => $this->routeName,
    +        '#route_parameters' => $this->routeParameters,
    +        '#options' => $this->options,
    +      );
    +    }
    

    Now this change I support! No one ever needs to touch this, except for #options, which is common to both.

  5. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldFormatter/LinkFormatter.php
    @@ -121,7 +121,8 @@ public function viewElements(FieldItemListInterface $items) {
    -      $link_title = $item->url;
    +      $url = $this->buildUrl($item);
    +      $link_title = $url->toString();
    

    This could totally be one line :)

  6. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldFormatter/LinkSeparateFormatter.php
    @@ -42,7 +42,8 @@ public function viewElements(FieldItemListInterface $items) {
    +      $url = $this->buildUrl($item);
    +      $link_title = $url->toString();
    
    @@ -58,19 +59,17 @@ public function viewElements(FieldItemListInterface $items) {
    +      $url_title = $url->toString();
    

    Same thing?

amateescu’s picture

  1. getRouteName()/getRouteParameters() do not throw similar exceptions if the url is external..
  2. calling an object's own methods for retrieving its own properties doesn't make any sense, and it's also slower:
        $url = new Url('<front>');
        Timer::start('test');
        for ($i = 1; $i < 1000; $i++) {
          $url->toArray();
        }
        Timer::stop('test');
        debug(Timer::read('test'));
    

    This code gives me the following results:

    before (by calling methods): 0.85 - 0.9 ms
    
    after (by returning the properties directly): 0.46 - 0.5 ms
    
  3. we'll be forced to either check isset($whatever['route_name']) or $foo->isExternal() every single time

    Yes, if the calling code doesn't just pass $url to another function/method that accepts it as an argument, it needs to know if the url is external or not. This is the case since we decided to bake in external support to the Url object..

  4. :)
  5. Nope, the $url object is also needed below in that method.
  6. Same here.
amateescu’s picture

FileSize
34.25 KB

Reroll & ping for reviews/rtbc :)

amateescu’s picture

Status: Needs work » Needs review
jibran’s picture

Seems all good to me. Let's see what @tim.plunkett thinks about the patch.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Url.php
    @@ -308,10 +317,10 @@ public function setAbsolute($absolute = TRUE) {
    -      return $this->urlGenerator()->generateFromPath($this->path, $this->getOptions());
    +      return $this->urlGenerator()->generateFromPath($this->path, $this->options);
    
    @@ -321,11 +330,19 @@ public function toString() {
    +    if ($this->external) {
    
    @@ -335,11 +352,19 @@ public function toArray() {
    +    if ($this->external) {
    

    I still think not using methods internally is a mistake. It will hurt if we ever try to refactor. And some of these changes are just to change it, and completely unrelated to the issue.

  2. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -111,31 +145,88 @@ public function settingsSummary() {
    +      catch (NotFoundHttpException $e) {
    +        $url_is_valid = FALSE;
    +      }
    ...
    +      catch (ParamNotConvertedException $e) {
    +        $url_is_valid = FALSE;
    +      }
    ...
    +        catch (NotFoundHttpException $e) {
    +          // Nothing to do here, validateUrl() emits form validation errors.
    +        }
    

    Can these really be thrown here? I know MatchingRouteNotFoundException is thrown by createFromPath()

    And the second hunk only catches 2 exceptions, not 3.

benjy’s picture

I still think not using methods internally is a mistake. It will hurt if we ever try to refactor.

I totally agree with this. Any performance gain we're getting is outweighed by the maintainability of the code IMO. If we decided we wanted to lazy load them properties or do any logic when retrieving the property, accessing them directly causes us a headache refactoring.

amateescu’s picture

FileSize
34.32 KB
2.75 KB

Re: #47

1. So you guys are seriously saying that updating the code of three dead simple methods (toString(), toArray(), toRenderArray()) whenever we feel like doing anything special with those *protected* variables will "hurt" and "cause a headache"? ... I'm speechless.

2. Yes, they can. Fixed the second hunk.

sun’s picture

Status: Needs review » Reviewed & tested by the community

I do not think we have a clear standard for when to call internal methods and when it is OK to just use internal properties. I think if the property value is (1) guaranteed to exist (e.g., initialized by the constructor) and (2) is trivial in terms of complexity, then a class instance can certainly make the shortcut to use its internal property directly, instead of calling a method.

But in any case, that's an academical detail which should not hold up this fine patch.

Thanks, @amateescu!

Berdir’s picture

Status: Reviewed & tested by the community » Needs work

Sorry for the late review, also haven't read anything, if I'm repeating anything that was discussed already, just point me to it.

Setting back to needs work (sorry), as there are a few things that should be addressed/checked. others are just ideas.

  1. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldType/LinkItem.php
    @@ -18,28 +20,50 @@
    +
    +  /**
    +   * Specifies whether the field supports only internal URLs.
    +   */
    +  const LINK_INTERNAL = 0;
    +
    +  /**
    +   * Specifies whether the field supports only external URLs.
    +   */
    +  const LINK_EXTERNAL = 1;
    +
    +  /**
    +   * Specifies whether the field supports both internal and external URLs.
    +   */
    +  const LINK_GENERIC = 2;
    

    Would it be useful to use constants that can be used as bitwise operations, so that LINK_GENERIC & LINK_EXTERNAL/LINK_INTERNAL => TRUE?

    The constants could also live on the interface as you added one anyway? static:: would still work. And we could be nice and work against an interface and not a specific class.

  2. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldType/LinkItem.php
    @@ -98,8 +146,15 @@ public function instanceSettingsForm(array $form, array &$form_state) {
       public function preSave() {
         // Trim any spaces around the URL and link text.
    -    $this->url = trim($this->url);
         $this->title = trim($this->title);
    +
    +    // Split out the link 'query' and 'fragment' parts.
    +    $parsed_url = UrlHelper::parse(trim($this->url));
    +    $this->url = $parsed_url['path'];
    +    $this->options = array(
    +      'query' => $parsed_url['query'],
    +      'fragment' => $parsed_url['fragment'],
    +    ) + $this->options;
    

    The first comment seems a bit stale, as the trim() on url is moved down?

    A bit confused that this doesn't seem to handle the route_name case, preSave() is also called then?

    If I understood correctly, then this field can either have a url or a route_name + parameter? Should possibly have a !empty() then around the logic here and might even want to keep the trim() at the top as a result of that?

  3. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -31,17 +37,45 @@ class LinkWidget extends WidgetBase {
    +      $element['url']['#description'] = $this->t('This can be an internal Drupal path such as %add-node or an external URL such as %drupal. Enter %front to link to the front page.', array('%front' => '<front>', '%add-node' => 'node/add', '%drupal' => 'http://drupal.org'));
    

    I think those place holders don't provide enough context to make it clear to a translator what they are. Also, "-" in a placeholder is rather uncommon.

    Maybe something like %internal_path, %external_path?

    That said, I just checked how the block path visibility setting does it, and it's very similar, you probably even copied it from there :)

    We check with Gabor, but it's not that important.

  4. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -111,31 +145,91 @@ public function settingsSummary() {
    +
    +    // Validate only if the field type supports external and/or internal URLs.
    +    if ($element['url']['#value'] !== '' && $url_type != LinkItem::LINK_EXTERNAL) {
    

    The comment here seems wrong, if it supports external and/or internal URL's then it would always be executed. general or internal? Based on my suggestion above about binary constants, this could be $url_type & LinkItem::LINK_INTERNAL. Although those are somewhat uncommon, it might be a bit more self-explaining given you understand it...

sun’s picture

I like the idea of using bit-masked flags for LinkItemInterface::INTERNAL and LinkItemInterface::EXTERNAL.

If we go with that, then we actually ought to be able to remove the ::GENERIC constant, because it can be composed out of the other two, and that is more natural to how bitwise flags are used in PHP core and elsewhere in Drupal.

The annotation is also able to use the constants; i.e., no need to put "2" in there. (Not sure whether the Annotation parser supports bitwise ORs though.)

Lastly, I just spotted a typo that I missed in previous reviews:

+++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldFormatter/LinkFormatter.php
@@ -156,41 +154,36 @@ public function viewElements(FieldItemListInterface $items) {
+      $url = new Url($item->route_name, (array) $item->route_paramaters, (array) $options);

Typo in ->route_paramaters

Berdir’s picture

Status: Needs work » Needs review
FileSize
34.44 KB
8.33 KB

Changed 1, 3, 4 and updated the comment for 2, not exactly sure how that works.

Also updated the typo in #52, that's a problem with our magic accessors, because they allow anything and don't throw any notices...

amateescu’s picture

Status: Needs review » Needs work

I have to say that I'm not a huge fan of bitmask flags for extremely simple use-cases like this, but I'm not really opposed to it either, so the latest patch looks fine to me. Just a minor thing:

+++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldWidget/LinkWidget.php
@@ -54,22 +54,21 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+    // If the field is configured to allows internal paths, it cannot use the

allow :)

YesCT’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
34.55 KB
3.98 KB

I'm pretty sure the allows/allow was not a core gate, so this probably should not have been moved to needs work for that.
I thought maybe there was something else that it needed work for.
@dawehner thought maybe it was the bit mask thing, but reading back through the comments, looks like there is (enough) agreement that that is fine.

Read through the patch though and found these nits.
And one concern about getPath().

  1. +++ b/core/lib/Drupal/Core/Url.php
    @@ -291,6 +291,15 @@ public function setOption($name, $value) {
    +   * Returns the path of the URL if it is external.
    +   *
    +   * @return string
    +   */
    +  public function getPath() {
    +    return $this->path;
    +  }
    

    I dont see the "if it is external".

    Is that meant to be a warning? Like:
    "Use this only if ..."

    see:

      /**
       * The external path.
       *
       * Only used if self::$external is TRUE.
       *
       * @var string
       */
      protected $path;
    

    Maybe it should throw an exception if it's not external?

    Hm. seems tim brought this up already in #47.3

    tim pointed out in irc that

      public function getInternalPath() {
        if ($this->isExternal()) {
          throw new \Exception('External URLs do not have internal representations.');
        }
        return $this->urlGenerator()->getPathFromRoute($this->getRouteName(), $this->getRouteParameters());
      }
    

    I'll investigate trying to do something similar next. But I'll post this stuff first.

  2. +++ b/core/lib/Drupal/Core/Url.php
    @@ -321,11 +330,19 @@ public function toString() {
    +    if ($this->external) {
    
    @@ -335,11 +352,19 @@ public function toArray() {
    +    if ($this->external) {
    

    I was going to say that this looked like it should be $this->isExternal(). Then I read back through the comments and this was already discussed (See @tim.plunkett in #47.1)

    Most were fixed in #49, but a couple missed. I did them.

    Also, the chunks of code in toArray() and toRenderArray() are identical... but they were identical before this issue anyway.

  3. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -111,31 +144,91 @@ public function settingsSummary() {
    +   * Form element validation handler; Validates the url property.
    

    should not be capital since it's not another sentence, also, I think it should reference the form.
    (https://drupal.org/node/1354#functions)

    validateTitle and validateUrl are not used anywhere. Oh, wait, maybe they are specified as a callback. Yeah as $element['#element_validate'][].

    I looked for examples for OO code, and they were all different.

    If there is an issue to decide on a way to document OO form validation methods, we should fix this in that issue.

    I'm leaving these alone.

YesCT’s picture

Status: Needs review » Needs work
diff --git a/core/lib/Drupal/Core/Url.php b/core/lib/Drupal/Core/Url.php
index 4747ed2..604d3d6 100644
--- a/core/lib/Drupal/Core/Url.php
+++ b/core/lib/Drupal/Core/Url.php
@@ -297,8 +297,13 @@ public function setOption($name, $value) {
    *
    * @return string
    *   The external path.
+   *
+   * @throws \Exception
    */
   public function getPath() {
+    if (!$this->isExternal()) {
+      throw new \Exception('Internal URLs do not have external paths.');
+    }
     return $this->path;
   }

Before that:
cd core;
./vendor/bin/phpunit --group Url --strict
OK (20 tests, 65 assertions)

after that:
same.

adding the exception did not make any tests fail.
means missing test coverage.

there is no getPath() called from any test.

might look to add something to ExternalUrlTest

[edit:}
only call to getInternalPath() in an assert is to make sure it returns null (for an external path). So that is probably missing test coverage also (that it returns something not null when it should). But that might not be the fault of this issue.

YesCT’s picture

Status: Needs work » Needs review
FileSize
35.72 KB
1.77 KB

added tests (not the "fix" throwing the exception)

and got:
$ ./vendor/bin/phpunit --group Url --strict
PHPUnit 3.7.21 by Sebastian Bergmann.

Configuration read from /Users/ctheys/foo/drupal/core/phpunit.xml.dist

................F.....

Time: 2 seconds, Memory: 47.50Mb

There was 1 failure:

1) Drupal\Tests\Core\UrlTest::testGetPathForInternalUrl
Failed asserting that exception of type "\Exception" is thrown.

FAILURES!
Tests: 22, Assertions: 68, Failures: 1.

With the fix:
OK (22 tests, 69 assertions)

Head (no patch at all) is:
OK (20 tests, 65 assertions)

I need feedback because I'm still new at phpunit tests and ran out of time to look @depends.

and

+++ b/core/tests/Drupal/Tests/Core/ExternalUrlTest.php
@@ -184,6 +184,17 @@ public function testGetInternalPath(Url $url) {
+    $this->assertNotNull($url->getPath());

oh, this was kinda cheating I think... :) since I'm just checking it wasn't null.

I wont be back at this for several hours, so anyone can take this on.

dawehner’s picture

FileSize
35.82 KB
1.12 KB

Regarding the named exceptions: I think \UnexpectedValueException could explain things semantically a bit better.

amateescu’s picture

Sorry, the 'needs work' from #54 happened because I had a very old browser session where the status field was set to NW.

Here's a review for the latest changes:

+++ b/core/lib/Drupal/Core/Url.php
@@ -297,8 +297,13 @@ public function setOption($name, $value) {
   public function getPath() {
+    if (!$this->isExternal()) {
+      throw new \Exception('Internal URLs do not have external paths.');
+    }

If we want to do this in this patch, we need to do it for getRouteName() and getRouteParameters() too. (throw an exception if the url is external)

effulgentsia’s picture

FileSize
35.82 KB

Reroll.

effulgentsia’s picture

This patch looks awesome! Just some minor nits. Also, the issue summary could use an update to just explain the plan chosen and any relevant details about implementation, plus screenshot(s) of any UI screens that changed. And, if we're doing D8->D8 change records, then this needs one, I think.

  1. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldType/LinkItem.php
    @@ -18,28 +20,35 @@
    - *   description = @Translation("Stores a URL string, optional varchar link text, and optional blob of attributes to assemble a link."),
    + *   description = @Translation("Stores a URL string, optional varchar link text, and optional blob of options to assemble a link."),
    

    We also store route name and params.

  2. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldType/LinkItem.php
    @@ -18,28 +20,35 @@
      *   instance_settings = {
    + *     "url_type" = \Drupal\link\LinkItemInterface::LINK_GENERIC,
    

    Would LINK_EXTERNAL make a better default to match HEAD?

  3. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldType/LinkItem.php
    @@ -18,28 +20,35 @@
    -    $properties['url'] = DataDefinition::create('uri')
    +    $properties['url'] = DataDefinition::create('string')
    

    Why?

  4. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldType/LinkItem.php
    @@ -62,8 +71,21 @@ public static function schema(FieldDefinitionInterface $field_definition) {
    -        'attributes' => array(
    -          'description' => 'Serialized array of attributes for the link.',
    +        'route_name' => array(
    +          'description' => 'The machine name of a defined Route this link represents.',
    +          'type' => 'varchar',
    +          'length' => 255,
    +          'not null' => FALSE,
    +        ),
    +        'route_parameters' => array(
    +          'description' => 'Serialized array of route parameters of the link.',
    +          'type' => 'blob',
    +          'size' => 'big',
    +          'not null' => FALSE,
    +          'serialize' => TRUE,
    +        ),
    +        'options' => array(
    +          'description' => 'Serialized array of options for the link.',
    

    We'll need a migration issue for this, so tagging as "needs migration" just to make sure that issue gets filed.

  5. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldType/LinkItem.php
    @@ -79,6 +101,17 @@ public static function schema(FieldDefinitionInterface $field_definition) {
    +      '#title' => t('Allowed url type'),
    +      '#default_value' => $this->getSetting('url_type'),
    +      '#options' => array(
    +        static::LINK_INTERNAL => t('Internal urls only'),
    +        static::LINK_EXTERNAL => t('External urls only'),
    +        static::LINK_GENERIC => t('Both internal and external urls'),
    

    I think we always capitalize "URL" in screen text, or has that changed at some point?

  6. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -111,31 +144,91 @@ public function settingsSummary() {
    +    // Validate only if the field type supports internal URLs.
    +    if ($element['url']['#value'] !== '' && $url_type & LinkItemInterface::LINK_INTERNAL) {
    

    Why check LINK_INTERNAL here? We already have that logic in the code that assigns this validation function. And if someone really wants to run this function on a external-only link (e.g., they want to use a #type='textfield' instead of #type='url' there for some strange reason), why not let them?

amateescu’s picture

This patch looks awesome!

Thanks :)

  1. Fixed.
  2. I'm not sure.. HEAD defaults to external because it doesn't have any other option, but I'd think generic links are the 80% use case. Maybe we should ask the usability people?
  3. Because 'node/1' is not a valid URI :)
  4. Where should we open this issue, in the core queue or in the IMP sandbox?
  5. Right, fixed.
  6. Agreed, fixed.

Also addressed my own review from #59 and I'll update the issue summary a bit later today.

Berdir’s picture

+++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldWidget/LinkWidget.php
@@ -178,7 +178,7 @@ public function validateUrl(&$element, &$form_state, $form) {
     $url_is_valid = TRUE;
 
     // Validate only if the field type supports internal URLs.
-    if ($element['url']['#value'] !== '' && $url_type & LinkItemInterface::LINK_INTERNAL) {
+    if ($element['url']['#value'] !== '') {
       try {

Does the comment here need an update?

Issue summary and screenshots still needed :)

amateescu’s picture

Assigned: amateescu » Unassigned
Issue summary: View changes
Issue tags: -Needs issue summary update, -Needs screenshots
FileSize
36.82 KB
739 bytes
28.11 KB

Rerolled, fixed the comment from #63, updated the issue summary and here's a screenshot with the new instance setting.

Berdir’s picture

Thanks, this looks good to me, 1+ to RTBC.

This is tagged as Needs change record, but I'm not sure what change we need to record exactly? This doesn't seem big enough to require a 8.x -> 8.x change record. Maybe update the Url change record a bit, but we apparently don't have one for that yet? At least I wasn't able to find one.

dawehner’s picture

This looks really great!

  1. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldFormatter/LinkSeparateFormatter.php
    @@ -58,19 +59,17 @@ public function viewElements(FieldItemListInterface $items) {
    +      $url_title = $url->toString();
    ...
    +        $url_title = truncate_utf8($url_title, $settings['trim_length'], FALSE, TRUE);
    

    The variable name is a bit odd, but it was like that before.

  2. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -111,31 +144,91 @@ public function settingsSummary() {
    +  public function validateUrl(&$element, &$form_state, $form) {
    

    Maybe this is a bad question but can't we extract this logic somewhere? Maybe to a constraint, not sure whether this makes sense at all, but ths logic here does not seems to be bound to a widget.

tim.plunkett’s picture

https://drupal.org/node/2189619 is a draft change notice from #2153891: Add a Url value object, no one ever reviewed it I guess.

amateescu’s picture

I agree that the validation logic should be extracted to a constraint. Unfortunately, due to some personal stuff I will be offline for about a week, so anyone can feel free to take over this issue :)

blueminds’s picture

I came across one problem here. If you use the link field via UI all is fine. But doing an update of an entity having this field programmatically will result in loss of the query data. That is due to the logic in the preSave() method, where it expects this->url to also have the query appended, which is not the case if you programmatically any other property but the url.

Basically the options value that is reset in the preSave() should be reset only in case the url value was intentionally reset. How we can tell this, not sure.

blueminds’s picture

Discussed this with Berdir and he suggested to move the logic from preSave() into the widget's massageFormValues(). Please see attached patch.

JayeshSolanki’s picture

Issue summary: View changes
FileSize
20.11 KB
JayeshSolanki’s picture

Issue summary: View changes
pwolanin’s picture

Assigned: Unassigned » pwolanin
Status: Needs review » Needs work

So, I think this needs 2 changes.

We shouldn't be forcing with widget code to do bitwise logic - that would better be exposed via helper methods to flag it as allowing internal or not.

Also, per #66/#68 let's move validation out of the widget form.

I'll work on these items.

Berdir’s picture

Note that for methods to work you either need to call $entity->field[0]->someMethod() or you need to provide a list class for it in the field type annotation.

pwolanin’s picture

In the patch code I seem them calling $this, like $this->isExternal()

pwolanin’s picture

Status: Needs work » Needs review
FileSize
12.34 KB
42.15 KB

This cleans up terminology to refer to link type, not url type. Make the render array method actually return a valida render array. Also fixes doxygen, makes exception types consistent, and removes an extra function call slowing down link rendering.

Berdir’s picture

+++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldWidget/LinkWidget.php
@@ -109,6 +108,22 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
 
   /**
+   * Is the LinkItem field definition configured to support links to routes?
+   */
+  protected function supportsInternalLinks() {
...
+
+  /**
+   * Is the LinkItem field definition configured to support external URLs?
+   */
+  protected function supportsExternalLinks() {

Not sure how exactly we need to document them, but never seen it done like this :)

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Url.php
    @@ -364,38 +370,47 @@ public function toArray() {
    +  public function toLinkRenderArray($link_title = '') {
    

    Really not sure whether we should do it, as url has not really something in common with links, in which case we should better put a static method on some other helper

  2. +++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -109,6 +108,22 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
       /**
    +   * Is the LinkItem field definition configured to support links to routes?
    +   */
    +  protected function supportsInternalLinks() {
    +    $link_type = $this->getFieldSetting('link_type');
    +    return (bool) ($link_type & LinkItemInterface::LINK_INTERNAL);
    +  }
    +
    +  /**
    +   * Is the LinkItem field definition configured to support external URLs?
    +   */
    

    We probably have to add an @return

pwolanin’s picture

Status: Needs work » Needs review
FileSize
5.31 KB
44.32 KB

discussed in person with dawehner.

Drupal\Core\Form\ConfirmFormInterface::getCancelRoute() is not documented as returning a Url object, so if we want to make that change, it should be a follow-up patch to change the interface and all implementations.

removed toLinkRenderArray()

fixed doxygen.

dawehner’s picture

Status: Needs review » Needs work

We should extend test coverage and maybe also fix the ones which fail: ConfirmFormHelperTest

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
45.53 KB

test fixes

pwolanin’s picture

FileSize
741 bytes
45.59 KB

better method name and comment in the test.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
3.52 KB
48.87 KB

We should remove the alias manager dependency from the link generator, since it's not needed.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Form/ConfirmFormHelper.php
@@ -9,7 +9,6 @@
-use Drupal\Core\Url;

@@ -34,34 +33,37 @@ class ConfirmFormHelper {
+    $link = array();
     $query = $request->query;
     // If a destination is specified, that serves as the cancel link.
     if ($query->has('destination')) {
       $options = UrlHelper::parse($query->get('destination'));
       $link = array(
+        '#type' => 'link',
+        '#title' => $form->getCancelText(),
         '#href' => $options['path'],
...
     elseif ($route = $form->getCancelRoute()) {
-      if (!($route instanceof Url)) {
-        if (empty($route['route_name'])) {
-          throw new \UnexpectedValueException(String::format('Missing route name in !class::getCancelRoute().', array('!class' => get_class($form))));
-        }
-        // Ensure there is something to pass as the params and options.
-        $route += array(
-          'route_parameters' => array(),
-          'options' => array(),
-        );
-        $route = new Url($route['route_name'], $route['route_parameters'], $route['options']);
+      if (empty($route['route_name'])) {
+        throw new \UnexpectedValueException(String::format('Missing route name in !class::getCancelRoute().', array('!class' => get_class($form))));
       }
-
-      $link = $route->toRenderArray();
+      $route += array(
+        'route_parameters' => array(),
+        'options' => array(),
+      );
+      // Ensure there is something to pass as the params and options.
+      $link = array(
+        '#type' => 'link',
+        '#title' => $form->getCancelText(),
+        '#route_name' => $route['route_name'],
+        '#route_parameters' => $route['route_parameters'],
+        '#options' => $route['options'],
+      );
     }
...
-    $link['#type'] = 'link';
-    $link['#title'] = $form->getCancelText();

Please don't change this?!
If the docs on getCancelRoute are wrong, we should fix then. That absolutely returns a Url object in places.

I haven't looked at any of the other changes since #70...

tim.plunkett’s picture

If we have no coverage for getCancelRoute returning a URL that's my fault, but it needs to be supported.
See #2215961: Entity::urlInfo() should return \Drupal\Core\Url for a patch that depends on this.

amateescu’s picture

I don't see the reason for all the changes since #70. Everyone said the patch is fine and it only needed the validation code to be moved to a constraint, but every update since then did anything but that..

pwolanin’s picture

@tim.plunkett - I can put it back, but then I think any patch that's actually adding Url objects as return values should convert them all and the interface - it should pretty mechanical.

Right, yes, constraint. Got distracted, obviously.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
51.16 KB

Here's a first attempt to move to using constraints.

The docs on constraints are... lacking. And there are some field API bugs like #2226723: Value changes from WidgetBase::massageFormValues() are discarded

Adds back basic support for Url in confirm form helper, though I still think it should be out until it's the sole value accepted.

Initially I implemented the constant as a method:

+  /**
+   * {@inheritdoc}
+   */
+  public function getConstraints() {
+    $constraint_manager = \Drupal::typedDataManager()->getValidationConstraintManager();
+    $constraints = parent::getConstraints();
+
+    $constraints[] = $constraint_manager->create('LinkType', array());
+
+    return $constraints;
+  }
 }

But this patch now has it declared via annotation.

tim.plunkett’s picture

Status: Needs review » Needs work
  1. +++ b/core/core.services.yml
    index 1b1fa30..0c1ff8b 100644
    --- a/core/lib/Drupal/Core/Form/ConfirmFormHelper.php
    

    I still don't understand why this file is changed at all. Other than code flow, what have you changed here?

  2. +++ b/core/lib/Drupal/Core/Url.php
    @@ -321,38 +354,38 @@ public function toString() {
    -  /**
    -   * Returns the route information for a render array.
    -   *
    -   * @return array
    -   *   An associative array suitable for a render array.
    -   */
    -  public function toRenderArray() {
    -    return array(
    -      '#route_name' => $this->getRouteName(),
    -      '#route_parameters' => $this->getRouteParameters(),
    -      '#options' => $this->getOptions(),
    -    );
    

    Where did this go?

pwolanin’s picture

@tim.plunkett. The toRenderArray() method didn't return an actual render array: it returned a fragment that couldn't be rendered by drupal_render().

Should the Url class know about link type render elements? In discussion with dawehner, it seemed clearer to move tis responsibility to link-related code. In an earler iteration I had it return a full link element, but I think removing it is the more sensible option.

Given that the toRenderArray() method is removed, ConfirmFormHelper needs to be adjusted.

tim.plunkett’s picture

Please revert both of those changes. I have no idea what you discussed, as you didn't post any of it here or open any other issues.

As I stated before, #2215961: Entity::urlInfo() should return \Drupal\Core\Url relies on both of those remaining as-is.

tim.plunkett’s picture

pwolanin’s picture

@tim.plunkett - regarding the render array method, it was returning an incomplete fragment, and really it isn't clear whether a Url should support this at all.

I initially suggested making it return a link element in a patch above, but it seemed more coherent to simply remove it after discussing with dawehner and now fago. We can put it back as a link element if it's really important for some reason, but returning a non-renderable fragment doesn't make sense.

Changing the Constraint and Validator to be in one class as a preview of the pattern change in #2226821: Combine Drupal Constraint and ConstraintValidation classes into one

I feel like there are some unresolved UX questions - like whether user's can change the field setting after it's initially set? What does that mean if e.g. you change is so existing data becomes invalid? In discussion with fago, we should do an EFQ to find out what data exists before allowing the user to change the setting.

pwolanin’s picture

Assigned: pwolanin » dawehner
Status: Needs work » Needs review
FileSize
8.01 KB
51.45 KB

had to add 'link_type' => LinkItemInterface::LINK_GENERIC, to the test code to avoid fails.

Apparently the default configuration is not getting picked up from the annotation. dawehner will try to figure out why.

This replaces the interface constant with a number, but it's still not working. Ideally better to use the constants.

dawehner’s picture

This should be it.

tim.plunkett’s picture

Status: Needs review » Needs work

Once again. Please move all of the ConfirmForm changes to a separate issue, and the toRenderArray removal to another issue as well.
None of those changes are in scope, and weren't in any patches before @pwolanin hijacked the issue in #76.

We need to discuss those changes, and that shouldn't hold up this issue.

sun’s picture

I didn't really study recent patches, but solely based on following recent comments via e-mail, I tend to agree with @tim.plunkett — This patch was RTBC already, and out of a sudden we started to revamp the originally proposed and working code + seemingly much more code.

Can we go back to the patch that was RTBC, give or take some later improvements, and move forward?

dawehner’s picture

Status: Needs work » Needs review
FileSize
48.3 KB
6.65 KB
  • Get rid of unrelated changes
  • Extend the test coverage of the URL class
tim.plunkett’s picture

Thanks for removing the ConfirmForm stuff. But we do need those isExternal parts of toArray and toRenderArray.

tim.plunkett’s picture

tim.plunkett’s picture

Status: Needs work » Needs review

Random failure.

jibran’s picture

  1. +++ b/core/modules/link/lib/Drupal/link/Plugin/Validation/Constraint/LinkTypeConstraint.php
    @@ -0,0 +1,91 @@
    + * Validation constraint for a link field receiving data allowed by its settings.
    

    Can we change it so that it'll be within in 80 char limit.

  2. +++ b/core/modules/link/lib/Drupal/link/Plugin/Validation/Constraint/LinkTypeConstraint.php
    @@ -0,0 +1,91 @@
    +  public function validate($value, Constraint $constraint) {
    +    if (isset($value)) {
    

    $value is not set to null so it should be set and we should typehint it to LinkItem in function definition.

  3. +++ b/core/modules/link/lib/Drupal/link/Plugin/Validation/Constraint/LinkTypeConstraint.php
    @@ -0,0 +1,91 @@
    +          // @todo This shouldn't be needed, but massageFormValues() may not run.
    

    more then 80 chars.

  4. +++ b/core/modules/link/lib/Drupal/link/Plugin/Validation/Constraint/LinkTypeConstraint.php
    @@ -0,0 +1,91 @@
    \ No newline at end of file
    

    :/

dawehner’s picture

Rebased.

$value is not set to null so it should be set and we should typehint it to LinkItem in function definition.

This would conflict with the parent interface.

dawehner’s picture

Some other small cleanup.

jibran’s picture

+++ b/core/modules/link/lib/Drupal/link/Plugin/Field/FieldWidget/LinkWidget.php
@@ -113,10 +113,11 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+   *   Return TRUE if the LinkItem field is configured to support links to

@@ -124,10 +125,11 @@ protected function supportsInternalLinks() {
+   *   Return TRUE if the LinkItem field is configured to support links to

Returns

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Nope, those are not the method docs, so they're not subject to those rules.

I barely touched this code, only undoing an overzealous change. So I'm going to RTBC this.

dawehner’s picture

Just changed it.

xjm’s picture

Usually we don't put any verb in the return description. :P So it's silly to worry about either way.

  • Commit bf4a582 on 8.x by Dries:
    Issue #2054011 by amateescu, pwolanin, dawehner, YesCT, tim.plunkett,...
Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x! Let's make progress on that beta blocker now. Thanks to the many contributors on this issue.

moshe weitzman’s picture

As a followup,

  1. template_preprocess_link_formatter_link_separate() looks really odd. It doesn't use the $url object that's passed to it.
  2. LinkFieldTest is a webtest. I am hoping we can remove these as we modernize each field type?
blueminds’s picture

jessebeach’s picture

Status: Fixed » Needs work

This was committed without a change notice :)

Here's the draft.

https://drupal.org/node/2231761

jessebeach’s picture

Title: Implement built-in support for internal URLs » Change record: Implement built-in support for internal URLs
jessebeach’s picture

'Needs change record' tag to 'Missing change record' tag.

dawehner’s picture

HA, that change notice could be written by some members of the VDC team directly :p

Note: https://drupal.org/node/2189619 has a lot of those information.

effulgentsia’s picture

What are the kinds of things we need for D8 to D8 change record? For example, do we need a change record for the link field's 'attributes' property being changed to 'options'? Or anything else that a contrib module implementing a formatter or widget for link fields might need to care about?

xjm’s picture

D8-D8 change records are only for major changes that significantly impact core and contrib developers, not every little API change. The more important (and time-consuming) task for issues like this is usually reviewing existing 7-8 change records and making sure they reflect/include the new change... the design of the change record system didn't quite account for the scope or length of this release cycle. ;)

amateescu’s picture

Title: Change record: Implement built-in support for internal URLs » Implement built-in support for internal URLs
Assigned: dawehner » Unassigned
Status: Needs work » Fixed
Issue tags: -Missing change record

Reseting to the proper status then.

xjm’s picture

https://drupal.org/node/2231761 is completely empty; is nothing needed at all?

effulgentsia’s picture

Correct. The Link module is new to core for D8. I added the "needs change record" tag in #61 in case we wanted a change record letting people know that it can now support internal paths, or that 'attributes' was changed to 'options', which would affect contrib modules that interact with link fields. But per #127, I don't know that either of that qualifies as major enough to warrant a 8->8 record.

xjm’s picture

Alright, I'll remove the draft. Thanks!

Status: Fixed » Closed (fixed)

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

Berdir’s picture

Issue tags: -sprint