Problem
Drupal 7 had two translation systems, node copy based content translation with a core UI and field based translation with a core API and a contrib UI. Drupal 8 attempts to standardize on field translation (which applies to entities of all kinds not just nodes). However, Drupal 8 still has both modules in the codebase. The legacy module is hidden from sight for new sites. We are not providing upgrade path in 8.0.
Proposal
Remove the module from the codebase, and tests that test it.
Dependencies
none.
Comment | File | Size | Author |
---|---|---|---|
#70 | 1952062-translation-module-70.interdiff.txt | 1.98 KB | kfritsche |
#70 | 1952062-translation-module-70.patch | 75.19 KB | kfritsche |
#67 | interdiff.txt | 1 KB | penyaskito |
#67 | 1952062-translation-module-67.patch | 75.87 KB | penyaskito |
#63 | interdiff-58-62.txt | 5.41 KB | vasi1186 |
Comments
Comment #1
Gábor HojtsyCurrently postponed on #1952044: Migration path for legacy content translation module data.
Comment #2
Gábor HojtsyMarked #1834276: establish what needs to be done to remove Content Translation from core and then remove it duplicate of this one, added #1807322: Filter comments taking into account the current content language to the dependencies list.
Comment #3
plachLinked this in #1836086: [meta] Entity Translation UI improvements.
We tried to bring on the comment stuff during the sprint weekend, but we were stopped: #1938096-4: Convert the node comments list to a view. It seems we should ping @amateescu to understand if we can rely on him to fix it.
Comment #4
chrishks CreditAttribution: chrishks commentedFrom line 2044 in comment 303 on #1498674-303: Refactor node properties to multilingual - remove this code
Comment #5
yched CreditAttribution: yched commentedAs part of #1969728: Implement Field API "field types" as TypedData Plugins, we'd welcome your opinions on #2018707: Do we still need to support hook_field_prepare_translation() ? :-)
Comment #6
catchCould we move this to contrib for now? Even if this isn't completely replaceable yet it's very close, and shipping with it seems a lot worse than shipping without.
Comment #7
Gábor HojtsyDo you mean the upgrade path would be in contrib too? Or that we should work on that later? We did not have the opportunity to work on that since we cannot reproduce the functionality yet.
Comment #8
catchI'm not sure on the upgrade path bit of this. Is there anything actually blocking an upgrade path in core from the point of view of the data itself or is it just that some features can't be replicated yet? We only guarantee people's data, not features, so that shouldn't block it IMO.
The main concern I have with this is that we end up with another profile module situation - it was marked hidden at the final hour of Drupal 7 because it made no sense to present it to new Drupal users, but we still have a critical issue open for an upgrade path even now.
So if we keep translation module around, then end up marking it hidden just before release, we could be back here at the end of 9.x with it still not completely nuked yet.
Comment #9
Gábor HojtsyI think ATM the situation is more like the set of people who could write this were busy with building features when that was allowed, now busy with API changes while that is still allowed and then if the upgrade path is possible we could focus on that after API changes / criticals are done.
Comment #10
plachWhat Gabor said.
The main critical issue with the upgrade path is that we will be merging nodes, which sounds like a lot of potential pain. In contrib D7 the upgrade path is a standalone module that provides a UI, hooks, and a functionality to redirect from the old urls to the new ones:
I am not sure whether it makes sense to have such code in core. It sounds much more like a D7 CCK thing, even if we don't provide a UI for it.
Comment #11
catchAssigning to Dries. It makes sense to me to have the upgrade path for this in contrib given it's not at all straightforward.
If the upgrade path is in contrib, then I think we could remove translation module now - I don't see any value to shipping Drupal 8 with both methods.
Comment #12
plachAs a side note @Jose Reyero is planning to revive the node translation module at https://drupal.org/project/translation.
Comment #13
Gábor HojtsyMarking as critical on the basis of it being a module removal and add upgrade path tag since this will set up an external upgrade path dependency for the eventual Drupal 8 release (much like it is with Views as far as I understand). This should be evaluated and executed or dismissed before the beta release.
Not sure how we track external upgrade path dependencies.
Comment #14
Dries CreditAttribution: Dries commentedI think it makes sense to remove the legacy approach from core. I'd be ok with fixing the upgrade path in contrib. If that proves to be problematic, we can always decide to move the ugprade path back to core.
Comment #15
Gábor HojtsyI've been thinking maybe git rm would be enough to do here (no patch needed), but there may be core hooks implemented that would need code removed from other parts of core. This may need a little bit of exploration.
Comment #16
plachShouldn't we wait to be able to translate titles with the Content Translation module?
Comment #17
Gábor Hojtsy@plach: all the required issues for that are marked critical (if not, they should be), so we already committed to not shipping Drupal 8 without solving that. Also the legacy content translation module is already hidden on new installs, so practically users don't have any way to translate node titles at the moment. Removing the module would not make a difference for regular users on new installs at this point.
Comment #18
Gábor HojtsyTag for upgrade path and moved onto sprint. The hooks need to be found that are implemented for this feature (especially in node module but possibly in some field modules). It is likely not just a git rm.
Comment #19
CMS Dude CreditAttribution: CMS Dude commentedAssigning this to myself ; working at Midwest Drupal Summit -- first issue.
Comment #20
CMS Dude CreditAttribution: CMS Dude commentedRemoving Translation Module
Double-checked to see if any other modules/code is calling on any of the old functions from the Translations module before deleting.
In #18 @Gábor Hojtsy
In IDE, did a search for any function calls that match the string "translation_," as well as any hooks that match string "_translation" Found no instances of any hooks or other functions defined in old translation module.
Comment #21
CMS Dude CreditAttribution: CMS Dude commentedComment #23
klonos...this usually helps ;)
Comment #24
klonos...xpost :/
Comment #25
YesCT CreditAttribution: YesCT commenteda bunch of fails are tests, upgrade path tests too, which are trying to enable the module.
we should edit those tests and not enable the module.
let us look at the tests and see if tests in content_translation are covering similar situations.
Comment #26
jair CreditAttribution: jair commentedComment #27
manningpete CreditAttribution: manningpete commentedRerolled.
Comment #28
manningpete CreditAttribution: manningpete commentedSetting it for needs review for testbot to run. Still needs work.
Comment #29.0
(not verified) CreditAttribution: commentedAdd new dependency.
Comment #30
webchickTagging all critical D8 upgrade path issues as "beta blocker."
Comment #31
Gábor HojtsyLooking at other modules, there are at least a few other things that should be removed:
- tnid from node table
- tnid based handling from views
Maybe there are other things that I did not yet find, this is just a quick look. The tnid schema actually references this issue in the code. Obviously we cannot remove the tnid data, we need it for migration in the contrib migration module. We should move it aside in the upgrade to an upgrade-only table, so the contrib upgrade module can still see it, but it is not in the live node table data.
Comment #32
vijaycs85removing translation module and permission related to it from few other test cases.
Comment #33
vijaycs85Adding tag...
Comment #35
Gábor HojtsyStill fails, but less than above :)
Comment #36
vijaycs85It looks like test assumes that translation is separate node. So had to hardcode language code few places. Issuing patch to make sure in right direction.
Comment #38
vijaycs85Re-rolling with current code base.
Comment #40
vijaycs85Fixing drupalPost() issue.
Comment #42
Gábor HojtsyI think this is now outdated, see #2083811: Remove langcode nesting for fields in entity $form structures, which should explain the fail :)
Other fails where eg. the upgrade path tests for renaming of permission form translate content to translate all content are just outdated. Since the update function is removed, there is no point in testing for that. That is an orphaned permission.
Comment #43
vijaycs85Thanks for the update @Gábor Hojtsy. Issuing new patch with below changes:
1. Removed upgrade path test - As it is trying to check different translation nid which is not anymore case in D8.
2. Updated LanguagePath test to get the latest code from #2083811: Remove langcode nesting for fields in entity $form structures
Comment #45
willyk CreditAttribution: willyk commentedI reviewed and tested the last submitted patch and it appears that including translation in ViewTestBase still causes the errors in HandlerAllTest due to some of the likely deeper views hander issue. Wondering if we should remove it there just to see if it prevents the failure?
Comment #46
willyk CreditAttribution: willyk commentedActually correction to the above #45, it appears that modifying/updating HandlerAllTest.php so that it doesn't enable 'translation' should resolve what's causing 1952062-translation-module-43.patch to fail testing.
Comment #47
willyk CreditAttribution: willyk commentedAs mentioned above, updating HandlerAllTest.php to enable 'translation_content' rather than 'translation' in order to try and resolve what's causing 1952062-translation-module-43.patch to fail testing. Hopefully this passes now.
Comment #48
willyk CreditAttribution: willyk commented@Gábor Hojtsy, just in case you want to remove from testing entirely, here's another patch that removes the old translation, as opposed to updating what's enabled as in the above 1952062-translation-module-47.patch.
I'm guessing the patch in 47 is preferred approach; but figured I add this one as well, since I wasn't certain the preferred approach given the status/timing we discussed yesterday. If either patch passes, perhaps you could choose whichever serves the purpose.
Comment #49
vijaycs85Comment #51
willyk CreditAttribution: willyk commentedApologies for the error in 1952062-translation-module-48.patch. The issue there should be corrected and the other two translation instances mentioned by @vijaycs85 should also be addressed in this revised patch.
Comment #52
vijaycs85Comment #53
pfrenssenOrder them alphabetically now that we have a nice list?
Move this to the top of testAliasTranslation. See the comment there.
An unneeded empty line is added, as well as a double space after the '='.
Comment #54
willyk CreditAttribution: willyk commentedCan you please clarify what you mean by "Move this to the top of testAliasTranslation. See the comment there." I don't see the comment you're referring to and am not following what you mean by the move. Ping me @willyk in #drupal-i18n if it's easier to discuss there. An updated patch is attached that includes the other two changes.
Comment #55
pfrenssenThe patch in #54 does not apply:
I will roll a valid patch.
Comment #56
pfrenssenIgnore my comment in #53 about moving the code block that enables translations for Page nodes. Looking closer the rest of the related configuration is also done in setUp(), so it is more consistent to keep it there.
Comment #58
willyk CreditAttribution: willyk commentedAttached in another patch that that addressed the problem that caused #54 to fail. It should now apply if someone wants to review :).
Comment #59
tim.plunkettI bet the bot will test!
Comment #60
pfrenssenHere's the interdiff between #56 and #58. Only some whitespace changed. The line that was added was not needed, and the one that was removed is usually left in place to indicate the end of a class definition. Whitespace-wise, #56 is a better patch :)
The failure in #56 is probably not related to the patch.
Comment #61
pfrenssen#56: 1952062-translation-module-56.patch queued for re-testing.
Comment #62
vasi1186 CreditAttribution: vasi1186 commentedRemoved some more code.
Comment #63
vasi1186 CreditAttribution: vasi1186 commentedand the interdiff
Comment #64
vasi1186 CreditAttribution: vasi1186 commentedComment #65
Gábor HojtsyWoot, great diffstat!
25 files changed, 31 insertions(+), 1464 deletions(-)
:)
Comment #66
Gábor Hojtsytagmonster
Comment #67
penyaskitoFor reviewing this, I searched for "module_exists('translation')". I found another reference to translation module in hook_system_info_alter implementation in node and I removed it.
I rgrep for 'tnid' and the only reference is ./core/modules/system/tests/upgrade/drupal-7.field.database.php, and that's OK.
So the attached patch only removed this hook_system_info_alter implementation in node.module.
There are still references to the translations paths in node_admin_paths, maybe this should be removed too or moved to content_translation module?
Comment #68
Gábor HojtsyWhat are those admin paths? Sounds like they may not apply to the other module anymore (or it has them already)?
Comment #69
penyaskitoAs we saw, they are the node/{nid}/translations, and they belong to node module.
So everything is OK from my POV. Reviews required.
Comment #70
kfritscheCompletely unrelated change, so I'm removed it.
This is just a minor code style issues. Fixed it.
Otherwise it seems good.
Manually tested installation process and one translation of a node, to be sure that nothing interfered.
RTBC if testbot not disagrees (only minor code styling change)
Comment #70.0
kfritscheUpdated issue summary.
Comment #70.1
YesCT CreditAttribution: YesCT commentedsince we are not doing upgrade path
Comment #71
catchThis is great. We already marked this module hidden for new installs, and with a migrate-based upgrade path the route from different nodes with tnids to translatable nodes will be a new source + some tweaking rather than a very complicated hook_update_N(). This needs to go in before we offer 8-8 updates due to the schema changes in node module itself.
Committed/pushed to 8.x, thanks!
I think we already have a change notice relating to this, but will either need to update it or create a new one if it's not covered properly yet.
Comment #72
vijaycs85Thanks @catch. I guess, we need to replace/update documentation on http://drupal.org/documentation/modules/translation_entity page. (including URL change and redirect ). Created new follow up to update this link in content_translation_help at #2102041: Update the URL of content_translation document in content_transation hook_help
Comment #73
plachWoot! Amazing work guys!
Comment #74
Gábor HojtsyWrote specific change notice for this at https://drupal.org/node/2103189
Updated existing change notice at https://drupal.org/node/2028009
Reopened #1952044: Migration path for legacy content translation module data for renewed core consideration.
Comment #75.0
(not verified) CreditAttribution: commentednope, that issue being wont fix is not about no upgrade path. so just saying it.
Comment #76
klonos...adding back tags eaten by the tag monster and moving related issues to their dedicated metadata section.
Comment #77
plach