Posted by nedjo on January 8, 2009 at 5:28pm
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | translation.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | patch (to be ported) |
| Issue tags: | i18n sprint |
Issue Summary
If a user returns to a page where a translation was previously created, s/he gets the same form and can create a duplicate translation.
Steps to recreate:
* Designate the page content type as translatable
* Enable two languages (en, fr)
* Create a page in en
* Translate to fr, noting the page, e.g., node/add/page?translation=1&language=fr
* After saving, return to the same page. Fill out the form again and submit.
Result:
* Two different translations in French, both with the same tnid.
Attached patch prevents this error by testing for existing translations in the requested language.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| translation-prevent-duplicate.patch | 1.45 KB | Idle | Unable to apply patch translation-prevent-duplicate.patch | View details | Re-test |
Comments
#1
#2
We also need a check for this at submit time.
#3
Good point.
I guess this would be a translation_nodeapi_validate() function:
* Test for node type is translatable
* Determine the applicable tnid by checking node->tnid or node->translation_source->tnid
* get translations (translation_node_get_translations($tnid), set form error on 'language' field if the language value is already in the translations array.
#4
@nedjo: sounds good.
#5
I added translation_nodeapi_validate() but then discovered that hook_nodeapi_prepare() actually fires even when using the back button, and since it was wiping out the node->language etc. before we got to submit, we end up with the same drupal_set_message() and no duplicate on submit anyway.
So, this patch keeps the _prepare(), doesn't add a validate, and adds tests for both visiting the node/add page again manually, and also a resubmission of the same form (which is equivalent to using the back button).
In both cases, you can create the node you're posting, but it'll simply not have a tnid - it'll be new content in it's own right. I'm not convinced this is ideal from a usability standpoint (although you're already at this page incorrectly anyway if you get there) - but it does fix the bug.
This could probably do with some manual testing by someone to confirm the same behaviour I'm getting - i.e. that we really don't need a _nodeapi_validate(). I think if we were to remove the early return; in _prepare() then we'd actually need the _validate() as well, which'd allow for form_set_error() to prevent posting at all - but that's adding extra code to do much the same thing, so if it's not needed, I don't think we should go out of our way to add it in.
#6
Discussed this with nedjo in irc, and there's situtations where you might get the _validate() being called in contrib even if it doesn't happen in core. So I've added that back in.
#7
I tested this and it all works great, including tests.
Cheers,
Stella
#8
A quick summary of the issue:
Issue:
Patch:
Cheers,
Stella
#9
The last submitted patch failed testing.
#10
Patch re-roll
#11
Marking back to RTBC
#12
The last submitted patch failed testing.
#13
Marking for a retest
#14
moving back to RTBC
#15
Oops. Sorry! I thought I had committed this already!
Committed to HEAD. :)
Marking down to 6.x, as I believe this is an issue there as well, yes?