Hello,

I have noticed that when you set a link field's target to "Allow the user to choose", when creating links on a node, no matter what option you chose (to either open in a new window or the same window), it always opens the link in a new window. I don't know if it makes a difference or not, but we have the limit of the number of links a user can add set to "Unlimited".

Please help!

Trevor

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

trevorsheridan’s picture

Priority: Normal » Major

Anyone? We really need to get this resolved. It seems so basic, but yet it doesn't work.

brad.bulger’s picture

if i set my field to "Allow user to choose", and they do not choose "Open URL in a New Window", the target ends up as, literally, "user". that opens in a new window/tab - at least, it does for me in Firefox and Safari. you might check to see if you're getting the same behavior.

this seems to be some artifact of the way that i'm displaying the information from the field, not the normal Link field behavior - probably a theme function or the like.

trevorsheridan’s picture

I'm not seeing this behavior myself. What happens for me is, if they leave "Open URL in a New Window" unchecked, it still opens the window in a new window. It seems that this field has no affect, it always opens the link in an new window.

brad.bulger’s picture

if you look at the original HTML source of your page, what are the target attributes of the two kinds of links, checked and unchecked? (by original HTML source, i mean vs using a DOM inspector like in Safari or Firebug or such - the pre-Javascript HTML created by Drupal)

if they both have target="_blank" then something else is adding it - a theme function or another module.
if they don't both have target="_blank" then some kind of Javascript alteration is at play.

quicksketch’s picture

Status: Active » Needs review
FileSize
635 bytes

This bug does seem to exist at the very least that the default value for the "Open URL in new window" checkbox is not properly set. Instead of checking itself if it has a value of LINK_TARGET_NEW_WINDOW, it's currently checking its value no matter what the value is. When you create new content, the default of the checkbox is intended to be unchecked (or so it would seem), but instead it's always checked.

This patch corrects this problem.

Note that #949612: Link Target default when set to "Open URL in a New Window" would also correct this same problem, but through a feature enhancement, not a bug fix.

nicholasThompson’s picture

We are experiencing this bug when the field is embedded in a CCK Block (http://drupal.org/project/cck_blocks).

I've been doing some debugging and the issue appears to be to do with the $item that is passed into _link_sanitize. For the normal Node field, the attributs array has 'target'=>0 (zero). However for the CCK block it is just an empty array....

nicholasThompson’s picture

Our fix is from:

if (empty($item['attributes']) || $item['attributes']['target'] == LINK_TARGET_DEFAULT) {

to

if (empty($item['attributes']) || $item['attributes']['target'] == LINK_TARGET_DEFAULT || $item['attributes']['target'] == LINK_TARGET_USER) {
brad.bulger’s picture

Version: 6.x-2.9 » 6.x-2.x-dev
FileSize
1011 bytes

that worked for me. though, that first empty() test (that was already there) seems odd - if [attributes] is empty, there's no need to unset [attributes][target]. not checking the New URL checkbox sets the value of target to zero, so i changed it to test for that, which i think might have been the intent:

if (empty($item['attributes']['target']) || $item['attributes']['target'] == LINK_TARGET_DEFAULT || $item['attributes']['target'] == LINK_TARGET_USER) {

the patch in #5 worked too, that will save me grief from people unintentionally setting that. but i had to change the test to a strict equals (===) because zero is loosely equal to any string:

'#default_value' => ($attributes['target'] === LINK_TARGET_NEW_WINDOW),

those changes work fine in 2.9 - i moved to dev because they apply to different files there.

wrd’s picture

I'm having the opposite problem - users can select "open in new window," but the target attribute is not added to the code, so the link always opens in the current window.

jcfiala’s picture

Status: Needs review » Needs work

Using the latest code, I'm really not able to reproduce this behavior. it seems like the link target all works fine.

Anyone able to come up with a simpletest that shows that this doesn't work? Or otherwise can guide me to setting up something that shows that this fails?

brad.bulger’s picture

FileSize
2.57 KB

i have not written any simpletests before so i can't help there. i just did a test on a vanilla installation using the latest dev release and i see both problems. the new window problem is most easily shown using the CCK Blocks module.

1) install dev versions of link and cck_blocks
2) create a content type with a link field, with unlimited number of values (easier to see things) and "Link Target" set to "Allow user to choose". check "Enabled" under "Provide block for this field". (cck export of a sample type is attached)
3) go to the blocks admin page and set the CCK Block for your link field to display somewhere - i added it to the Content region for simplicity
4) create a node of your new content type. you will see that it is defaulting to "Open URL in a New Window" being checked. add two links, one with "New Window" checked and one unchecked.
5) when you look at the node, you'll see that the fields work correctly when displayed by the node itself. but in the CCK Blocks field block, the link that had "New Window" unchecked is rendered with target="user"

