Problem/Motivation

It looks like #2367069: Link 1.3 removes the query string from field tokens has broken the handling of relative / language prefixed urls.
Reason for this is that the commit introduced in _link_sanitize() a call to url() which fills $item['url']. Later the content of $item['url'] is used as url parameter in other calls to url() so some stuff is duplicated and or scrambled (urlencoded).

Proposed resolution

The goal of #2367069: Link 1.3 removes the query string from field tokens was to have proper entity tokens, I'd say let's solve this using the entity token api.
Attached patch declares a special getter which ensures a fully processed url is returned.
I've written a new test that checks the language prefix and / or absolute / relative representations of the stored url. Locally all tests pass now.

Beware: if you use a php version with curl 7.35 you might run into random test failures with the curl error "Problem (2) in the Chunked-Encoded data" - updating to a newer php/curl version fixes this. Just mentioning because I lost hoooouuuurs due totally random test failures...

Remaining tasks

Reviews needed.

User interface changes

None.

API changes

None.

Comments

The last submitted patch, tests-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, link-broken-relative-urls-with-language-prefix.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
8.18 KB
FAILED: [[SimpleTest]]: [MySQL] 1,414 pass(es), 8 fail(s), and 0 exception(s). View
9.68 KB
PASSED: [[SimpleTest]]: [MySQL] 1,422 pass(es). View

Okay got it, not all systems have clean urls enabled ;) Let's try this.
So, we should see a fail - but just for the test-only patch.

The last submitted patch, 3: tests-only-2428185-1.patch, failed testing.

das-peter’s picture

Priority: Major » Critical
FileSize
10.51 KB
PASSED: [[SimpleTest]]: [MySQL] 1,422 pass(es). View
955 bytes

I just came across another serious issue with _link_sanitize(). It seems a perfectly valid case that the function is called multiple times for the same item. However, as we change the item data by reference this means that processed data are processed again, which can lead to very unexpected results (missing query and all sorts of trouble).
To avoid such nasty surprises I suggest we store the original data and ensure the function always acts on those.
Updated patch attached.

TheodorosPloumis’s picture

jcfiala’s picture

Status: Needs review » Reviewed & tested by the community

Okay, since TheodorosPloumis says it worked for him too, I'll say it's RTBC! Which means that I'll see this the next time I'm looking for patches to include for Link.

johnpicozzi’s picture

It appears this is happening in 7.X-1.4 as well... Does anyone have a version of the patch that will work on 1.4?

Da_Cloud’s picture

@johnpicozzi you can apply the above patch against the 7.x-1.4 branch as well.

I can also confirm that the upgrade from version 1.3 to version 1.4 breaks the urls on multilanguage sites. Which gets fixed by applying the above patch.

johnpicozzi’s picture

Maybe I am missing something here but I have the following in a link field.

sites/xxx.mysite.com.fr/files/some.pdf

On output of the link I am getting

xxx.mysite.com/fr/en/sites/xxx.mysite.com.fr/files/some.pdf

where my base url is xxx.mysite.com/fr - The en shouldn't be in the relative path as it's a language code. correct?

Thanks for the help!

das-peter’s picture

@johnpicozzi I would've said the language prefix hasn't to be in the base url in the first place. But I'm certainly not aware of all the scenarios.

mfernea’s picture

Status: Reviewed & tested by the community » Needs work

For us only the patch from #3 works. If we apply the one at #5 we get an error like: EntityMalformedException: Missing bundle property on entity of type paragraphs_item. in entity_extract_ids() (line 7880 of ../includes/common.inc).

We are using the Paragraphs module and the error occurs only when having a paragraph with a link field inside another paragraph.

das-peter’s picture

Status: Needs work » Needs review

EntityMalformedException: Missing bundle property on entity of type paragraphs_item. in entity_extract_ids() (line 7880 of ../includes/common.inc).
That error sounds strange for this kind of patch.
There's no additional usage of entity_extract_ids() and as far as I can tell the patch doesn't anything that would affect the original entity.
But the error posted means that the original paragraphs_item entity is broken.
I guess we need more reviews on that, anyone with the paragraphs module?

recrit’s picture

re-rolled the patch to only set a flag on the item processed. Setting the $item['_link_sanitize_raw'] = $item; can lead to memory leaks on multiple calls for the same item since the $item is passed by reference.
The updated patch returns if the item has been processed since there is no need to re-process the link item.

karlos007’s picture

I confirm this bug. I confirm that patch #14 works for me well.

thatguy’s picture

I'm not sure if my problem falls under this issue but it seems to be similar. In my multilingual site I have a problem when I am trying to create internal link to a node with different language using it's path. For example if I am in a node which language is finnish and type to the link field path "example-node", which would be a node in english, I would assume that it will create a link to a node which path alias is "example-node". Instead it will create the link with the current nodes language prefix "fi/example-node".
I understand this is propably more of a problem with the i18n and how it adds the prefix but I was able to create the link correctly when I manually set the language of link in the url() function which is used in the _link_sanitize function. So I'm wondering if it could be possible to get the language of the node where the internal link leads to and use it to form the link correctly. This is also propably the cause for the failing validation of multilingual links which atleast I have run into since the language of the linked node does not seem to be used for the drupal_lookup_path.

eelkeblok’s picture

The patch works for me. I had a small review issue; the test contains

global $base_url, $base_path;

but these globals are never used. Attached is an updated version of the patch with this line removed. If somebody could verify this is a valid change, RTBC has my vote.

skipyT’s picture

Status: Needs review » Needs work

Hi,
I don't think this patch works in all of the cases.
Having a property getter in some cases creates an invalid url. The first part to avoid multiple sanitisation looks good, but the property getter is not called every time, and this leads to links without language prefixes (url function is not called on raw value if property getter is not called.)

Why did you modify that part? I think we should delete that part and keep only the _sanitize property checking.

weseze’s picture

Here is a patch without the getter.
Works fine for us on relative URLs, without the patch we get double language prefixes.

skipyT’s picture

Status: Needs work » Reviewed & tested by the community

this one works great

Status: Reviewed & tested by the community » Needs work
harsha012’s picture

Status: Needs review » Needs work

The last submitted patch, 22: language-prefix-and-relative-links-broken-2428185-22.patch, failed testing.

The last submitted patch, 22: language-prefix-and-relative-links-broken-2428185-22.patch, failed testing.

The last submitted patch, 22: language-prefix-and-relative-links-broken-2428185-22.patch, failed testing.

harsha012’s picture

added patch

Status: Needs review » Needs work

The last submitted patch, 26: language-prefix-and-relative-links-broken-2428185-26.patch, failed testing.

harsha012’s picture

GoZ’s picture

#26 works fine

lamp5’s picture

Patch #14 works well for me.

hgoto’s picture

The Patch #22 and #26 seem to add extra coding standard fixes which are not necessary to address this issue. So I'd like to reroll the patch #19 with the added tests in the patch #17.

There will be a lot of failed tests but I'm not sure they're related to this change.

FYI, subjective function _link_sanitize() is going to be fixed in other issues and we'll need to update the patch after other patches are committed.

#2707887: Sanitize function changes url

Status: Needs review » Needs work
kingandy’s picture

Patch in #31 works for me on 7.x-1.4, thanks! (We were up to THREE language prefixes.)

rcodina’s picture

Patch in #31 works for me too! Before the patch I also had up to three language prefixes.

hgoto’s picture

idebr’s picture

Status: Needs work » Needs review

The automated tests are passing again. Setting status back to 'Needs review.