Thnx for this module. Very useful.

Problem: radio buttons missing. See attached screen shot, missing-radios.png.

Working through the code, found that tags for the radios were not being generated by Drupal. Maybe because the ['#options'] array created in diff_node_revisions() (in the file diff.pages.inc) had keys, but all the values were MT (empty). See the attached file, option-values-mt.png. It's a debug screen, showing the state of the form array created by diff_node_revisions(), just before it returns.

Replaced line 68:

$revision_ids[$revision->vid] = '';

with:

$revision_ids[$revision->vid] = $revision->vid;

The options now have values at the end of diff_node_revisions(). The radio buttons reappear (see screenshot radios-present.png, attached). They work as expected.

Using the bootstrap theme.

CommentFileSizeAuthor
#5 diff-2068015-5-revert-commit.patch514 bytesAlan D.
#2 diff-2068015-2-missing-radio-fapi-elements.patch514 bytesAlan D.
option-values-mt.png37.42 KBmathieso
radios-present.png26.39 KBmathieso
missing-radios.png20.34 KBmathieso
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Tilo’s picture

Tilo CreditAttribution: Tilo commented

Confirmed - we had the same issue.
Great job Mathieso, many thanks

Alan D.’s picture

Alan D. CreditAttribution: Alan D. commented
Version: 7.x-3.2 » 7.x-3.x-dev
Status: Active » Needs review
FileSize
diff-2068015-2-missing-radio-fapi-elements.patch514 bytes

Fix looks fine, but it is strange that this has only been reported twice from ~20,000 installations. Can I ask what Drupal version, PHP version and OS you are using? And are there any special theming functions for the radios or modules that alter radios?

mathieso’s picture

mathieso CreditAttribution: mathieso commented

Alan,

Yes, I thought it was strange as well. I'm using the Bootstrap theme. Other than that, everything is standard. CentOS, PHP 5.3, D7.22, etc.

My guess is that it's an interaction with Bootstrap, but I'm not sure.

Kieran

Alan D.’s picture

Alan D. CreditAttribution: Alan D. commented
Status: Needs review » Fixed

Seems harmless enough. Committed. 932792d :)

Thanks

Alan D.’s picture

Alan D. CreditAttribution: Alan D. commented
Status: Fixed » Closed (works as designed)
FileSize
diff-2068015-5-revert-commit.patch514 bytes

Not so harmless, I totally overlooked the labels showing... so I had to revert this.

Please use the latest Bootstrap theme: #1649392: Checkbox or radio inputs without titles don't render.

If you are using another theme, please report first to that themes issue queue, referencing the Diff module in the issue description.

Cheers

gmclelland’s picture

gmclelland CreditAttribution: gmclelland commented

@Alan D. - Your patch in #5 does remove the revision ids, but in some cases that can be helpful to know the revision id.

For example when using the https://drupal.org/project/revision_scheduler you have to know the revision id to schedule a publish/unpublish of a revision.

This might need to be a separate issue, but one strange thing I see with or without your patch is that sometimes you see two radio buttons listed for each revision.

You can actually see the problem in this image
Double radio buttons

gmclelland’s picture

gmclelland CreditAttribution: gmclelland commented

So the problem I was seeing in #6 is because diff module is using the functionality provided by the node module which is already reported #1881318: Make node diff_node_revisions() operations extendable

I still think the revision id would be helpful to have.