Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
API-level bug
translation_entity_enable_widget() calls a form #process function during form building. This is not only conceptually wrong, it has a real-world effect. The process function, translation_entity_language_configuration_element_process() sets a #submit callback on the top-level $form that is passed in. Because this happens before drupal_prepare_form() runs, the default "{$form_id}_submit" submit handler (i.e. user_admin_settings_submit for the user settings form) is not added to the list of submit handlers. Therefore the actual saving of the form values never takes place.
(Note: user_admin_settings() uses system_config_form() which adds a submit handler on its own, but it takes care of conditionally adding the default #submit handler, in the same way drupal_prepare_form() does, therefore this detail can be ignored for the sake of this issue. It would make more sense for system_config_form() to actually be a system_form_alter() or #process callback of some sort, because then it wouldn't have to duplicate the logic from drupal_prepare_form(), but that is off-topic here.)
Resulting UI-level bug
The user settings form does not work (as in the settings are not saved!) when translation_entity.module is enabled.
Note: Because this only happens when translation_entity.module is enabled, I have not marked this issue "critical". Because the user settings encompass user registration and cancellation settings, e-mail settings, and more, this arguably makes the site unusable for people using content translation. There is a trivial workaround, but it's 0% clear that the user settings get borked by poor translation_entity.module, and I don't think we can expect people to find this issue. Therefore I find "major" to be fitting.
Proposed resolution
Add logic to not override the default #submit handler to translation_entity_language_configuration_element_process(), but add to it instead.
Remaining tasks
Commit.
User interface changes
None.
API changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | 1894596-1_translation-entity-breaks-user-admin-settings_test.patch | 579 bytes | tstoeckler |
| #5 | patch-1894596.diff | 316 bytes | Uccio |
| #7 | 1894596-wip-do-not-test.patch | 5.84 KB | tstoeckler |
| #25 | 1894596-25-TEST-ONLY.patch | 568 bytes | googletorp |
| #25 | 1894596-25.patch | 1.1 KB | googletorp |
Comments
Comment #1
tstoecklerHere is a trivial test to prove that this is a problem. We probably don't want to commit this as part of the fix in the end, though.
For the record, I am for resolution 1. of the issue summary, although I was not really involved with the translation_entity.module, so I do not know why it currently does things the way it does.
Comment #3
tstoecklerAs can be seen from the failed assertion message in the test log, the custom user settings do not get set, which proves the point of the patch. For example the anonymous user name setting:
Let's decide what we want to do here.
Comment #4
Uccio CreditAttribution: Uccio commentedComment #5
Uccio CreditAttribution: Uccio commentedThis patch fix the problem of lost data in form submit
Comment #6
tstoeckler#5 is in fact a solution to the problem, but I think the fact that user module would need to explicitly declare the submit handler which is otherwise implied, would be a DX problem.
I still don't think user_admin_settings() (conditionally) calling into translation_entity functions is a good thing, as is noted by the @todo in that function, but I think we can fix the immediate bug here by solution 1. in the original post. If I get to it, I will code something up in the next few days.
Comment #7
tstoecklerHere's something I was able to cook up. This properly uses a #process callback which IMO is the right approach.
By doing that and using the 'language_configuration' element it adds a couple of options, such as default language. Additionally it fixes the user admin settings form. Because comment module uses the function I removed in this patch as well, but that seemed more complicated to refactor.
Some of the things translation_entity.module does that I touched here seem very much unholy, but I really have too little knowledge about this module to really judge that. I'll ping plach in IRC.
Comment #8
plachThe sole goal of this messy code is being able to reuse it both in the entity-type-specific configuration pages, e.g. in the user account settings or the content type settings, and in the content language settings page. If the solution you are proposing works in both cases, then great :)
I think the test as it is now doesn't make much sense, we should enable ET in every test for every module integrating with ET. I think we might either skip fixing tests altogether as this is a rather edgy use case or provide something more generic relying on the test entity.
Comment #9
Gábor HojtsyTagging.
Comment #10
Gábor HojtsyComment #11
jair CreditAttribution: jair commentedComment #12
tim.plunkettComment #13
shnark CreditAttribution: shnark commentedI'm going to try to do a reroll.
Comment #14
BMDan CreditAttribution: BMDan commented@EllaTheHarpy put quite a bit of work into this, and then @YesCT and I looked through this issue together further.
The previous patches don't apply, but they don't seem relevant to the current codebase, either. In fact, this issue may not even exist any more. Thus, I'm pushing the status to "needs work", and removing the reroll tag per the recommendation of @YesCT, to properly reflect the fact that this patch either needs a rewrite from scratch, or this needs to be closed WONTFIX or dupe (if someone can track down where it was, in fact, fixed).
Comment #15
BerdirBump, I think I saw a recent issue claiming that this was fixed?
Comment #16
Gábor Hojtsy@Berdir: I am not aware. There has been some talk about removing language settings from entity settings admin forms because its painful to need to enable translation on the entity and then visiting the field settings forms one by one and if we'd not have these settings here, people would be lead right away to the master settings page which has everything at once :) That would in itself solve this issue I think :D It may be better UX because it leads you to an effective UX or it may be bad UX because it removes some settings from the entity config itself and moves it to 3rd party screen instead of integrating into the entity setting right away like others do, but these settings are across entity types, bundles and fields / sub-fields, so its not really integratable as-is to one of the screens. Of course from the technical point of view of this issue, that would be a workaround.
Comment #17
maijs CreditAttribution: maijs commentedThis issue still persists. It was brought up in #2355053: ConfigFormBase::buildForm() is missing a default submit handler (with different solution, though) as well.
Comment #18
omar CreditAttribution: omar as a volunteer and commentedJust here to confirm that, as per "Resulting UI-level bug", changes made to the User Settings form still aren't getting saved as of beta10.
Comment #19
Gábor Hojtsy#2534556: Account settings can't be saved if content_translation module enabled was closed as a duplicate. Bring on sprint as a result. Someone wanna take the patch from there and figure out if one here is better or there? :)
Comment #20
plachI guess you meant this :)
Comment #21
aspilicious CreditAttribution: aspilicious commentedThis doesn't brake stuff hard so this isn't critical. But for multilingual D8 this is a serious showstopper if you can't edit the config files with drush.
Comment #22
Gábor HojtsyYeah :/
Comment #23
googletorp CreditAttribution: googletorp as a volunteer commentedSo a simple way of going about this, is to add the standard submit handler (::submitForm) to the primary save button, which will fix the issue of it being disabled when content_translation adds it's submit handler to the primary save button.
Comment #25
googletorp CreditAttribution: googletorp as a volunteer commentedRedid patch with tests
Comment #32
googletorp CreditAttribution: googletorp as a volunteer and at Reveal IT commentedFixed the name of the content_translation module from old patch.
Comment #33
giancarlosotelo CreditAttribution: giancarlosotelo at MD Systems GmbH commentedComment #36
Gábor Hojtsy@googletorp that the test only patch passes and the fix fails proves we are not yet there :/
Comment #37
swentel CreditAttribution: swentel commentedMore practical approach. testAccountLanguageSettingsUI() works fine, let's see if the rest works as well (I assume so though).
Comment #40
Gábor HojtsyLooks like the most practical approach yeah. Also updated issue summary.
Comment #41
alexpottCommitted 68e23f1 and pushed to 8.0.x. Thanks!
Comment #43
Gábor HojtsyYay!
Comment #44
aspilicious CreditAttribution: aspilicious commentedThank you @swentel :)
Comment #45
googletorp CreditAttribution: googletorp as a volunteer and at Reveal IT commentedHmm, couldn't we get strange issues in contrib with this patch. If the form where this is attached has Add another AJAX button or similar unwanted cases? The enable translation feature would be saved at unexpected times. Shouldn't we find a better solution for this?
Comment #46
Leksat CreditAttribution: Leksat at Amazee Labs commented@googletorp, an alternative solution was proposed in 2534556#3, but I guess it's too late :)
Comment #47
Gábor HojtsyWell, its never too late to get better fixes in, please open yet another issue or reopen the other one with the better fix if you think that is better.