While working on #1851086: Replace admin/people with a View, I discovered that user_admin_account() has a new call to if (module_invoke('translation_entity', 'translate_access', $account)) {
.
That now makes for one of my last failing tests, because there is no field for a translation link.
The entire module has no integration, and it's out of scope for the other issue to add it. (If it was the case of just adding a field handler, I'd have done that already :))
Comment | File | Size | Author |
---|---|---|---|
#27 | et-1896268-27.patch | 12.02 KB | tim.plunkett |
#27 | interdiff.txt | 929 bytes | tim.plunkett |
#25 | et-1896268-24.patch | 12.04 KB | tim.plunkett |
#25 | interdiff.txt | 819 bytes | tim.plunkett |
#22 | et-1896268-22.patch | 11.9 KB | tim.plunkett |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedComment #2
plachYes, we need this. I think the main problem right now is that we have not decided yet how to deal with the storage of entity translations, I am not even sure we will store them. As a consequence I'm not even sure the
{translation_entity}
table, which is what the D7 views integration revolves around, will stay. See #1807800-41: Add status and authoring information as generic entity translation metadata.I think this should be postponed to #1810370: Entity Translation API improvements.
Comment #3
tim.plunkettWell, if we want to postpone full integration on that, we can make this a meta, but we could easily add a field handler for entities that does
Since that's all we need for the admin/people and admin/content tables.
Comment #4
plach@tim.plunkett:
As you think it's better, no strong perference about this.
Comment #5
tim.plunkettHere's what I had in mind.
Comment #6
plachThanks!
The
hook_views_data()
stuff should go into the respective modules: ET has no dependencies, not even soft ones, on entity-defining modules.Comment #7
tim.plunkettI was surprised to see
module_exists('translation_entity')
sprinkled throughout core, so it seems I have the exact opposite viewpoint: why should I as an entity type provider have to care about translation_entity, when the handler is generic and could/should be handled elsewhere? It basically means that contrib modules won't bother.Comment #8
xjmSo I went back and forth about which module(s) should be providing the data integration. In this case I think
hook_views_data()
in each entity module is the right choice, because non-core modules will not have the option of adding totranslation_entity_views_data_alter()
. This might mean that some contrib modules fail to expose their data for translation_entity, but if so, that can be treated as a bug in those modules.(That said I do think some of the
module_exists()
for translation entity are kind of awkward. E.g., user_admin_account().)Comment #9
plachI don't think it's correct that ET has to know anything about the modules integrating with it: for instance contrib modules wishing to leverage the translate link will have to do exactly what I suggested in #7. ET provides yet another entity-level feature, I think asking it to integrate with any possible entity-defining module out there makes no sense. It would be like asking the Entity module to provide the necessary entity info keys to integrate the various controllers for nodes, terms, comments, users and so on. This is reversing the dependency chain.
ET is coded in a way that it doesn't need to know anything about entity-defining modules. OTOH if a module wishes to integrate translation it needs to provide the related keys. Hence it (softly) depends on ET. It may sound blasphemy but, yes, Node depends on ET and not viceversa.
The only way I'd see for going the way you are proposing would be to define yet other entity info keys allowing ET to alter views data in a generic fashion. And also in this case it would be Node's (and friends') responsibility to provide them.
Comment #10
plachI totally agree, we have an issue that may help with this: #1446382: Need a reliable way to determine if a specific bundle for an entity type is translatable.
Comment #11
tim.plunkettPer #8, I think this works out well, mostly because it demonstrates to contrib what to do.
Comment #12
tim.plunkettentity_test apparently doesn't support multilingual, only entity_test_mul does? And there are seven or so entity_test_* entity types, only one of which has basic Views integration. So I used user instead.
Comment #14
dawehnerIs there any reason why we don't iterate over all entity types in entity_translation.views.inc?
What about override click_sort() in the handler?
Missing doc :)
Is there a reason why we use module_invoke and not a simple function call, but in general I never understood the advantage.
should be probably protected.
Comment #15
plachWell, as I said in #9 if there's a way for ET to deal with that stuff in a generic way I have no problem to include those in entity_translation.views.inc.
Comment #16
tim.plunkettThere is no easy or generic way I know of, especially since it checks 'user' and then adds to the 'users' data, and 'taxonomy' => 'taxonomy_term_data'.
That module_invoke is certainly ugly, but it is moved code.
Yes, it 100% should be protected, but the parent method is not, and fixing that in 4 classes is out of scope :(
I'll fix the docblock and the click_sort in a bit.
Comment #17
plachWould it make sense to move the code wrapped by the ifs in its own autoloaded class? Can we leverage plugins here?
Comment #18
tim.plunkettIf by that you mean
if (module_invoke('translation_entity', 'translate_access', $entity)) {
, I have no idea. I don't know how plugins would help with that.This addresses the feedback from #14.
Comment #20
YesCT CreditAttribution: YesCT commented#1897770: Make getTranslatorPermissions() protected follow-up for #16 (and last part of #14)
Comment #22
tim.plunkettEntityTranslationUITest contains really useful helper methods to set up the translation UI. However, despite it being an abstract class, it contains a test method. So there is no way to use those helper methods without having to override that test method. This is more follow-up fodder :)
Comment #23
tim.plunkett#1807776: Support both simple and editorial workflows for translating entities does the refactoring we need, but it's buried inside a feature request. Once this is in I'll help reroll that one.
Comment #24
dawehnerThe only thin I'm wondering about is whether we should test the case in which the user does not have access to the entity.
Comment #25
tim.plunkett#24 is a good idea.
Comment #26
plachNot sure this is needed: given that the plugin is part of ET can we hust call translation_entity_translate_access()?
Otherwise looks RTBC to me.
Comment #27
tim.plunkettAh, good call.
Comment #28
dawehnerPerfect if it is green!
Comment #29
damiankloip CreditAttribution: damiankloip commentedSorry, just a small nitpick, otherwise I agree this is good.
Just wondering, should this not say '...overview for nodes' but '..overview for a node' ? Same with all of the others. That might mean users think it's going to some super translation page for ALL THE NODES?
Eck :/ I guess this can't be helped now..
Comment #30
webchickLooking at the other fields' labels it seems like they're a little all over the place wrt singular vs. plural, so I think consistencizing them everywhere can be a follow-up. And yes, the things we are doing to perfectly lovely wrapper functions in D8 make me want to head-desk. :(
From the "Things that make you go 'Buh?!'" department...
This is a link to node/1/translations and the like, not node/1/view. So it should not be labeled 'view.' :)
Otherwise, this looks good to me, so I fixed that up on commit (renamed to 'translate') and...
Committed and pushed to 8x. Thanks!
Comment #31
plachActually, now that I think about it, node and comment links use the "Translations" text. Maybe a small follow-up?
Comment #32
webchickYou mean relabel the link to "translations" instead? I thought about that but no, we want parity with what admin/content and the like listings are doing now. Those dropbuttons all use "action" keywords: 'view', 'edit', 'translate'.
Comment #33
plachThen I guess comment and node links are wrong, because functionally those links are the same.
Comment #34
webchickOh. Right you are. :) Didn't notice that.
I dunno though. On nodes we also have "revisions" tab, which is a noun. But it's worth filing a follow-up so we can figure out how/if we want to reconcile that.
Comment #35
YesCT CreditAttribution: YesCT commentedfollow-up: #1899486: unify dropbutton use of translate/translations wording in links