Problem/Motivation

D10 will remove jQuery dependency. Any progress to remove this from extlink Module?

Steps to reproduce

Proposed resolution

Replace jQuery code with JavaScript

Remaining tasks

Test/review and add it to a new version of the module

User interface changes

none

API changes

none

Data model changes

none

Issue fork extlink-3238995

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

cola created an issue. See original summary.

granik made their first commit to this issue’s fork.

granik’s picture

Version: 8.x-1.6 » 8.x-1.x-dev
Status: Active » Needs review
StatusFileSize
new13.97 KB

Created a merge request. The jquery code has been replaced with vanilla javascript. Checked on Firefox, Chrome and Safari, should work. But I would ask for review, it should be tested properly.

Also added a patch file to use it with composer.

grienauer’s picture

Issue summary: View changes
tyler36’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new3.98 KB

Tested on Drupal `10.0.3`

## Test

1. Added internal-only Link field to 'Article' node.
2. Added external-only Link field to 'Article' node.
3. Created new article with internal & external links.

## Other

https://www.drupal.org/project/extlink will also need updating to remove the `jQuery` reference from the first paragraph.

solideogloria’s picture

Issue tags: +Drupal 10
anybody’s picture

Status: Reviewed & tested by the community » Needs work

Thanks! Conflicts need to be resolved!

granik’s picture

Status: Needs work » Needs review

@Anybody, thanks. Just rebased, but would be ok if you test your new mailto feature again after my rebase.

anybody’s picture

Status: Needs review » Needs work
granik’s picture

Status: Needs work » Needs review
smustgrave’s picture

Version: 8.x-1.x-dev » 2.0.x-dev
smustgrave’s picture

Status: Needs review » Needs work

Would like to include with 2.0.x but appears to breaking a number of tests.

smustgrave’s picture

Woo got main pipeline all green so any change should be legit issues.

solideogloria’s picture

The rebase had lots of commits, because I merged once instead of rebasing prior to that.

solideogloria’s picture

I fixed almost all of the eslint issues. There are a few that I don't know if they should be ignored or changed.

smustgrave’s picture

Wouldn't worry about those, but the test failures could be showing an issue.

solideogloria’s picture

Both failing tests are because it's not finding the external settings file.

It has

  public function testExtlinkDisabledOnAdminRoutes(): void {
    $this->drupalLogin($this->adminUser);
    $this->drupalGet(self::EXTLINK_ADMIN_PATH);
    $this->assertSession()->checkboxNotChecked('extlink_exclude_admin_routes');
    $this->assertSession()->responseContains('/extlink/js/extlink.js');

    // Disable Extlink on admin routes.
    $this->drupalGet(self::EXTLINK_ADMIN_PATH);
    $this->submitForm(['extlink_exclude_admin_routes' => TRUE], 'Save configuration');
    $this->assertSession()->responseNotContains('/extlink/js/extlink.js');

Should it be this instead, with /extlink/settings.js?

  public function testExtlinkDisabledOnAdminRoutes(): void {
    $this->drupalLogin($this->adminUser);
    $this->drupalGet(self::EXTLINK_ADMIN_PATH);
    $this->assertSession()->checkboxNotChecked('extlink_exclude_admin_routes');
    $this->assertSession()->responseContains('/extlink/settings.js');

    // Disable Extlink on admin routes.
    $this->drupalGet(self::EXTLINK_ADMIN_PATH);
    $this->submitForm(['extlink_exclude_admin_routes' => TRUE], 'Save configuration');
    $this->assertSession()->responseNotContains('/extlink/settings.js');

See in extlink.module:

function extlink_library_info_alter(&$libraries, $extension): void {
  if (($extension === 'extlink') &&
      isset($libraries['drupal.extlink']) &&
      \Drupal::config('extlink.settings')->get('extlink_use_external_js_file')) {

    $host = \Drupal::request()->getBasePath();
    $new_key[$host . '/extlink/settings.js'] = $libraries['extlink.settings']['js']['/extlink/settings.js'];
    $libraries['extlink.settings']['js'] = $new_key;

    // Add the external settings JS file as a dependency to the drupal.extlink
    // library so that it will be loaded when configuration is set to use the
    // external file.
    $libraries['drupal.extlink']['dependencies'][] = 'extlink/extlink.settings';
  }
smustgrave’s picture

Ah that took me forever to figure out yesterday. So the test may need to be updated to check the host. Gitlab running with localhost/web was causing all tests to fail before.

solideogloria’s picture

I think this will fix the tests. The functionality with the external settings is actually working when I run it. It's just checking for the wrong string in the response.

solideogloria’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work

Reverted the changes to the test as don't think we should update those. If the jquery removal is correct then nothing should break. So think maybe the js changes around the nofollow/referrer needs some work.

smustgrave’s picture

Don't have the why yet but appears to be when the link has a target already set that the js fails to add a rel attribute

smustgrave’s picture

Status: Needs work » Needs review

So I moved the filter part of the js down after the rel is added. Wasn't needed before but tests appear green. Thoughts?

solideogloria’s picture

Status: Needs review » Needs work

It's better, but I don't think it's quite right. The externalLinks variable is filtered down, meaning that later when noreferrer is added, it won't be added to links that already had the target attribute set, even if the target is _blank. Is that the desired functionality? The form setting says this:

A link that specifies target='_self' will not be changed to target='_blank'.

My understanding of the "no override" setting was that it shouldn't override the target attribute, but that the nofollow and noreferrer should still be set on the link.

Personally, I think this

      // Apply the target attribute to all links.
      externalLinks = externalLinks.filter((link) => {
        // Filter out links with target set if option specified.
        return !(drupalSettings.data.extlink.extTargetNoOverride && link.matches('a[target]'));
      });

      // Add target attr to open link in a new tab.
      externalLinks.forEach((link, i) => {
        externalLinks[i].setAttribute('target', '_blank');
      });

Should be modified so that externalLinks is not changed. Something like this:

      // Add target attr to open link in a new tab if not set.
      externalLinks.forEach((link, i) => {
        if (!(drupalSettings.data.extlink.extTargetNoOverride && link.matches('a[target]'))) {
          externalLinks[i].setAttribute('target', '_blank');
        }
      });
smustgrave’s picture

Applied suggestion but have to run. If it works and we are happy I can merge this evening.

smustgrave’s picture

Status: Needs work » Needs review

Tests passed

solideogloria’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Do we need to add a test to cover that noreferrer is added to links with a target attribute already set?

  • smustgrave committed 3497129c on 2.0.x
    Issue #3238995 by solideogloria, smustgrave, granik, tyler36: Remove...
smustgrave’s picture

Status: Reviewed & tested by the community » Fixed

Think we are probably good.

Down to 2 issue so will plan a beta1 release in the next few days.

Status: Fixed » Closed (fixed)

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