Cannot add relative internal link

This is as per designed.

May I ask why this is as designed? I upgraded from an earlier version of the module where relative URLs were allowed and this really makes the module more awkward to use. Perhaps this could go in as a feature request?

If this is by design, this design is very, VERY weird. This should not be a feature request - I assume this is a bug-report. I'll have a look at this later today.

This needs validation in the form to make sure invalid values are not allowed, as well as tests to also check this. But this approach should work for both external and internal links.

Attached patch uses the alias manager to find the alias of the node.

However, this added behavior should also increase the need to fix #2669028: No cache invalidation when settings change.. Because we now allow linking to a node (well, that was actually always allowed trough a full url). This means we should also try to add the cache tag of the node that's being linked to. That way the cache gets invalidated when the gets edited and the link doesn't go to a 404.

We'll be using this patch in production and if I find more things that need to be changed with this patch I'll update the issue. If I find the time I'll also write form validation and tests. But because the person configuring this is in our office I don't need that at the moment. So feel free to do that.

I just tested the above patch, it solved our problem.

Thanks for confirming that this works. But this 100% needs form validation +integration tests before commit.

Next steps to finishing this patch are:

1. Add form validation: - make sure that the link either starts with http:// or https:// or that you can create Url::fromUserInput without it thrown an exception. If it doesn't: add a form validation error.
2. Add an integration test that tests with a couple of correct values ('/node/2', '/user/1', 'http://example.com')
3. In that integration test, also test with incorrect values and check that the submission fails (example: 'node/1', 'ftp://example.com')

@everyone

Its already an HTML 5 "URL" type field(check with source code or firebug tool) which asks user to enter valid URL and apart form this it is also mentioned below the field in the help text
that link should start with http:// or https://.

So it already accepts internal/external URLs.

PFA for the same.

Its already an HTML 5 "URL" type field(check with source code or firebug tool) which asks user to enter valid URL and apart form this it is also mentioned below the field in the help text
that link should start with http:// or https://.

So it already accepts internal/external URLs.

I don't think you understand what an internal url is. A http://example.com/node/1234 style link is not internal not even when you're developing example.com.

That's why the attached patch changes the type of input to a normal input, that way you can use <front>, /node/10 and links like that also work.

I think to check whether the link is internal or external
rather than

if (strpos($link, 'http') === 0) {
        $popup_link = $link;
      }

we should use UrlHelper::isExternal($link)

UrlHelper.

This change was introduced in #2613768: Invalid URI in privacy policy link causes WSOD, so any solution should take the problem/bug outlined there into account.

Regarding tests: I wrote extensive tests for the popup link field, including testing external and internal urls and handling of aliases. The tests were part of #2662090: Privacy Policy link no longer accepts external urls, and were green at the time they were written. Unfortunately, it seems that when that issue got committed, the newly added tests was not included in the commit. The good news is that we can probably re-use those tests with a couple of tweaks and use them here.

Honestly, I think it might be best to revert #2613768: Invalid URI in privacy policy link causes WSOD, add the existing test from #2662090: Privacy Policy link no longer accepts external urls via this issue (won't need much tweaks if that issue gets reverted), and re-open #2613768: Invalid URI in privacy policy link causes WSOD to fix it while still allowing internal/relative URLs (and special cases such as <front>)

The approach discussed in #13 looks great. @mr.baileys and I can talk about this together here at ironcamp if that's useful?

First stab, with extensive testing. Both external and internal urls are allowed, aliases are used when rendering an internal path for which an alias is registered, en special case URL '<front>' also works.

This patch also contains the fix for caching described in #2669028: No cache invalidation when settings change., since this is required to make the tests succeed (otherwise we'd have to flush caches after each save).

Rerolled.

Overall this looks good; however I found some nits to pick.