if you apply the patch to the link module, the field will now default to "New Window" unchecked, and links that have that unchecked display with no target attribute.

trevorsheridan’s picture

This patch (#8) worked perfectly for me! Nice work!

sarhansg’s picture

Version: 6.x-2.x-dev » 6.x-2.9

I have a similar problem with this module. However, instead of opening in a new window it opens the link in the current window when the checkbox is ticked. Would the patches help? Also how do I apply these patches?

dqd’s picture

Priority: Major » Normal
Status: Needs work » Fixed

fixed in latest dev. please report back for any leftovers.

Status: Fixed » Closed (fixed)

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

mrconnerton’s picture

I ran into this bug today as well using the latest dev version. Non of the patches that are not in dev worked for me but I hunted the problem down to link.module line ~367 where $attributes is being assigned:

$attributes = isset($element['#value']['attributes']) ? $element['#value']['attributes'] : $field['attributes'];

The problem was $attributes was still serialized thus breaking setting the #default_value of the field. To fix I just changed the line to:

$attributes = isset($element['#value']['attributes']) ? unserialize($element['#value']['attributes']) : $field['attributes'];

Not sure how mine is different than other peoples.

ParisLiakos’s picture

Version: 6.x-2.9 » 6.x-2.10
Status: Closed (fixed) » Needs work
braham.p’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
1.72 KB

Hi All,

I also faced the above issue. Please try this patch.

tim.plunkett’s picture

Version: 6.x-2.10 » 7.x-1.x-dev

This is still an active bug in 7.x-1.x

tim.plunkett’s picture

FileSize
2.05 KB

This, in conjunction with #2117099: Expose attributes as property info, fixes it for me.

0 == 'user' is TRUE
0 === 'user' is FALSE.

tim.plunkett’s picture

FileSize
1.73 KB

Leaving out that last hunk, not as sure about that part.

redndahead’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me. Especially when thinking about #2117099: Expose attributes as property info

jcfiala’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

I continue to not be able to reproduce this problem. I'm not going to fold this patch in, until someone can demonstrate to me how this is broken.

tim.plunkett’s picture

0 == 'user' is TRUE
0 === 'user' is FALSE.

That should have been enough...

redndahead’s picture

I understand the difficulty if you can't reproduce the issue. We are able to reproduce this issue on our installation. If nothing else the patch that is suggested should be an improvement as it makes sure that it truly is equals and not just a 0 to string comparison. Do you see any problems arising to making sure the types are the same?

redndahead’s picture

Status: Postponed (maintainer needs more info) » Reviewed & tested by the community
dqd’s picture

Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

I can't reproduce it too. And to repeatly point out that these lines

0 == 'user' is TRUE
0 === 'user' is FALSE.

would be enough to RTBC it, is a little bit ... isn't it? ;-) Not meant offending.

Well. There are scenarios where it makes sense to choose == over ===. And you forgot to mark the function which is asking. Which is IMHO more important than the exchanging of all == to === or != to !==. E.g. There is a big difference between strpos() or !empty() for example.

As fas as I know, such "made" problems with === are trending but should only occur in PHP with functions treating a string in Position 0 as false. That's e.g., the reason why to prefer !empty over strpos(!==). But I wonder how this could make any difference since !empty() or empty() should actually be enough here in this cases?

For your reference:
https://www.google.de/search?q=php%20check%20if%20string%20is%20empty%20or%20null
http://www.programmerinterview.com/index.php/php-questions/difference-between-and-in-php/

My try to reproduce:

  1. I have installed latest Drupal 7.28 on latest Ubuntu with standart LAMP stack
  2. I installed some typical Drupal modules like views, panels, rules, token, pathauto, jquery_update, ctools, etc
  3. I installed link module.
  4. I changed the default content type "Article" in "manage fields" and added a link field. Named it testlink and set the link target to "let the user choose." and values to "unlimited".
  5. I have created a new article with a link and choose open link in new window. Link opens in new window.
  6. I edited the article and choose open in same window. Link opened in same window.
  7. I created another link (add another) and did the same with the same expected behaviour.
  8. I added another link field but not with unlimited values. The same.

Sorry folks but repeating the quoted lines over and over isn't enough. There is a reason why PHP supports BOTH checks (=== vs. ==). They have both their usecases. And you are not the first Drupal contributors humbling over the code of link module. Patching for mothers fun isn't the way to go. Please give more info about which modules and PHP versions are invoked and if there is any chance of that other modules like Display Suite or Views etc. change the markup of your rendered link fields... surprisingly ... In the meatime I will go down the road and will try to find out if there was a good reason to choose == over === over the last years here.

redndahead’s picture

That's fine you are the maintainer. It's obviously a problem with more than just me, but I use drush make so I can keep this patch going. I'm not totally sure why it's set to 0 maybe it's features or something else. That said your argument is that there is no reason it should be 0 and the value should be the string that it's in the drop down. If that's the case then there is no harm in making it ===. You are expecting it to be a string. You are fine matching that it's a string. By adding this patch you aren't hurting your code at all, but it's solving problems for multiple people.

dqd’s picture

@redndahead: I am not sure if my english is good enough for not getting misunderstood. It wasn't meant offending or refusing in any way. I partially agree with you in some of what you say and I tend to commit this. But I also have to agree with jcfiala about that there is still no complete issue report on this and I still can't reproduce it. Even if it "doesn't hurt" anybody to let this lil' change in, I am not sure if this issue is fixed then. I still wonder why we have here ...

<?php

/* if is empty - or if is not empty but is the string LINK_TARGET_DEFAULT ... then */
if (empty($item['attributes']) || (isset($item['attributes']['target']) && $item['attributes']['target'] === LINK_TARGET_DEFAULT)) {}

/* if is not empty - and if is not empty and is the string LINK_TARGET_DEFAULT ... then */ 
if (!empty($settings['attributes']['target']) && $settings['attributes']['target'] === LINK_TARGET_USER){}

?>

... but maybe I am too tired. 9 o'clock in the morning here already, still had no sleep this night...

Leksat’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
2.39 KB

Sorry, I did not checked all comments, but I think the bug I've met is the same as described here.

Basically, the "Open URL in a New Window" option is a checkbox, and when it is unchecked it returns integer 0. This value is not checked in the _link_process(), and as a result we have links having target="0".

Attached patch fixes this. BTW, the patch is made on the base of 7.x-1.3 version. Probably it needs a reroll.

chaloum’s picture

Just letting you know that I ran the patch but the function is still broken. Links that are outside the domain are always opened in a new window.

dqd’s picture

Priority: Major » Normal

Are you guys sure that it doesn't conflict with Drupal core text format setting for links or sth ? @chaloum: thanks for reporting back! This underlines that it was a good decision of @jcfiala to not let it in yet.

drupalninja99’s picture

Is there a reason why this didn't get ported to Drupal 8?

Status: Needs review » Needs work

The last submitted patch, 30: link-target_0-981404-30.patch, failed testing. View results

dqd’s picture

@drupalninja99: because Link is in Drupal core now and has nothing to do with this issue for 6 and 7 ? ...