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.

Files: 
CommentFileSizeAuthor
#1 1894596-1_translation-entity-breaks-user-admin-settings_test.patch579 byteststoeckler
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 1894596-1_translation-entity-breaks-user-admin-settings_test.patch. Unable to apply patch. See the log in the details link for more information. View
#5 patch-1894596.diff316 bytesUccio
PASSED: [[SimpleTest]]: [MySQL] 52,580 pass(es). View
#7 1894596-wip-do-not-test.patch5.84 KBtstoeckler
#25 1894596-25-TEST-ONLY.patch568 bytesgoogletorp
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,562 pass(es), 1 fail(s), and 0 exception(s). View
#25 1894596-25.patch1.1 KBgoogletorp
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,535 pass(es), 19 fail(s), and 4 exception(s). View
#32 1894596-32-TEST-ONLY.patch569 bytesgoogletorp
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,838 pass(es). View
#32 1894596-32.patch1.1 KBgoogletorp
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,800 pass(es), 18 fail(s), and 4 exception(s). View
#37 1894596-37.patch2.05 KBswentel
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,566 pass(es). View
#37 1894596-37-test-only.patch1006 bytesswentel
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,558 pass(es), 2 fail(s), and 0 exception(s). View

Comments

tstoeckler’s picture

Status: Active » Needs review
FileSize
579 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 1894596-1_translation-entity-breaks-user-admin-settings_test.patch. Unable to apply patch. See the log in the details link for more information. View

Here 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.

Status: Needs review » Needs work
tstoeckler’s picture

Status: Needs work » Active

As 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:

Value 'ei;oN\\nnY5' is equal to value 'Anonymous'.

Let's decide what we want to do here.

Uccio’s picture

Assigned: Unassigned » Uccio
Uccio’s picture

Status: Active » Needs review
FileSize
316 bytes
PASSED: [[SimpleTest]]: [MySQL] 52,580 pass(es). View

This patch fix the problem of lost data in form submit

tstoeckler’s picture

#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.

tstoeckler’s picture

Status: Needs review » Needs work
FileSize
5.84 KB

Here'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.

plach’s picture

The 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.

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-content

Tagging.

Gábor Hojtsy’s picture

Title: translation_entity_enable_widget() breaks user settings form » content_translation_enable_widget() breaks user settings form
Component: translation_entity.module » content_translation.module
jair’s picture

Status: Needs work » Needs review
Issue tags: +Needs reroll
tim.plunkett’s picture

Status: Needs review » Needs work
shnark’s picture

Assigned: Uccio » Unassigned

I'm going to try to do a reroll.

BMDan’s picture

Issue tags: -Needs reroll

@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).

Berdir’s picture

Bump, I think I saw a recent issue claiming that this was fixed?

Gábor Hojtsy’s picture

@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.

maijs’s picture

This issue still persists. It was brought up in #2355053: ConfigFormBase::buildForm() is missing a default submit handler (with different solution, though) as well.

omar’s picture

Just 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.

Gábor Hojtsy’s picture

#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? :)

plach’s picture

Issue tags: +sprint

I guess you meant this :)

aspilicious’s picture

This 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.

Gábor Hojtsy’s picture

Yeah :/

googletorp’s picture

Status: Needs work » Needs review
FileSize
561 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,517 pass(es), 18 fail(s), and 4 exception(s). View

So 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.

googletorp’s picture

FileSize
568 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,562 pass(es), 1 fail(s), and 0 exception(s). View
1.1 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,535 pass(es), 19 fail(s), and 4 exception(s). View

Redid patch with tests

The last submitted patch, 23: 1894596-23.patch, failed testing.

The last submitted patch, 25: 1894596-25-TEST-ONLY.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 25: 1894596-25.patch, failed testing.

Status: Needs work » Needs review

googletorp queued 25: 1894596-25.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 25: 1894596-25.patch, failed testing.

googletorp’s picture

FileSize
569 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,838 pass(es). View
1.1 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,800 pass(es), 18 fail(s), and 4 exception(s). View

Fixed the name of the content_translation module from old patch.

giancarlosotelo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 32: 1894596-32.patch, failed testing.

The last submitted patch, 32: 1894596-32.patch, failed testing.

Gábor Hojtsy’s picture

@googletorp that the test only patch passes and the fix fails proves we are not yet there :/

swentel’s picture

Status: Needs work » Needs review
FileSize
1006 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,558 pass(es), 2 fail(s), and 0 exception(s). View
2.05 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,566 pass(es). View

More practical approach. testAccountLanguageSettingsUI() works fine, let's see if the rest works as well (I assume so though).

The last submitted patch, 37: 1894596-37-test-only.patch, failed testing.

The last submitted patch, 37: 1894596-37-test-only.patch, failed testing.

Gábor Hojtsy’s picture

Title: content_translation_enable_widget() breaks user settings form » Account settings cannot be saved anymore if content translation module is installed
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Looks like the most practical approach yeah. Also updated issue summary.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 68e23f1 and pushed to 8.0.x. Thanks!

  • alexpott committed 68e23f1 on 8.0.x
    Issue #1894596 by googletorp, tstoeckler, swentel, Uccio, Gábor Hojtsy:...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay!

aspilicious’s picture

Thank you @swentel :)

googletorp’s picture

Hmm, 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?

Leksat’s picture

@googletorp, an alternative solution was proposed in 2534556#3, but I guess it's too late :)

Gábor Hojtsy’s picture

Well, 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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.