As reported by rickvug in #896814: Lazy-load URLs into the database introduced a compatibility problem between flag_note and flag_page. Removing the following code fixes the problem:

/*
 * Override get_link_type() function so flag_page module can create a
 * content_id only when necessary.
 */
function get_link_type() {
  $link_types = flag_get_link_types();

  // Override the default flag_flag_link by changing the module of ther selected link type.
  if (isset($this->link_type) && isset($link_types[$this->link_type])) {
    $link_types[$this->link_type]['module'] = 'flag_page';
    return $link_types[$this->link_type];
  }
  else {
    $link_types['normal']['module'] = 'flag_page';
    return $link_types['normal'];    
  }
}

I think the best way to get round this issue will be to make the lazy-loading optional (and advisable on large sites). And check for the existence of the flag_note module and warn users.

CommentFileSizeAuthor
#7 1017484_flag_page_and_flag_note.patch1.62 KBalexpott
#5 flag_page-1017484.patch1.12 KBRavi.J

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Removing the get_link_type() function will stop the lazy creation of flag_pages which on large and busy sites could result in a heavy load on the database.

Ravi.J’s picture

Ravi.J commented

One way to deal with this is for flag_page to consume all menu paths that look like "flag/%/%" and provided by flag related modules.

$flag object has link_type attribute, and we can identify that link_type "flag_note_form" is being provided by flag_note module and once we have this information flag_page_flag_link can call hook_flag_link of flag_note module to create accurate "href" and "destination" attributes.

I am going to look into this a bit further and hopefully will be able to post a patch here.

Ravi.J’s picture

Ravi.J commented

One way to deal with this is for flag_page to consume all menu paths that look like "flag/%/%" and provided by flag related modules.

$flag object has link_type attribute, and we can identify that link_type "flag_note_form" is being provided by flag_note module and once we have this information flag_page_flag_link can call hook_flag_link of flag_note module to create accurate "href" and "destination" attributes.

I am going to look into this a bit further and hopefully will be able to post a patch here.

Ravi.J’s picture

Ravi.J commented

I wrote this tin piece of code that goes into flag_page_flag_link() function , which seems to resolved the issue.
Below is how the function looks after the change

function flag_page_flag_link($flag, $action, $content_id) {
  /***
   * Fix for flag_note and other flag modules compatibility
   */
  if (isset($flag->link_type)) {
     $link_types = flag_get_link_types();
     $link_object = $link_types[$flag->link_type];
     $flag_link = module_invoke($link_object['module'], 'flag_link',$flag, $action, $content_id);
     if ($flag_link) return $flag_link;
  }
  // When other modules donot provide custom menu callbacks
  $title = isset($_GET['title']) ? $_GET['title'] : drupal_get_title();
  $token = flag_get_token($content_id);
  
  if (drupal_is_front_page()) {
    $title = variable_get('site_name', '');
  }
  return array(
    'href' => 'flag/'. ($flag->link_type == 'confirm' ? 'confirm/' : '') ."$action/$flag->name/$content_id",
    'query' => drupal_get_destination() . '&token='. $token . '&title=' . rawurlencode($title),
  );  
}

I don't see any load issues as a result of this fix.

Ravi.J’s picture

Ravi.J commented
StatusFileSize
new flag_page-1017484.patch1.12 KB

Cleaned up code from above .. Patch attached

rickvug’s picture

rickvug commented
Status: Active » Needs review

Sub. Can anyone with solid knowledge of flag review the approach? This seems to work but I'd want to make sure there are no adverse effects.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 1017484_flag_page_and_flag_note.patch1.62 KB

Patch in #5 doesn't save the page's title to the flag_page_data_url table. This would mean that the title field provided in the flag_page views integration won't work.

Patch attached fixes this issue by adding the title to flag_note's url for the flag_note form.

Still testing for further issues...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Closed (fixed)

I've committed the code in #7 to 6.x-2.x-dev.

Will do a release once #1022818: Provide views integration with Flag note fields is fixed.