Problem/Motivation
Entity operations delegates URI generation to entities at all times and therefore generate links to the original language variant of the entity at all times. This is a problem both for content and config entities. (1) for config entities operations always switch language to the original language of that particular entity (2) for content entities, operations links on translations are never generated, translations also return links to the original language variant. This results in the following confusing/incorrect UX:
Config entity operations:
Content entity operations:
This may cause unintended data loss with the delete operations for example if the user does not read the confirmation form carefully. Trying to delete a translation of a content entity will lead to deleting the whole entity and only the confirmation form stops the user.
Additionally, the content entity edit form natively handles translations, but it is not accessible by users having only translation permissions, hence the related edit links are not displayed in that case.
Proposed resolution
- Config entity operations should avoid explicitly forcing a language and should fall back on the request language.
- Content entity operations should use the proper language of the translation to be handled. This works for edit and delete links.
- Delete on the original language still deletes the whole entity, for that to not happen, we need #2443991: Allow default_langcode field value to be changed, not to be resolved here.
- However edit/delete on content entities will be accessible only by users having update access on the entity. So additionally, make the Content Translation module provide two more operations (and the corresponding Views entity link handlers): "edit translation" and "delete translation", which will point to the translation form and the translation deletion form provided by CT. These will be accessible only by users having translation permissions and not having update access on the selected entity.
Remaining tasks
Discuss. Write tests.
User interface changes
- Config entity operations will keep the current page language in the links (instead of original language of entity)
- Content entity operations point to the right translation instead of always to the original language of entity
- Translators who don't have node admin permissions can still edit and delete translations thanks to new operations
API changes
None foreseen at the moment.
Comment | File | Size | Author |
---|---|---|---|
#79 | 2476563-entity-operations-79.patch | 17.64 KB | penyaskito |
#7 | 2476563.patch | 6.94 KB | amateescu |
#11 | log-node_admin-1.pdf | 46.72 KB | plach |
#11 | log-node_admin-2.pdf | 53.97 KB | plach |
#22 | 2476563-22.patch | 6.08 KB | amateescu |
Comments
Comment #1
dawehnerComment #2
stefan.r CreditAttribution: stefan.r commentedJust quoting what @plach said in the parent issue to provide some context on what we want here:
@dawehner do you agree with this approach? It's just the edit and delete operations that we need to address, I don't think there are any other ones?
Comment #3
plachIf we are ok with the proposed plan I can work on this.
Comment #4
catchHow do you delete the actual entity if the delete links are for translations?
What happens if you delete the source translation?
Comment #5
stefan.r CreditAttribution: stefan.r commentedGuessing for now we can't do this, in the parent issue there was talk of being able to delete the source translation without deleting the entity as a non-critical followup outside of the scope of this issue and the parent. In such case we'd probably have to ask the user which language to reassign the node to.
Comment #6
dawehnerI would have expected to have 2 operations available: "delete %entity" and "delete %entity translation in %language",
but I see the point that the user might thing, delete points to deleting the translation for itself.
For the bulk operation case we already will need to ask the user directly, whether to remove the entity or the translation for the entities which will be removed,
see
\Drupal\node\Form\DeleteMultiple
. Given that I guess we want to have the exact same behaviour for the delete route as well.Given that translations are a thing which exists outside of the context of content_translation (they are part of entity API),
I think adapting the generic form, is one way forward.
About which plugin do we talk about?
\Drupal\views\Plugin\views\field\EntityOperations
?The code which adds the entity operation links is the following in
\Drupal\Core\Entity\EntityListBuilder::getDefaultOperations
Can't we use the alter hook available above?
$this->moduleHandler->alter('entity_operation', $operations, $entity);
in order to change the existing delete url info object?
Comment #7
amateescu CreditAttribution: amateescu for Drupal Association commentedThis patch fixes the entity operations handler to be translation aware, but, like #2473989: Lack of dynamic language field / filter makes shipping core views hard to be both compatible with mono and multilingual, it depends on #2428103: String formatter should link to its translation. which fixes
$entity->urlInfo()
to return the correct url.As proposed in #2473989-40: Lack of dynamic language field / filter makes shipping core views hard to be both compatible with mono and multilingual, this is the actual critical issue and that one should be demoted to major.
Comment #8
webchickComment #9
Gábor HojtsyI think ideally operations and bulk operations should either work in an entity level mode or a translation level mode (configured on the view?!). Exposing both entity and translation level operations in the dropdown sounds like a bit too much. You would have two of each actions? Also if you filter a view to original languages of entities only, you would only get one item per entity and then what should be in the operations? We should either let the view configure what level the operations should affect or make it configurable AND exposable. I can easily imagine use cases where you exclusively want entity level operations only (where the page makes it clear you are acting on entities) and where you only want translation level operations.
@catch: When you delete the original translation of an entity, the entity API should have provisions already to pick another original language for that entity I believe.
Comment #10
dawehnerAdded a subissue for the bulk operations side of the problem space #2484037: Make Views bulk operations entity translation aware
Comment #11
plachI'm really not sure about that, at least I don't recall ever coding such logic.
Posting a couple of chat logs for future reference.
Comment #12
webchickUpdating issue summary to make it clear that this is blocked on #2428103: String formatter should link to its translation..
Comment #13
Gábor Hojtsy@plach: oh, what happens now if you delete the original language translation of something then? You get refused with an exception?
Comment #14
amateescu CreditAttribution: amateescu for Drupal Association commentedNope, the entity and all its translations are deleted :/
Comment #15
stefan.r CreditAttribution: stefan.r commentedAnd to make this even more confusing, depending on which part of the user interface you use, in some places the button/option is hidden on the source language, in other places it deletes the whole entity (even if you only clicked the "Delete (this translation)" button), and in other places it throws an error, if I remember correctly.
I couldn't find any code that would allow us to pick another default language for an entity,
ContentEntityBase::removeTranslation()
explicitly disallows this as well:@dawehner I guess we can avoid that by using different verbiage?
Comment #16
stefan.r CreditAttribution: stefan.r commented@gabor:
Yes that would make sense. Or alternatively, just to get back to what was discussed in #2473989: Lack of dynamic language field / filter makes shipping core views hard to be both compatible with mono and multilingual: As "operations" is just edit and delete, and edit is always one language at a time we can just have 3 operations on the UI level (once we allow people to delete separate translations without deleting the whole entity):
edit (french) / delete (french) / delete (all languages)
...where it doesn't matter if the view is filtered by language or not.
Similarly, for bulk operations we could have a dropdown with:
Or even two dependent dropdowns where the leftmost one is a list of bulk operations and the rightmost one says "all languages", and optionally "french" (if we're filtering by that and translation-level operations are available for the selected bulk operation on the left).
And in case of the deletion of a an original language version, in the confirmation screen we can ask the user for every individual affected entity what the new default language should be (with dropdowns or radios).
Comment #17
dawehnerI had the hope that we could make them more intelligent so depending on the context (source language vs. non source language) do a different action.
Comment #18
stefan.r CreditAttribution: stefan.r commentedThe assumption here was that it shouldn't matter whether something is in a source language or not.
Can you clarify with an example?
Comment #19
dawehnerWell, you go to admin/content and see both the source and the translatations, so if you delete for example the translation it should just remove the translation. This is what I think about contextual different behaviour of the links. Similar could be applied to publish/unpublish, but IMHO this is not a critical issue, or at least less of a.
Comment #20
stefan.r CreditAttribution: stefan.r commented@dawehner I think we're actually on the same page here and I may just not have explained correctly. I was talking outside of the current possibilities of Drupal and outside of just the scope of this issue. Basically my point was to let this behave like Drupal 7, and for every operation / bulk operation have the option to apply them on either on the translation level, to all different translations and the source translation, or on the entity level. In the UI, for the latter 2 options all we would say is "all languages".
In such case it really shouldn't matter whether we're looking at the source translation for an entity or a translated version, as the behavior will be the same regardless, and there would be no need for the list of operations to be different.
So even if we have a language filter with the following options:
And we filter by "french", and we select 3 french rows (where 1 is a french non-source translation and 2 are french source translations), we'd still want to have a bulk operation that allows for unpublishing everything, ie. the 3 selected french rows and all their translations, as well as having a bulk operation that unpublishes just the selected versions. Similarly for deletion.
@Gabor's point seemed to be that the list of bulk operations would get too long, but then we could still split the dropdown into 2 dropdowns, 1 for the name of the bulk operation and 1 for whether it applies to just the language we're filtering on or to all languages.
Comment #21
webchickDowngrading, per #2473989-51: Lack of dynamic language field / filter makes shipping core views hard to be both compatible with mono and multilingual. This is no longer in the critical path once #2485159: Switch admin/content from showing one translation per row to one node per row is fixed.
Comment #22
amateescu CreditAttribution: amateescu for Drupal Association commentedI was working on finishing this before it got demoted, so here's a patch that fixes the "criticalness" of this issue: deleting an entity translation results in deleting the whole entity. By the way, this happens even in normal site operation (e.g. going to the delete form of an entity translation), not only through views, so maybe we should bump it back since it's still an unexpected data loss problem?
Still depends on #2428103: String formatter should link to its translation. so leaving postponed for the moment.
Comment #23
stefan.r CreditAttribution: stefan.r commentedWe discussed this on IRC and that interdiff in #22 should get its own critical issue :)
Comment #24
plachUpdated the IS summary with the proposed solution for this particular issue I discussed with @amateescu and @dawehner in IRC.
Can we move the discussion about bulk operations to #2484037: Make Views bulk operations entity translation aware? I agree that the UX implied by both issues should be consistent, but it's a bit confusing that we are mainly discussing the other issue here.
Comment #25
plachComment #26
plachComment #27
stefan.r CreditAttribution: stefan.r commentedFiled a followup to discuss removal of source translations in #2485499: Allow source translations to be removed
Comment #28
plachThanks :)
Comment #29
webchick#2428103: String formatter should link to its translation. got committed, this should be unblocked now.
Comment #30
webchickComment #31
webchickSigh. One more. Sorry for the noise. :(
Comment #34
amateescu CreditAttribution: amateescu for Drupal Association commentedFiled the critical data loss issue: #2486177: Deleting an entity translation from the UI deletes the whole entity
Comment #35
webchickWoohoo #2486177: Deleting an entity translation from the UI deletes the whole entity is in. :) Would still love to see this one fixed, though. It's pretty nasty.
Comment #36
amateescu CreditAttribution: amateescu as a volunteer commentedI wrote this patch a few days ago but I didn't get to post it due to high fever and stuff :( It works but it needs tests, so if anyone wants to work on it, please do.
Edit: (cont.) because I won't be able to do much for a few more days.
Comment #37
webchickOh no. :( Feel better!!
Comment #39
Gábor Hojtsy#2504663: Entity operations links tied to original entity language, ignore both views row and UI languages is a duplicate, so trying to integrate progress from there here.
1. Add missing phpdoc to cosntructor.
2. Use the methods on the trait for less code :)
3. Keep comment on why no call to parent::query().
Comment #40
Gábor HojtsyThe problem also spans way beyond the views operations. It applies to all config entities. I opened #2504663: Entity operations links tied to original entity language, ignore both views row and UI languages based on what @webchick found in the listing of views config entities. Since that is considered a duplicate, integrating those changes here too.
My idea for that is basically all config entity operations need to ignore the language of the entity. We can either fix this directly in the entity URL methods (making them behave differently for config entites) OR each getDefaultOperations() method (which is what I did). This solves the problem of "I'm looking at my menus admin page in Spanish but actually going to edit a menu appears in English, French, etc. base on the menu itself and not my UI language".
Comment #43
Gábor HojtsyDramatically updated issue summary. Hopefully brought over all concerns that were originally there.
Comment #44
Gábor HojtsyFixing all the tests.
Comment #45
amateescu CreditAttribution: amateescu as a volunteer commentedThanks, Gábor!
Given that users are interacting with config entities in a different way than with content entities (i.e. admin interface for config vs. user-facing interface for content), I think it's ok to hardcode a different behavior in the config entity URL methods :)
Comment #46
Gábor Hojtsy@amateescu: that would mean a direct change in ConfigEntityBase::urlInfo(). Here you go.
Comment #48
Gábor HojtsyUpdating test expectations then.
Comment #49
Gábor HojtsyTest for the config entity changes and test only version for them.
Comment #51
Gábor HojtsyExpanding on FieldEntityOperationsTest with language variance, no entity translations yet.
Comment #53
Gábor HojtsyAdding Spanish translations to all entities in the test. Now this will not yet work of course because the view works off of the entity_test table, not entity_test_data. But if I try to make it work off of the data table, there is of course no data table. But if I make the EntityTest entity define a data table, its views data altering code gets confused, etc.
So not sure how best to make this work without making a round of unrelated changes for entity_test :( We can make this test/view use nodes instead?
Comment #54
Gábor HojtsyLet's test that too.
Comment #56
jhodgdonWe need to revive this issue.
Comment #57
Gábor HojtsyI agree this should be revived but I was unsuccessful so far to find someone to help refocus the tests. Removing from sprint as a consequence.
Comment #58
penyaskitoRerolling first.
Comment #61
fran seva CreditAttribution: fran seva at Bluespark commentedI'm working on test
Comment #62
penyaskitoSorry, I had work in progress which I didn't uploaded as I intended to finish this today.
This replace the view to use a node. There are still some issues with destination which you, Fran, can try to fix.
Comment #64
fran seva CreditAttribution: fran seva at Bluespark commentedok @penyaskito. I'm going to try to fix it.
Comment #66
isntall CreditAttribution: isntall at Drupal Association commentedThe patch in comment #62 has been doing odd things on the qa.d.o testbots and has been cancelled.
Comment #67
penyaskitoYeah, sorry, my verbosity was too high and the testbot wasnt happy with it.
I'm back to work on this during the extended sprints.
Comment #68
penyaskitoNow should be ready :D
Comment #70
Gábor HojtsyArticle
Comment #72
Gábor HojtsyTalking to @penyaskito, looks like we need to substring-remove the base path from the URL or something along those lines before checking.
Comment #73
penyaskitoLet's remove the base path then. Thanks @webflo for assisting with permissions stuff.
Comment #74
Gábor HojtsyYay, thanks for the updated tests! Lets get this in :)
Comment #75
Gábor HojtsyComment #76
catchappear in?
Aren't we adding something here now?
Why would the test run with two different profiles?
Creating the language doesn't rebuild the container?
Missing space.
Comment #77
penyaskito1. Fixed.
2. Right, we don't call parent::query but we are relying on TranslationLanguageRenderer. I'm not sure how to make this comment useful though.
3. Removed the check for profile.
4. Adding a language with the API doesn't rebuild the container, we need to do it.
5. Fixed.
Needs work still for 2.
Comment #78
jhodgdonAh, I hear someone calling my name "We need a better comment, Jennifer!" :)
So my suggestion would be to write something like:
Do not call parent::query(), because we do not want the default query behavior for Views fields. Instead, let the entity translation renderer provide the correct query behavior.
Comment #79
penyaskitoThanks Jennifer!
Comment #80
jhodgdonOK. This patch was RTBC in #75, marked needs work in #76, and I believe all the concerns of @catch were addressed, so back to RTBC on that basis.
Comment #81
Gábor HojtsyYay, let's get this in :)
Comment #82
webchickBack to catch.
Comment #84
jhodgdonLooks like a server problem on the test bot, sigh.
Comment #86
Gábor HojtsyShould get in before RC, its pretty bad.
Comment #87
catchCommitted/pushed to 8.0.x, thanks!
Comment #89
Gábor HojtsyYay, amazing! Thanks everyone for making this happen :)