Add better support for linking to anchors

I've been working on a "views display" plugin set, but run into this as well. The problem is, that the Substitution plugin only recieves the entity that is stored in the link attributes, not the path or any other info. I see two ways to work with this:

1. Add some attributes of the <a> tag as argument to the Substitution plugin's getUrl method, especially href from which the Substitution can extract the fragment, query etc.
2. Provide all attributes (or the DomElement itself) to the Substitution, and move the entity handling into the Substitution plugin.
   That way, the default canonical plugin could load the entity, get the translation etc. and other substitutions that are not (only) based on entities still have a way to say "I know this, it's a link to XYZ!".

Personally, I'd much prefer option 2. This would also vastly extend what matchers/substitutions can do. Maybe the Suggestion plugins (which already set which substitution plugin should be used) can be used to also specify which attributes should be passed to the Substitution plugin, e.g. the keys used within jsonSerialize

I've attached a patch that adds the link's attributes for use by the Substitution plugin. Also, the EntityMatcher will now retain fragment and query strings. I've also attached a version that works with #2979652-2: Substitution broken for links to non-file based media.

Is the changed approach I suggested in 2. something that should be considered? If so, it should probably get its own issue.

Updated the patch for current dev. We should probably also add tests for this new behavior.

This functionality would be super great.

What if, instead of query/fragment info being entered into the URL field (with the URL itself), what if they each got their own fields...? I feel like it could be a better UX for content editors.

Re-rolled the patch for latest beta version.

works perfect with Linkit 8.x-5.0-beta9

as @ckaotik stated "We should probably also add tests for this new behavior."

Patch in #8 does not apply to 5.0.0-beta9 or 5.0.0-beta10, I'm not sure what version it will apply to.

The patch here worked for me initially https://www.drupal.org/project/anchor_link/issues/2958532. Would be nice for linkit and anchor link to coordinate a fix for this.

Big change with the patch is that it doesn't work with the linkit buttons but ties linkit into Drupal link.

Partial re-roll of patch #8 for 6.0.0-beta2
This only contains the autocomplete part for fragments.

Rerolled for current dev with additional test

Updated version of #16 that fixes a duplicated anchor for me.

Patch for the failing test.

Replaced $parse_url = parse_url($attributes['href']); with $parse_url = parse_url($element->getAttribute('href'));

@sleitner your latest patch is only half the size than the previous one, as if a lot of things were missing?

@jurgenhaas you are right, the new test was untracked.

Fixes some coding standard warnings

Looking good on 6.0.0-beta3

Also tested in 8.x-5.x-dev and seems working.

Reroll without changing anything

#22/27 designed for 8.x-5.x test fail because ckeditor does not find the toolbar settings

#26 works with 6.0.x

What about passing just query and fragment. This keeps the plugin interface much cleaner.

Patch does not apply anymore

Please review #36

patch does not apply

Rerolled patch #36, please review it.

Thanks!

Rerolled the patch for latest changes.

+1 - would be great to get this feature in a stable release :-)

I've taken the patch in #41 and put it into a merge request to demonstrate passing test coverage against the latest development in Linkit.

Data attributes are currently removed again when adding a URL fragment manually. It seems that this is caused by https://www.drupal.org/project/linkit/issues/3443845.

Analyzing it further, it turns out that the patch actually works, hence setting it back to reviewed & tested. My issue is caused by a custom NodeMatcher::buildPath(), which does not consider the URL options. Sorry for the confusion.

Patch at #41 works perfectly for me with Linkit 6.1.4 and Drupal 10.3

Thanks

The problem is that when you append the fragment to the path, LinkIt will no longer return the data-entity-type and data-entity-uuid attributes for the tag that it inserts. This means that any substitution plugins (like one that converts an internal node path to the proper path alias) will no longer work.

Testing this today with the 7.x branch, it appears that the data-entity-type and data-entity-uuid attributes are preserved just fine when an anchor/fragment is present:

Is there another scenario where this is still problematic?

Patch from #41 works and fixes the issue on 6.1.4. I also tested on 7.0.0-alpha1 version and there the issue still remains but on dev-7.x it is fixed and doesn't need patching.

Atleast for our projects, we'll continue using the 6.1.4 version with the patch until issue is fixed in a stable version.

Maybe I have a unique configuration, but the patch in #41 does not work for me.

Drupal 10.3.6
Linkit 6.1.4
CKEditor Anchor Link 3.0.0-beta1

