Problem/Motivation

Linkit filter is stripping out the query string and the fragment from the URL.

Proposed resolution

Fix it.

Remaining tasks

None.

User interface changes

None.

API changes

New \Drupal\linkit\SubstitutionInterface::getUrl() signature:

  /**
   * Get the URL associated with a given entity.
   *
   * @param \Drupal\Core\Entity\EntityInterface $entity
   *   The entity to get a URL for.
   * @param array $options
   *   (optional) See UrlGeneratorInterface::generateFromRoute() for the
   *   available options. Defaults to [].
   *
   * @return \Drupal\Core\GeneratedUrl
   *   A url to replace.
   *
   * @see \Drupal\Core\Routing\UrlGeneratorInterface::generateFromRoute()
   */
  public function getUrl(EntityInterface $entity, $options = []);

Note that there's no BC break as the new parameter is optional.

Data model changes

None.

CommentFileSizeAuthor
#17 2895153-17.patch2.88 KBidebr
#17 2895153-17-test-only.patch1.15 KBidebr
#16 2895153-16.patch2.88 KBlendude
#16 interdiff-2895153-14-16.txt909 byteslendude
#14 2895153-14.patch1.91 KBlendude
#14 interdiff-2895153-12-14.txt1.5 KBlendude
#12 2895153-12.patch2.13 KBlendude
#12 2895153-12-TEST-ONLY.patch1.15 KBlendude
#11 diff-10-11.txt298 bytesbgilhome
#11 linkit_substitution_query_fragment-2895153-11.patch915 bytesbgilhome
#10 linkit_substitution_query_fragment-2895153-10.patch976 bytesJānis Bebrītis
#4 2895153-4.interdiff.txt3.48 KBclaudiu.cristea
#4 2895153-4.patch5.34 KBclaudiu.cristea
#2 2895153-2.patch5.58 KBclaudiu.cristea

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Active » Needs review
StatusFileSize
new 2895153-2.patch5.58 KB

Patch.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
StatusFileSize
new 2895153-4.patch5.34 KB
new 2895153-4.interdiff.txt3.48 KB

The previous patch incorrectly used Url::fromUri(). Fix this and also the Media plugin can reuse code from File, so I extended Media from File.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue tags: +Needs tests

It seems that I misunderstood what this filter is doing. This filter transforms markup like:

<a data-entity-type="node" data-entity-uuid="35890452-f266-429a-a29e-f8053c608522">Some text</a>

into:

<a data-entity-type="node" data-entity-uuid="35890452-f266-429a-a29e-f8053c608522" href="/node/123">Some text</a>

I would like to have this filter. But what about the case when the incoming markup has already set a href="..."? In my specific case I have initial markup that contains also tags like:

<a data-entity-type="node" data-entity-uuid="35890452-f266-429a-a29e-f8053c608522" href="/node/123?a=x&b=y#anchor">Some text</a>

This is recreating the href but is ignoring totally the existing href="..." and so I'm losing the query and the fragment. I see two options:

  1. Go with the idea from this patch and extract the query and fragment from incoming href="...", if exists.
  2. Add a new filter setting Do not recreate href="...", if already exists (or so), somehow, the same as we are doing with the title.
    Note that the new setting may refer only to preserve the query & fragment part of the incoming link.

I would like a maintainer input before improving the patch and writing tests.

johnpitcairn’s picture

johnpitcairn commented

I posted a similar issue a few months ago, I will mark that as a duplicate of this.

Option 2 will not generate aliased paths, which is not very desirable.

I would favor option 1. Extract any existing query parameters and fragment, then add those to the generated URL.

anon’s picture

anon
Primary language Swedish
Location Gothenburg
commented

I'm unable to save links with query and/or fragments through the GUI, how did you manage to do that? However, if I editor the source in the editor so the href have query and fragments I can see the problem.

For the first solution: where do we store the extracted info?

For the secound; If I understand it correct, that would generate alises, and then add the query and fragment parts from the original href. But do we need a new filter settings for this? Why not do it all the time?

anon’s picture

anon
Primary language Swedish
Location Gothenburg
commented
Status: Needs review » Needs work
johnpitcairn’s picture

johnpitcairn commented

I'm unable to save links with query and/or fragments through the GUI

Thanks. I'm not editing source, just appending them to the path that is generated after selecting a target in the dialog, before clicking "save" in the dialog. They are preserved in the editor and on display but the path does not get aliased. The query works, but the fragment only works if Redirect is not used to redirect to aliased paths.

The query and fragment are preserved in the saved content for me, so that presumably would allow the filter to handle it? But if they are not preserved for you, um...

For the secound; If I understand it correct, that would generate alises, and then add the query and fragment parts from the original href. But do we need a new filter settings for this? Why not do it all the time?

Indeed, why not? Presumably we could use Drupal's URL-parsing to split the path and reappend query and fragment.

This begs the question "what about extra arguments" like views args especially. If those can be parsed off and reappended after the alias is generated that would be pretty cool.

Jānis Bebrītis’s picture

Jānis Bebrītis commented
StatusFileSize
new linkit_substitution_query_fragment-2895153-10.patch976 bytes

Having the same issue, "/node/123?test=1#anchor" gets translated into "/somepath" instead of "/somepath?test=1#anchor".

I did not notice the existing patch, so I made a quick one for 5.0-beta5. Tried to change patch from #4 to work with both 5.0-beta5 and 5.0-beta6, but it did not work out, there are some rejects in it. Also, this did not fix my issue, so I'm posting mine.

bgilhome’s picture

bgilhome commented
StatusFileSize
new linkit_substitution_query_fragment-2895153-11.patch915 bytes
new diff-10-11.txt298 bytes

Thanks Jānis - the patch just needed a slight reroll from current 5.x.dev, attached.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 2895153-12-TEST-ONLY.patch1.15 KB
new 2895153-12.patch2.13 KB

Here is a test for this, test-only file is also the interdiff to #11

The last submitted patch, 12: 2895153-12-TEST-ONLY.patch, failed testing. View results

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new interdiff-2895153-12-14.txt1.5 KB
new 2895153-14.patch1.91 KB

In order to get a proper workflow when working within the dialog, the $href_dirty_check check should not fail when you only add a fragment, so changed this to only check the URL.

If we don't do this, you would need to select the right option from the autocomplete, then type in the fragment and then select the right option from the autocomplete again to get it to work.

Status: Needs review » Needs work

The last submitted patch, 14: 2895153-14.patch, failed testing. View results

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2895153-14-16.txt909 bytes
new 2895153-16.patch2.88 KB

Duh, left out half the patch

idebr’s picture

idebr commented
StatusFileSize
new 2895153-17-test-only.patch1.15 KB
new 2895153-17.patch2.88 KB

Posting a test-only patch to show the tests are working correctly. The full patch is unchanged.

The last submitted patch, 17: 2895153-17-test-only.patch, failed testing. View results

idebr’s picture

idebr commented
Status: Needs review » Reviewed & tested by the community

Thanks, the patch works as expected! Setting to RTBC.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

+1 RTBC, manually tested this on a site and it works a charm 💎

Thanks!

attisan’s picture

attisan
Location Vienna
commented

+1 works great

nortmas’s picture

nortmas commented

Works good, thank you!

jeremyr’s picture

jeremyr commented

+1 works for me too.

jeremyr’s picture

jeremyr commented

Is there anything else needed here? Is someone able to commit this?

  • anon committed c9705bd on 8.x-5.x authored by idebr 
    Issue #2895153 by Lendude, claudiu.cristea, idebr, bgilhome, Jānis...
anon’s picture

anon
Primary language Swedish
Location Gothenburg
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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