When I save links like https://cc.readytalk.com/cc/s/showReg?udc=tn9kykvvigxh they are being cut off at the ?. I have searched through lots of resources and am not fining anything on this.

Thanks,
Steve

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

skessler’s picture

Title: Link gets cut off » Link gets cut off when Workbench status is changed

Looking into this further the link is getting cut off when the status is changed using the Workbench module. We are not finding any other abnormal changes in the content when the Workbench status is changed.

stevector’s picture

The Workbench Moderation form results in a node_save() when changing a node from an unpublished state like "draft" or "needs review" to published. The $node object passed from node_save is one that just comes through hook_node_view() or some other means.

The loaded node might have a link field value that looks like this:

array (
  'und' => 
  array (
    0 => 
    array (
      'url' => 'http://google.com/',
      'title' => 'google',
      'attributes' => 
      array (
      ),
      'query' => 
      array (
        'test1' => 'test1',
        'test2' => 'test2',
      ),
      'display_url' => 'http://google.com/?test1=test1&test2=test2',
      'html' => true,
    ),
  ),
)

_link_santize() breaks up the simpler array so that the 'url' property no longer contains it's full string. When the node is sent to node_save(), link module does not reconstruct the broken up url property.

skessler’s picture

Status: Active » Closed (fixed)

This appears to be fixed with the newest dev release of Workbench Moderation.

stevector’s picture

Status: Closed (fixed) » Active

The changes I pushed to Workbench Moderation yesterday aren't related to the small moderation form. I'm still able to reproduce this problem with these steps.

- Enable moderation on the article content type
- Add field_link to the article content type.
- Create an article and set the url value of field_link to http://google.com/?test1=test1&test2=test2
- Save the article node to the state "Draft"
- Verify that the link points to http://google.com/?test1=test1&test2=test2
- Using the small form at the top of the node page, change the node to the "Needs Review" state
- Verify that the link still points to http://google.com/?test1=test1&test2=test2
- Using the small form at the top of the node page, change the node back to the "Draft" state
- Verify that the link still points to http://google.com/?test1=test1&test2=test2
- Using the small form at the top of the node page, change the node to the "Published" state
- Verify that the link now points to http://google.com

stevector’s picture

Title: Link gets cut off when Workbench status is changed » Link gets cut off when Workbench Moderation status is changed
bk_bigfish’s picture

Following the steps outlined in #4 I too lose the url parameters.

Zach’s picture

Status: Active » Fixed

There is a small hack to fix this.

Add this code:

  foreach($node->field_rel_links['und'] as &$link){
    $link['url'] = $link['display_url'];
  }

Replace field_rel_links with the field data name. Add multiple foreach if multiple field data.

Code goes into workbench_moderation/workbench_moderation.module into function workbench_moderation_moderate_form. Add this in beginning of code.

This bug happens due to url key having it's value stripped as mentioned before.

I havn't tested the latest workbench module so I will not look further into a more permanent fix for this at the moment.

Zach’s picture

Status: Fixed » Needs review

Was meant to mark as needs review, but missed it and can't change.

jcfiala’s picture

Status: Needs review » Active

That's helpful Zach, but there's two things here. (And I apologize if I say something werd, but I'm not familiar with workbench yet.)

1) No everyone will have named their link fields 'field_rel_links', so this isn't really a patch. (Well, and it's not a patch file.)

2) If this is something that needs to change in workbench code, then you need to open this ticket over at their issue queue and add the information about this there - otherwise, nothing will get changed because I certainly can't change workflow from over here. :)

I'm not closing it immediately, but it sounds like the issue should be closed over here and opened up over there, with a proper patch that reads from the ... (and I'm going to get this table name wrong because I'm tired) field_instance table and iterates over the link fields in appropriate entity bundle/types.

quicksketch’s picture

@stevector described the problem quite well. The problem appears to be that Link module is modifying it's own contents inside of the $node object when the node is being viewed. Workbench Moderation passes in this fully loaded/prepared node into node_save(), which causes the link value to loose its query string.

This can be reproduced much more easily without Workbench Moderation with this simple PHP snippet:

$nid = 81; // Or whatever node you're testing on.
$node = node_load($nid);   // "http://google.com/?q=example" === $node->field_link[LANGUAGE_NONE][0]['url']
node_view($node);          // "http://google.com/" === $node->field_link[LANGUAGE_NONE][0]['url']
node_save($node);          // Query string is lost.

