The attached patch fixes three items:

1. gigya_form_node_type_form_alter() was pulling the node type from the $form object incorrectly, getting a null value every time. This meant type-specific settings were never saved or loaded in the form.

2. In gigya_comments_ui_parmes(), if the node-type-specific settings were unset, the module would use hardcoded defaults. This patch forces them to use the defaults from the main gigya configuration.

3. Removed 'cid' and 'mode' parameters from the comments settings, since they don't appear in the API at http://developers.gigya.com/020_Client_API/010_Socialize/socialize.showC..., and appear to be cruft.

CommentFileSizeAuthor
gigya-comments-fallback-to-defaults.patch3.64 KBbrianV

Comments

sokrplare’s picture

sokrplare commented

Definitely needed fixes! Thanks for the patch.

sokrplare’s picture

sokrplare commented

Hmm...must have lost the changes applied from this patch when I moved to the latest dev version. I think this one might need to be re-rolled against the latest dev though should be super minor re-roll.

Re: item 2 in the issue description - I did the exact same thing in #1869474: Comments variables are not set so comments do not display before content type form is saved and was just about to close as dupe - however, I'm wondering if the lines saved by nesting variable_get calls is beneficial over splitting out into variables and if-statements as you have. I know variable_get is a little finicky though so was nervous nesting in the first place - thoughts?

brianV’s picture

brianV commented

This was This patch doesn't need a reroll - it was rolled against HEAD on Friday, last commit was 4 weeks ago.

I tend to prefer the way I did it in this patch (obviously), due to it's (IMO) better code readability. That said, I don't really care which version goes in!

That said, it might be worth just focusing on one of the two issues, since the maintainers seems to be a bit slow to respond...

sokrplare’s picture

sokrplare commented

Yeah, I don't particularly care either. Re-reroll - hmmm, I'd thought I saw $type used .... ah, was mixing up which function I was looking at. Nevermind the reroll! :)

sokrplare’s picture

sokrplare commented
Priority: Normal » Major

For the record, Gigya's product manager (Liran) kindly emailed the other day to get impressions on the module. We've exchanged emails and he noted that...

"Sorry for not being as active as I'd like in our forum recently, but we're currently very busy implementing Gigya in other platforms (Magento, Hybris, Demandware, Wordpress and more)...That being said, the patches ... are very helpful and I can assure you that we will review every one of them, once we go back to working on a new Drupal release in a few weeks time." (emphasis mine)

In my reply I requested the patch in this ticket be committed to dev if at all possible since it is a blocker for all new users. Switching this to major accordingly. However, they might just be overloaded and not able to hit it until Jan.

Nice that they are being active with it though.

gambaweb’s picture

gambaweb commented
Status: Needs review » Fixed

thanks for the patch pushed to the dev

Status: Fixed » Closed (fixed)

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