The poll module doesn't provide any default choices on translation which is counter to the default behavior of other fields like title, body, etc.
This patch catches the translate_prepare hook and populates the choices. Patches for HEAD and D6 supplied.
Comments
Comment #1
neclimdulComment #2
zroger CreditAttribution: zroger commentedI've only reviewed the drupal 7 patch.
It's very simple and obvious what it does. Let's get this in.
Comment #3
stella CreditAttribution: stella commentedLooks good to me too, and patches apply cleanly.
Comment #4
stella CreditAttribution: stella commentedComment #5
webchickSo this looks like a very straight-forward fix, and I imagine it is probably blatantly obvious to someone familiar with the translation system what this fixes, but it is not in anyway obvious to me. :)
Is it possible to make a small test that demonstrates the bug this is fixing? That would help me to understand why it's important it be committed.
Comment #6
caktux CreditAttribution: caktux commentedconfirming that.. to reproduce, it assumes the poll content type is set to have multilingual capabilities with translations (3rd i18n option in workflow settings), a poll already created set with the default site language, and when you go to translate that node, choices don't get populated from the original node.
Great patch :)
Comment #7
webchickAh-ha. Great. Thanks for the summary, caktux!
Given that that's the issue, it doesn't really make sense to add a test here, since this is just one instance of the hook. This would rather go into a more general translation system test suite testing all hooks, including this one (incidentally, I would love to see said tests written if they don't exist yet).
Committed to HEAD. Moving to 6.x for consideration.
Comment #8
webchickAhem. :P
Comment #9
Gábor HojtsyThis is indeed an omission in the Drupal 6 language system. I think we did not realize that it is this obvious to implement and postponed it as long as we were out in feature freeze.
Anyway, this patch looks great indeed. However, I'd like to point out an omission and get that fixed before we get this committed to Drupal 6. The cloned options also copy the vote values from the original node, which IMHO should not be done by default. The default behavior should be to null the votes out for creation of the translation (in the cloned node). There is similar code in poll_insert() in Drupal 6, but that only ever applies if the user is not a node admin. So the vote counts will be right for non-admins on cloning, but admins would need to clear out the votes on translation.
I'd love to get this committed to Drupal 6 after the above fix is committed to 7 and all changes are in one patch for Drupal 7.
Comment #10
neclimdulComment #11
neclimdulBah, forgot the comment. updated to handle Gabor's comments.
Comment #13
amateescu CreditAttribution: amateescu commentedRerolled the D7 patch from #10, now applies cleanly to D7 and D8.
The patch for D6 is still good to go after this one gets in.
Comment #14
amateescu CreditAttribution: amateescu commentedRe-rolled for the great '/core' migration in D8.
Comment #15
Gábor HojtsyLooks good and simple and is discussed at great length above, should be RTBC once tests pass IMHO.
Comment #16
amateescu CreditAttribution: amateescu commentedThere we go, all green & happy :)
Comment #17
catchOK this looks fine, so I've committed/pushed to 8.x.
After that I realised 1. do we still want to backport this to 7.x and 2. that I should've knocked it back to CNW for tests. So.. let's add the tests here (or find out why it's not worth it), then consider backport or not?
Comment #18
Gábor Hojtsy@amateescu: can you help with writing a test for this? My understanding is the test would do the following:
1. Create a poll node, cast a vote.
2. Mark the poll node type translatable (enable translation module).
3. Create a translation.
4. Ensure that the vote count on the translated poll is 0 (ie. the previously recorded vote is not present on the translation).
If the patch is rolled back 4 would fail, otherwise 4 would pass.
Comment #19
amateescu CreditAttribution: amateescu commentedYep, I'll do that in a couple of days.
Comment #20
c31ck CreditAttribution: c31ck commentedI've created a test that:
- Creates a poll.
- Marks poll content type as translatable.
- Sets a vote count on the poll.
- Tries to translate the poll and checks that vote counts are not copied from the original poll.
The first patch should pass, the second rolls back the changes from #14 and should fail.
Comment #22
c31ck CreditAttribution: c31ck commentedTests behaved as expected. Setting to needs review.
Comment #23
Gábor HojtsyThe test looks good. I think the checking for the addition of the new language is a bit too verbose. We don't need to check that twice. Checking for the drupal set message text on the resulting page is fine. I guess you used the language name from the language_list() output. However, you can just hardwire 'Dutch' in the t() arguments since it is a known value for the test. Otherwise looks pretty complete.
Comment #24
amateescu CreditAttribution: amateescu commentedc31ck was faster :)
Comment #25
c31ck CreditAttribution: c31ck commentedI'll change the test according to Gábor's comments tonight.
Comment #26
c31ck CreditAttribution: c31ck commentedChanged test according to #23: Check enabling of language only once and hardcoded 'Dutch' in the t() args.
The first patch should pass, the second rolls back the changes from #14 and should fail.
Comment #28
c31ck CreditAttribution: c31ck commentedTests behaved as expected. Setting to needs review.
Comment #29
Gábor HojtsyTagging as current focus. Looked through the tests twice and the proposed testing method looks good and comprehensive. The patch already landed, so we only need the tests to land.
Comment #30
xjmTest looks excellent, but is missing some API documentation:
The class and test method need docblocks. See: http://drupal.org/node/1354#functions and http://drupal.org/node/1354#classes.
I won't set the status back, but it would be nice if someone could reroll to add those two docblocks. Otherwise, we'll have to have a followup issue for it which sort of violates the whole documentation gate thing. :P Tagging novice to add some docblocks to the patch in #26.
Comment #31
Dries CreditAttribution: Dries commentedLet's add the documentation first, and then set back to RTBC. Thanks!
Sounds like we're really close.
Comment #32
Gábor Hojtsy@Dries: agreed. @amateescu or @c31ck can you do that? Thanks a lot!
Comment #33
c31ck CreditAttribution: c31ck commentedAdded docblocks to the class and the test method.
Comment #34
amateescu CreditAttribution: amateescu commentedLooks good to me.
Comment #35
xjmThe summaries should start with third-person verbs. Please review the referenced points in the API doc. ;)
Comment #36
Gábor HojtsyComment #37
no_commit_credit CreditAttribution: no_commit_credit commentedComment #38
no_commit_credit CreditAttribution: no_commit_credit commentedSorry, this is a bit better.
Comment #40
c31ck CreditAttribution: c31ck commented#38: 404116-need-coffee.patch queued for re-testing.
Comment #41
Gábor HojtsyIt is a set of test additions only with now even better comments. So should be good to go again :)
Comment #42
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks for adding the documentation so quickly.
Comment #43
amateescu CreditAttribution: amateescu commentedComment #44
Gábor HojtsyTagging for the content handling leg of D8MI.
Comment #45
c31ck CreditAttribution: c31ck commentedPatch that resets the vote count values on translation and adds the test.
Comment #47
c31ck CreditAttribution: c31ck commentedTests behave as expected, resetting status.
Comment #48
amateescu CreditAttribution: amateescu commentedSimple re-roll of the D8 patch.
Comment #49
webchickCommitted and pushed to 7.x. Thanks!
Bumping down to 6.x.
Comment #50
Albert Volkman CreditAttribution: Albert Volkman commentedRe-roll of initial patch by neclimdul.
Comment #51
Albert Volkman CreditAttribution: Albert Volkman commented