One would not expect node_view() to alter the $node object in such a way that the node could no longer be saved, but at the same time Workbench Moderation should probably not be passing in a modified $node to node_save() to begin with. So I'd say this is on the plate of both modules to solve, since both issues are probably bugs in each module that can cause unexpected behavior in other situations.

quicksketch’s picture

Title: Link gets cut off when Workbench Moderation status is changed » Link should not modify $item['url'] on node_view() (causes data loss when combined with Workbench Moderation)
FileSize
1.77 KB

It looks like this problem has been (inadvertently) solved by #1645640: Since <front> isn't implemented, take it off the project page until it is., which modified the $item['url'] value so that it's changed to a full version of the URL (after sanitization) rather than the cropped off version.

I'd argue that we shouldn't be changing the user-provided data *in any way* during the node_view() operation, since this can lead to the unexpected data manipulation described in #10. As-is, the current code would cause things like tokens to be accidentally saved back into the node in their rendered form. So the immediate problem of loosing the query string is fixed but a new problem remains.

This patch removes all modifications to the $item['url'] property entirely, leaving the user-entered values unchanged.

quicksketch’s picture

Status: Active » Needs review
balumahender’s picture

Hi ,

I am also getting same problem but not in local . It is happening in production where memcache and varnish cache is there

jcfiala’s picture

I'm a little worried that not changing $item is a problem as I think that's how the theme functions get the updated data. But I'll look at that soon.

jcfiala’s picture

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

Hmm.... no. I think you're wrong, quicksketch.

The code you're changing is in _link_sanitize(). _link_sanitize() is called from link_field_prepare_view(), which is an implementation of hook_field_prepare_view(). And that documentation clearly says "Make changes or additions to field values by altering the $items parameter by reference. There is no return value."

If this is a problem, the problem exists in Drupal Core, not in this module.

donquixote’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
458 bytes

I think #11 has correctly described the problem.
Unfortunately, the patch in #11 will break BC, because formatters may already expect the $item['url'] value to contain the absolute url.

As a band-aid fix, we could save $item['original_url'] in the url, to have the original url always available.

@jcfiala:
Yes, this is a flaw in core, that we have to deal with.
Viewing an entity may change its data.
I think the main idea is to add additional data to the $item array, but avoid to overwrite existing data.
E.g. setting the $item['display_url'] does not do any damage, because other code simply ignores this value.

donquixote’s picture

Btw, my personal use case is, I want to create a plugin for Crumbs, which uses a link field to determine the breadcrumb parent.
This absolutely needs the unmodified url.

donquixote’s picture

Version: 7.x-1.0-beta1 » 7.x-1.x-dev

The above patch in #16 won't do anything about workbench moderation.
But maybe it can be a start.

jrao’s picture

#16 doesn't seem to work, the original_url is not in the field loaded to node. The patch in #1814444: Also provide raw URL value in field array seems to work (needs to be re-rolled though).

das-peter’s picture

Issue summary: View changes

Just had a similar issue when using paragraphs and translation.
The url property was modified when translating content.
Very nasty bug.

I think the best approach is indeed the same as used in the text module in core - which creates the additional data entries safe_value / safe_summary in hook_field_load().
Simply put: Never modify properties that are stored in the DB - use new properties instead.

However, BC is an issue - especially considering the huge amount of time this has been around now.
I think we could introduce a switch (variable) that changes the behavior - so existing installations would run like before while new installations use the new approach.
The switch between the two approaches could be placed in the Drupal status page where the old approach is shown as warning - with the hint that changing the approach could break formatters and needs testing.

jbitdrop’s picture

Simply put: Never modify properties that are stored in the DB - use new properties instead.

Hm, I agree on we should solve this asap. Let me try to bring jcfiala and diqidoq (and renato) on the table if we can get rid off it any soon.

idebr’s picture

Coming from #1910946: Mark the anchor tag as having no title

Similar to URL there is currently no way to know the original title as well. An optional title is replaced with the display_url.

pifagor’s picture

Look good

  • pifagor committed 695ef7b on 7.x-1.x authored by idebr
    Issue #1475790 by idebr, quicksketch, donquixote, skessler, jcfiala,...
pifagor’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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