Problem/Motivation
While trying to fix #2546212: Entity view/form mode formatter/widget settings have no translation UI we stumbled upon a problem with the hasTranslatable method in ConfigNamesMapper. When, for example, we have three config objects to be checked in this method, if one of the config objects does not have translatable items, hasTranslatable returns FALSE for all three config items. Furthermore, hasTranslatable Returns TRUE if no config objects are checked.
Proposed resolution
Reverse the logic of the method, returning TRUE if any of the checked config objects have translatable items, analogous to the hasTranslation method.
Remaining tasks
(reviews needed, tests to be written or run, documentation to be written, etc.)
User interface changes
None.
API changes
ConfigNamesWrapper::hasTranslatable will sometimes now return FALSE where they previously returned TRUE and vice versa. Furthermore, ConfigNamesWrapper::hasTranslatable will now return FALSE when there are no config objects to be checked.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#33 | config_names_wrapper_has_translatable-2571655-33.patch | 6.67 KB | maxocub |
Comments
Comment #2
jan.stoecklerComment #3
jan.stoecklerLet's try this.
Comment #5
jan.stoecklerSorry, noob here :-)
Comment #7
jan.stoeckler...and again.
Comment #8
jan.stoecklerComment #10
jan.stoecklerAdd a $mapper->populateFromRequest(\Drupal::request()); to ConfigTranslationOverviewAccess::access because without it, $this->getConfigNames() in ConfigNamesMapper::hasTranslatable is (always?) empty.
Comment #11
jan.stoecklerStatus change.
Comment #13
jan.stoecklerAnother attempt.
Comment #19
tstoecklerPromoting to major, because this blocks a major bug.
This is postponed on #2295255: Convert ConfigMapperInterface::populateFromRequest() to use RouteMatch. We cannot pull in the request in access checkers, we only have access to the route match.
Comment #20
tstoecklerComment #21
robbertnl CreditAttribution: robbertnl at Wowww commentedI was working on this issue today. Didn't get it fixed.
Is it possible that this issue can be caused by the DrupalTestCase which might not have the context for Drupal::request() ?
Comment #22
maxocub CreditAttribution: maxocub commentedHere's a combined patch with the one from #2295255: Convert ConfigMapperInterface::populateFromRequest() to use RouteMatch.
I also:
The previously failing tests now pass on my machine, let's see if the bots agree.
Comment #24
maxocub CreditAttribution: maxocub commentedCorrected the unit test, let's see if it pass.
Still trying to figure out why the other one fails...
Comment #25
maxocub CreditAttribution: maxocub commentedComment #26
tstoecklerThis looks really great, awesome work @maxocub, especially with the tests.
One minor comment:
The type hint should not be necessary, as the return value of RouteMatchInterface::getRouteObject() is documented? (I hope :-))
Comment #27
maxocub CreditAttribution: maxocub commented@tstoeckler: You're right, it is documented.
I also corrected the failing test, it was trying to translate a view in the original language.
Comment #31
Gábor Hojtsy#2295255: Convert ConfigMapperInterface::populateFromRequest() to use RouteMatch landed. We need a non-combined patch :)
Comment #32
Gábor HojtsyComment #33
maxocub CreditAttribution: maxocub commentedNew non-combined patch.
Comment #34
tstoecklerThanks @maxocub!
Comment #35
tstoecklerAdded a draft change notice.
Comment #36
vijaycs85Looks good. +1 for RTBC.
Comment #38
maxocub CreditAttribution: maxocub commentedDoesn't looks like it's related.
Comment #40
tstoecklerComment #41
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI'm confused by why some of the hunks in #33 are part of this issue's scope. So throwing this at bot to see what happens.
Comment #44
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOk, I get all the changes in #33 now. Hiding #41 and re-RTBC'ing #33. Also adding credit to @tstoeckler for multiple reviews and writing the change notice.
Comment #46
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed #33 to 8.0.x and published the CR. drupal.org doesn't seem to let me uncheck my credit checkbox, but I manually removed myself from the commit message that I committed, since the patch I uploaded in #41 wasn't a meaningful contribution.
Comment #47
Gábor HojtsyGreat work all, thanks!
Comment #49
maxocub CreditAttribution: maxocub commentedComment #51
maxocub CreditAttribution: maxocub as a volunteer and commented