1. +++ b/eu_cookie_compliance.module
   @@ -60,88 +61,90 @@ function eu_cookie_compliance_page_attachments(&$attachments) {
   +      'popup_link'           => $config->get('popup_link'),
   +      'popup_link_new_window' => $config->get('popup_link_new_window'),
   +      'popup_link'           => $popup_link,
   +      'popup_link_new_window' => !empty($config->get('popup_link_new_window')) ? $config->get('popup_link_new_window') : 1,
   

   This is double - but it was like that in the old version as well. But let's fix this while we're at it.

2. +++ b/eu_cookie_compliance.module
   @@ -182,3 +185,37 @@ function eu_cookie_compliance_validate_hex($element, FormStateInterface &$form_s
   +function eu_cookie_compliance_validate_url($element, FormStateInterface &$form_state) {
   
   +++ b/src/Form/EuCookieComplianceConfigForm.php
   @@ -180,6 +180,7 @@ class EuCookieComplianceConfigForm extends ConfigFormBase {
   +      '#element_validate' => array('eu_cookie_compliance_validate_url'),
   

   This is a function in the .module, but we should probably move it to a public function in the Form to make sure that that code lives closer to the actual form?

3. +++ b/eu_cookie_compliance.module
   @@ -182,3 +185,37 @@ function eu_cookie_compliance_validate_hex($element, FormStateInterface &$form_s
   +  if (!empty($element['#value'])) {
   

   Let's reduce the nesting there. At least by doing: if (empty($element['#value']) { return; }

4. +++ b/eu_cookie_compliance.module
   @@ -182,3 +185,37 @@ function eu_cookie_compliance_validate_hex($element, FormStateInterface &$form_s
   +    else {
   +      // Internal URL.
   

   This comment doesn't add a lot of value. Let's just remove that.

5. +++ b/src/Tests/EuCookieComplianceConfigFormTest.php
   @@ -32,14 +36,15 @@ class EuCookieComplianceConfigFormTest extends EuCookieComplianceTestBase {
   +    $this->drupalCreateContentType(array('type' => 'page', 'name' => 'Basic page'));
   

   /s/array/[/

6. +++ b/src/Tests/EuCookieComplianceConfigFormTest.php
   @@ -53,4 +58,67 @@ class EuCookieComplianceConfigFormTest extends EuCookieComplianceTestBase {
   +    $scenarios = [
   

   let's make this more specific by renaming this to $invalid_scenarios? Not sure though, you can choose to ignore this.

Thanks @borission_

Great work! Let's get this in.

Let's insert it then directly.

Let's wait for a few extra patchs for a new beta ? You can use hash in composer to use it anyway.

As usual I forgot to change the status.

This broke our site as we had entered "internal:/..." as a path.. :)
Don't know if we can make an update hook here...

Why would you use, "internal:/" instead or "/node/id" or "/path/alias" or "http://mywebsite.com/my/path". I guess these URI format are only internal code to track the route. Not meant to be use like this.

We used it before to make it work (a nifty hack), with this update the site crashed.
So we had to disable some code to prevent the fatal, change the param and re-enable the code.

The patch did not work in my site (Drupal 8.2.3)

In my local works.

#20 doesn't work for me. Type of URL field in settings is url and do not validates any relative links or value.

@manuel.adan the patch #20 is already on dev. Try dev

@killua99 still in current -dev: type of popup_link config field is url, so browser doesn't allow enter values like "/legal". My current workaround for that is to implement hook_form_eu_cookie_compliance_config_form_alter():

  // Custom and simple fix for #2798829
  $form['eu_cookie_compliance']['popup_link']['#type'] = 'textfield';

Confirm this is not fixed in dev. However the error is only apparent in some Browsers.

The HTML for the popup_link field is marked as type="url". The spec indicates this should be an absolute URL, so it's wrong for this field.

Latest Firefox by default enforces that. If the field contains a relative URL, Firefox displays a message "Please enter a URL" and does not submit the form (Drupal validate is never called).

The change in #33 looks good to me. #type=url should only be used for an absolute URL, and textfield seems like the best option to allow relative URL.

Drupal.org won't let me re-open the issue.

The issue then looks like to be on the Drupal Core side.

This error happen as well for Menu links or Site home page?

Site home page works fine because it has code:

    $form['front_page']['site_frontpage'] = array(
      '#type' => '<strong>textfield</strong>',
      '#title' => t('Default front page'),

This module needs to do the same. Only set type URL if it is an absolute URL.

Seems ridiculous but, someone can provide a patch? even I know is a minor change.

It will help me to committed asap.

Please also open a new issue. Thanks

I have raised a new issue #2852407: Browser prevents adding relative internal link

Hi, all - #20 plus changing the type of the field works for us - any idea when there will be a new stable release?

Thanks!

@LNakamura - By end of September

@svenryen - cool, thanks!

#20 works fine with multilingual sites
Thanks

#20 works but you can't use the settings form because the link field has the url type. Mentioned before.