When I go to save the link, either by selecting the matched node or tabbing out of the URL field, the anchor portion of the link (“#foo”) is stripped away.

Sorry, please disregard, Linkit wasn’t being patched properly in my local environment. Patch in #41 worked for me too.

I believe RTBC is the proper status for this…

Okay, thanks for the work and the information on which branch this is still needed for.

I also tested on 7.0.0-alpha1 version and there the issue still remains but on dev-7.x it is fixed and doesn't need patching.

I've released a new version of the 7.x branch, 7.0.0-alpha2.

Patch from #41 works and fixes the issue on 6.1.4.

I've merged this into the 6.1.x branch and released 6.1.5.

- Drupal 10.3.6
- Linkit 7.0.0-alpha2
- CKEditor Anchor Link 3.0.0-beta1

I have the same problem as #52: when I insert the anchor it is deleted.

This issue introduced a breaking change:

  public function getUrl(EntityInterface $entity, array $options = []);

As a result, media_entity_download for example has a fatal error again with this.

This was also proposed in another issue where I had already provided feedback about that.

IMHO this should be reverted, or a follow-up should be done to remove the argument from the interface.

Found the issue: #3041045: Allow "Linkit URL converter" filter to generate links based on the language of the current page, rather than the language of the referenced entity.

IMHO this should be reverted, or a follow-up should be done to remove the argument from the interface.

Re-opening this issue, with the plan to revert the change so that the 6.1.x branch does not introduce BC breaking changes...

Okay, release 6.1.6, published on 2024-10-17, reverts this change: https://www.drupal.org/project/linkit/releases/6.1.6

Setting this back to "Needs work" for an implementation that avoids BC-breaking changes.

I think you can only do this in the 6.1.x to provide a warning that a new param will be added in the next major:

public function getUrl(EntityInterface $entity /*, array $options = [] */);

We already updated to 6.1.5 :(

Created a new MR containing the suggested change of kim.pepper

I think you can only do this in the 6.1.x to provide a warning that a new param will be added in the next major:

Based on my testing in comment #50 and confirmation from another in #51, this is effectively resolved in the next major version. The intent in the recent merge here was to provide equivalent functionality for folks who still cannot upgrade to Linkit 7.x due to API changes introduced there.

Thanks @casey for providing the MR that does not include the second parameter to getUrl(). Folks who are interested in this change in 6.1.x, please review!

> this is effectively resolved in the next major version

Do we know *why* it was resolved there? 7.x doesn't have $options, so apparently there are other solutions that don't require this.

Even if it's fixed there, the test coverage should be ported to 7.x to verify that there.

This issue and patches here likely predate the change to return URL objects. Just like #3041045: Allow "Linkit URL converter" filter to generate links based on the language of the current page, rather than the language of the referenced entity, where I made the same arguments, why are we passing in $options, set it on the URL object that we then return again? Can't we just set it on the returned URL object (while keeping BC for 6.x where it might not be a URL object yet)

That other issue moved away from this approach too, didn't test it yet, but apparently that worked there too, with a very similar requirement.

The current MR produced a fatal error when using the LinkitFormatter:

ValueError: func_get_arg(): Argument #1 ($position) must be less than the number of the arguments passed to the currently executed function in func_get_arg() (line 24 of modules/contrib/linkit/src/Plugin/Linkit/Substitution/Canonical.php).

That's because the formatter does not pass $options, so I quickly added sanity checks to all occurrences of func_get_arg(1).

Apart from that, I agree with @berdir. I don't understand why we are passing the options to the substitution plugin that creates the URL, and still process the fragment and query parameters afterwards. I also think this processing code has a bug (left a comment on the MR).

Assigning myself to review questions raised in #70 & #72...

Running into an issue on version 7.0.4 with Drupal 11.1.7 where anchor links don't quite work as I expected. As described in #52, I am able to:

1. Add a node URL to the link tool, e.g. node/123, and click the found node.
2. Add the desired anchor, e.g. #test to the URL.
3. Click the green OK sign -> Everything works..

I am also able to:

1. Add a node URL with an anochor, e.g. node/123#test
2. Click the green OK sign twice -> Everything works.

I am however NOT able to:

1. Add a node URL with an anochor, e.g. node/123#test
2. Click the first suggested node -> Anchor disappears.

The last option is the one I'm wishing would work, but currently it does not.

I can not get anchor links to work properly in standard installation of Drupal 10.5.2 with only linkit 7.0.8 installed additionally:

When adding the url fragment linkit removes the data attributes. It actually is caused by https://www.drupal.org/project/linkit/issues/3443845. Commenting out those lines fixes the issue:

linkFormView.urlInputView.fieldView.element.addEventListener('input', function (evt) {
     //linkit.set('entityType', null);
     //linkit.set('entityUuid', null);
     //linkit.set('entitySubstitution', null);
});

Changing a link to a node to an external url as described in that issue still works correctly, if one clicks on the "No content suggestion found" autocomplete dropdown before clicking on Update.

This patch fixed it for me in 7.0.8.

When adding the url fragment linkit removes the data attributes. It actually is caused by https://www.drupal.org/project/linkit/issues/3443845. Commenting out those lines fixes the issue:

Agreed that this was a consequence of that change. I've updated the MR to clear out external URLs' data attributes later on in the execution, making it compatible with the logic for updating link anchors here.

The latest changes in the MR are compatible with Linkit 7.1.x.

From #74:

I am however NOT able to:

Add a node URL with an anochor, e.g. node/123#test
Click the first suggested node -> Anchor disappears.

With the latest changes, I am able to perform the steps above and have the anchor persist. Please confirm.

This is ready for further review!

Left a few comments for MR!74, but the code works including changing to an external url. Above all, clicking twice to insert the anchor is not necessary, anymore.

It doesn't look like #70 was considered so far? This targets 7.x now and adding a parameter to an exist method on an interface is still a breaking change.

There are means to do in a BC way now with commented properties and stuff, but I'm still fairly certain that this is not required to pass $options through because we can apply options to the returned URL object?

Also seems to need a rebase.

Re-rolled #76 against 7.0.15.