Add an option to override disqus_identifier

Maybe abstract the disqus_identifier generation into some kind of hook?

/**
 * Generate a hook given the object and its type.
 */
hook_disqus_identifier($type, $object) {
  if ($type == 'user') {
    return 'user/' . $object->uid;
  }
}

This would return an array of possible identifiers. How would we determine which identifier to use? Maybe the last one in the array? Thanks!

Was considering something like http://drupal.org/project/uuid .

I'm very interested in this - I'm working on a project that shows the same content to different groups of users, and would allow them to use DISQUS to talk about it.

The issue being that the conversation gets very convoluted and confusing when it's not isolated to just that group.

I know it's been a few months, but is it something you're still considering? Maybe you have a patch kicking around?

I had needs similar to the OP, so I built on the approach in #4, but in the Drupal 6.x version of the module.

The attached patch and JS move the identifier to a db column, giving greater abstraction and flexibility with regards to Disqus threads and nodes.

hook_set_disqus_identifier() was implemented to give modules an opportunity to alter the identifier based on more sophisticated rules. A helper, or third-party module could make use of this to integrate with UUID if desired.

On the edit form itself, I parroted Pathauto, giving a simple "Auto identifier" option, with manual override possible. This is set to only appear for Disqus administrators.

(You'll need to rename disqus.identifier.txt to disqus.identifier.js to use)

This needs to go into d7 first.

Michael Shick! I like this functionality a lot. Having control over what the identifier is from the user space makes perfect sense. Let's port this up to Drupal 7 and get it in 6.

Attached is a port of #6 to Drupal 7.

Changes from #6, some of which I think would be nice to backport to D6:

1. Changes the hook name to hook_disqus_identifier_alter() and uses drupal_alter() to be more consistent with other alter hooks. (Also, since it's an alter hook, theme's can implement it though that's not an issue with D6).
2. Passes node/user object to the alter hook for context when changing the identifier.
3. Added API docs for the new hook.
4. Handles disabling the identifier field (a la pathauto) using #states. Also hides all Disqus settings when Disqus is disabled for a node. (D7-only)
5. Fixes problems with drupal_write not having the primary key.
6. Adds Disqus settings to vertical tabs summary. (Sorry, I realize that's outside the scope of this issue. I didn't realize I'd added that until I proof-read the patch. I can pull it out, if you'd prefer.) (D7-only, I believe...)

My thought with this patch was if we really need to override on per-node basis. I think that we'd cover at least 80% of use cases if we'd provide a way to set identifier pattern for individual node types. We could still fire an alter hook for those people that need more fine-grained control.

Thoughts?

#10: Unless you're arguing that these additions are adding unneeded complexity to the node edit form, I don't see a problem with allowing per-node IDs.

Is there something I'm missing? Thanks.

I suppose I should add that my use case surrounded importing content that already had a Disqus ID set that could not be replicated using a node-type pattern. I could handle that using the alter hook, but it seemed cleaner to have the Disqus ID as part of the node edit form.

Better error checking when programmatically importing nodes, such as during Migrate module imports.

OK, you are right. No real overhead here and looks like a nice feature. If someone needs pattern-based identifiers we support that via alter hook.

Patch looks a bit buggy, thought. If I configure disqus stuff on node edit form and save I see settings in DB. When I navigate back to node edit form I see default configuration. It looks like it is not loaded properly.

I also see no changed on vertical tab summary when I configure stuff.

I'm not seeing the settings-not-sticking issue -- if I set comments to Disqus and set it to automatic and save, it stays that way with the node/nid identifier showing the custom ID field when I re-edit. If I uncheck the auto-ID option and edit the custom ID, that's preserved when I re-edit the page.

(Note: if the auto-ID checkbox is unchecked, but the custom ID field is left blank, the code assumes this was a mistake and sets the auto-ID option, because we can't have a blank Disqus ID, as far as I know).

You're right, the vertical tab summaries aren't updating correctly, only on page load. I'll look into that.

Corrects vertical tab summary issues. Should now toggle between Open, Closed, and Disqus comments as you change settings.

I always forget that...

Summary now works as expected, while settings still does not load properly when I return to edit page. Are you sure you work on latest 7.x-1.x?

Looks like I was a bit behind head:

$ git pull
remote: Counting objects: 9, done.
remote: Compressing objects: 100% (5/5), done.
remote: Total 5 (delta 4), reused 0 (delta 0)
Unpacking objects: 100% (5/5), done.
From http://git.drupal.org/project/disqus
   851666d..bf74a73  7.x-1.x    -> origin/7.x-1.x
First, rewinding head to replay your work on top of it...
Fast-forwarded 7.x-1.x to bf74a730b30e01f8478e573377440c5aa2df81f9.

I've attached a patch against that latest 7.x-1.x head.

settings still does not load properly when I return to edit page

I'm still not able to repro this with new node or when editing existing nodes...

Did you run update.php (or drush updatedb) after applying the patch? There's a schema change included in the patch. Can you verify that an "identifier" column has been added to the disqus table? Thanks.

I'm still getting same bug. I tried to configure identifier on same not for two times. I always get default settings when I come back to the form and there are two rows in DB:

mysql> select * from disqus;
+-----+------+--------+-------------+
| did | nid  | status | identifier  |
+-----+------+--------+-------------+
|   7 | 3869 |      1 | adfasdfasdf |
|   8 | 3869 |      1 | asdfasdf    |
+-----+------+--------+-------------+
2 rows in set (0.00 sec)

I have run DB updates.

Thanks for checking the DB table... Unfortunately, it should be working based on what I see.

Let me try this on a completely new install later this evening and get back to you. Perhaps there's something leftover in my localhost that's obscuring the bug?

Sorry for the delay... Vacation got in the way! :)

I'm still unable to reproduce the settings not sticking issue. I tried it with a brand new install and it's working correctly. One last thing to check: is the Disqus shortname set? All the code that sets non-default settings takes place in a conditional that checks that disqus_domain is not empty. (Whether there should be a better warning or something is a fair point -- I was following the settings code that was there previously. Perhaps that can be raised in a separate issue?).

Thanks.

@@ -332,13 +361,12 @@ function disqus_node_insert($node) {
+  // If we're back to our default settings, delete the row from the table.
+  if ($node->disqus_status && $node->disqus_identifier_auto) {
+    disqus_node_delete($node);
+  }
+  else {
+    disqus_node_insert($node);
   }
 }

$node->disqus_status and $node->disqus_indentifier_auto will only be set if using edit form. This values will not exist if node will be loaded and then saved via API, which will result in notices and unexpected behaviour.

@@ -318,13 +331,29 @@ function disqus_node_delete($node) {
+  // Write the value only if it's disabled (default is enabled) or if there is a
+  // user-specified identifier.
+  if (isset($node->disqus_status) && isset($node->disqus_identifier_auto) && (!$node->disqus_status || !$node->disqus_identifier_auto)) {
     $data = array(

Same as above.

Sorry for the delay. I finally had a chance to get back to this issue.

I've updated the patch based on #23. The second code block already checks that $node->disqus_ members exist. I've added the check to the first code block as well. Also, the Views field handler was using $disqus['identifier'] for the path, I've changed that to use $disqus['url'] instead. Note that makes for absolute paths rather than relative paths, but that might be preferable when the field is used in an RSS feed, for example.

1. +++ b/disqus.module
   @@ -362,11 +362,13 @@ function disqus_node_insert($node) {
   -  if ($node->disqus_status && $node->disqus_identifier_auto) {
   -    disqus_node_delete($node);
   -  }
   -  else {
   -    disqus_node_insert($node);
   +  if (isset($node->disqus_status) && isset($node->disqus_identifier_auto)) {
   +    if ($node->disqus_status && $node->disqus_identifier_auto) {
   +      disqus_node_delete($node);
   +    }
   +    else {
   +      disqus_node_insert($node);
   +    }
      }
   

   That is not enough. Disqus data is stored in $node->disqus array at load. That means that any developers doing stuff from code will change their configuration there. This code will ignore that. We have to find a way around that.

2. +++ b/disqus.views.inc
   @@ -40,11 +40,12 @@ class views_handler_field_node_disqus_comment_count extends views_handler_field
   +      dsm($disqus);
          // Build a renderable array for the link.
   

   Don't include debug stuff.

Finally got some time to continue with this issue...

Don't include debug stuff.

Ack. Sorry about that. Will remove in the next patch.

Disqus data is stored in $node->disqus array at load. That means that any developers doing stuff from code will change their configuration there. This code will ignore that. We have to find a way around that.

I think I understand... Just to clarify, on a given node edit form, the user can make a change to $node->discuss_foo while a module can change $node->disqus['foo'] at the same time? Or is it only one or the other?

If a conflict occurs, would it be safe to assume that the user's edit on the node edit form would take priority? Or should we save a copy of the current settings in order to compare both $node->disqus_foo and $node->disqus['foo'] against? That seems like we'd be adding a lot of complexity for an edge case...

Thoughts?

I think that node edit form adds $node->discuss_foo automatically. I think it is safe to use this value if exists, but we should definitely also check $node->disqus['foo'] if not.

Coming into this issue on the front end of a D->D migration and looking at posts like #1993242: Migrating content from Drupal 6 to Drupal 7 upgraded site, I see this as a bit of a sticking point. I'm wondering if this is a good time to create a new branch of the module with a reliance on the UUID module.

By definition UUIDs are universal and they can serve the dual purpose of being the identifier that disqus requires. Whether this is a UUID on the node or whether disqus (and the disqus table) uses node UUIDs is flexible but I figure it is a more standard way of operating, even at the expense of adding another module. One thing to clarify is that as a non-entity disqus itself won't be able to have UUIDs but since disqus piggy-backs on the node entity we should be using that anyway.

Here's a patch that I'm considering using on a site that needs alternate ids due to migration. You'll note that I've opted to make the identifiers UUID based and non-gui-changeable. Much in the same way that UUIDs for entities are just what they are once created I'd like to think that in future when disqus enabled nodes are created, they will get a UUID that'll stay with them throughout any migration.

I'm happy to put a little more work in if there are necessary adjustments or improvements to the module. I'm also happy to open a dialog about any of the decisions I've made in this patch and attempt to justify why they were made.

Unassigning for the time being.

Re #29:
Thank you for your contribution. It is great to hear that you find this module useful.

Using UUIDs is an interesting idea, but I'd say we should probably postpone this until we port the module on D8. UUIDs are integrated in core with D8 and all entites get it by default.

If this goes in I'd definitely say we need to expose it in UI.

Patch in #29 includes some other fixes, that are not related to this specific issue (mostly Disqus API integration). That improvements should be discussed in separate issues.

I can absolutely abstract out Disqus API integration into another patch/issue but I'm not sure I agree with your point that UUIDs should wait until D8.

UUIDs are not a Drupal only concept and they would still be useful here. The UUID module is available in D7 and provides the same functionality that D8 core does - the only reason I opted on not making it a dep was because it really is entity focussed.

I would doubt the D7 version of Disqus warrants entity-ification although if you think the best option is to backport things from D8 then I imagine Disqus-7.x will have a fairly major overhaul to create custom entities and integrate UUID with them.

Happy to go with whatever is decided though :)

Just throwing my hat into the ring for a GUI feature like "attach Disqus comments to URL aliases instead of NIDs" or something.

I too am coming from a D6 to D7 migration and all of my comments are attached to the wrong nodes now, even though all my URLs are the same.

Can anyone clarify if #29 would help my current situation, or would that only apply to newly created nodes after the UUID module was installed? I'm a bit confused as the where this patch stands right now.

I'm happy to manually reattach comment threads to new NIDs this time around, but how would I do that?

I looked into the database and my D6 "disqus" table has DID (discussion ID?), NID (node ID), etc.

My "disqus" table on the new D7 site is completely empty, even though Disqus is up and running and people are leaving comments.

I tried changing some nid values from my D6 table that should have attached the proper discussions to the new nodes, then imported it into my D7 "disqus" table, but that didn't do anything.

What can I do to re-attach my comment threads to the proper nodes? None of the Disqus migration tools work either.

I've used the patch in #24 + #25 and changed the identifiers on all my blog posts so now the correct comments are showing up on my blog posts.

Now the issue is how to move on from here.

Before I applied the patch, I chatted with Disqus support, and they ported 1 comment thread over for me on their backend as a test (something not available with their migration tools). The comments for that thread were being displayed correctly, which was great, but produced other problems like the Title within the disqus.com administration area being incorrect, but linking to the correct page. It was not a manageable solution.

After applying the patch, manually updating the identifier to the previous node/NID works, but seems like a temporary solution. Will there be an upgrade path for this in the future using UUID or similar? Will I have to redo this every time I migrate a site and the node ids change?

Would it be better to export all my comments, completely clear my disqus.com comments, and re-import the comments attached to the correct node ids (if that's even possible)?

Can anyone outline what the next step would be? Ideally I'd like to do the work now so I don't have to deal with this again in the future if my node ids change.

Disqus API might be helpful with this problems. See: https://disqus.com/api/docs.

We have some API integration, but you'll probably need custom code to achieve what you need. Check examples in disqus.module to see how API integration works.

The question that is raised in #1175668-36: Add an option to override disqus_identifier is if the functionality in the sandbox should be considered a separate project or is it not considered distinct enough and perhaps that functionality get incorporated into this project directly? Depending on the answer to that question, I'm happy to work on getting the sandbox project promoted. But I don't want to cause module fragmentation if this is functionality that should get incorporated into here.

It definitely makes sense to include this into this module. There is a patch in #24 that is almost there. It needs just a bit of work and should be ready to go in.

I came across this issue at the tail end of a D6->D7 upgrade project, and unfortunately it wasn't until the very end that I bothered to test Disqus and noticed that the comments weren't lining up. After talking with Disqus support and looking at code and realizing that the node/$nid was used to link nodes with Disqus threads, I was able to implement this patch in concert with my migration code to get Disqus comments associated properly on the new site.

The migration code additions were pretty small. All I had to do was add a few lines to my DrupalNode6Migration::prepare() method:

  function prepare($entity, stdClass $row) {
    // Add source nid to Disqus field so that comments are mapped to new nodes.
    // Uses patch from https://www.drupal.org/node/1175668#comment-7847119.
    // nid 57403 is when Disqus comments were first used.
    if ($row->nid > 57402) {
      $entity->disqus = array(
        'domain' => variable_get('disqus_domain', ''),
        'url' => $entity->path['alias'],
        'title' => $entity->title,
        'did' => NULL,
        'developer' => 1,
      );

      $entity->disqus_identifier_auto = FALSE;
      $entity->disqus_status = TRUE;
      $entity->disqus_identifier = 'node/' . $row->nid;
    }
    else {
      // Pre Disqus, so we disable Disqus on the node.
      $entity->disqus_status = FALSE;
      $entity->disqus_identifier_auto = FALSE;
    }

This code does a few things:

1. For non Disqus nodes (the site had been using Drupal comments since it started in 2005 to a couple years ago), disable the Disqus comments and Automatic identifier checkboxes.
2. For Disqus nodes:
   • Fill in the title, url, and Disqus domain values
   • Disable the Automatic identifier checkbox
   • Enable the Disqus comments checkbox

There have been a few commits to the 7.x-1.x branch since the patch was rolled, so I've attached a patch rolled against the 2014-07-02 dev version.

Updating status so hopefully this can get committed, since I have it working on two large sites.

Sorry, bad form to RTBC one's own patch...

But thank you for posting the migration code. That's very helpful!

Well, if you look at the history, you'll see it's really not my patch. I didn't write it or change it; I just re-rolled it for the latest dev version and tested it on my sites.

Sorry, I missed the fact that #39 was a re-roll. Apologies.

Comparing #24 and #39 it looks like #39 removes the disqus.admin.js. Was there a reason for that?

What's the status on adding this to the module!? (Issue has been open for 4 years!)

A patch that adds hook_disqus_identifier_alter.

/**
 * Modify the Disqus identifier.
 *
 * @param array $data
 *  - identifier: The disqus identifier.
 *  - context: The object and type.
 */
function hook_disqus_identifier_alter(&$identifier, $context) {
  if ($context['type'] == 'node') {
    $identifier = 'drupal/' . $context['object']->id;
  }
}

@dolphinonmobile, can you include an interdiff when posting new patches? It help when following an issue that, as you point out, has been open for ages and only gets attention every now and then.

Thanks!

Since that last patch, there have been a lot of changes to the module, so this is a patch against the most recent dev version (2016-01-22), which is post-v7.x-1.12. It includes the stuff I missed in the patch in #39, plus disqus_ds changes from @dolphinonmobile's patch in #45. I have it working on a couple sites.

This took a few hours of work, so the sooner we could get this in, the better (I've had to re-roll it twice now).

Thanks.

in case anyone's interested in a multilingual drupal environment: with this patch you can load the same disqus thread on all the translation nodes for the same content.

in case anyone's interested in a multilingual drupal environment: with this patch you can load the same disqus thread on all the translation nodes for the same content.
(sorry wrong attachment in my previous comment, if anyone can delete it)

@wonder95, I just tried #48 and it does NOT yet work 100%. Here's what happens:

1) Per screenshot, it seems like it'll work just fine.

2) So I uncheck "Automatic identifier", enter a different node ID and save.

3) I double-check that it still doesn't work, so go back to edit the same page again.

4) I'm back to step #1, meaning #2 does NOT get saved and "Automatic identifier" is STILL checked with the same original node ID instead of the different one I just entered.

To double-check that the concept itself works, I went to line #263 of the patched disqus.module file itself and manually changed "$node->nid" to "'60'". This did work, getting ALL of my pages to show the same exact thread of Disqus comments for node 60 throughout my entire website.

In summary, either I did something else wrong (I did apply the patch to 7.x-1.x-dev dated 2016-03-03), or you forgot something. Can you double-check on your end? I'll gladly help out with testing within 24 hours of whatever new patch you create until this works fully on my website. Thanks!

@wonder95, I have some good news. On a hunch, I applied #39 to 7.x-1.10 and it DOES work. So now I just have to compare both patches and try to figure out why the hell #48 doesn't work for me. I'll report back if I find the source of this problem.

Ok I figured out what the issue was and eventually combined #39 & #48 into a new patch that works for 7.x-1.x-dev dated 2016-03-03. The source of the problem that I described in #52 was that some pieces of code from #39 were not included in #48, so I just put them back in. Here are a couple of additional changes:

1) Per screenshot, I noticed that there were two rows written to the database (for nid 60). After I deleted lines 374 - 381 of the original disqus.module file, I double-checked that there was only one row written (as can be seen for nid 59). So if these lines need to be put back in, then something else needs to be changed.

2) There was something weird in one of the two changes for #48 applied to the disqus_ds.module file, so I undid it because the original version looked fine to me.

This is my first patch, so please go a bit easy on me. I had a lot of fun creating it. :)

Confirming that patch in #54 works great, thanks!

@aasarava, if this works for you, can you mark it as RTBC so we can get this committed?

Marking as RTBC (remember, I just re-rolled this, it's not my patch).

I think this is more effort than it's worth, there's no need to store the identifier when it can be modified using drupal_alter (#1419562: Add a hook to inject variables into js file) and node hooks (#1873744: URLs and titles for comments don't update).

Even if this were to be added it would need an update script to fill in the value on existing records.