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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jan.stoeckler created an issue. See original summary.

jan.stoeckler’s picture

Issue tags: +language-config
jan.stoeckler’s picture

Status: Active » Needs review
FileSize
709 bytes

Let's try this.

Status: Needs review » Needs work

The last submitted patch, 3: 2546212-3-config_names_wrapper_has_translatable.patch, failed testing.

jan.stoeckler’s picture

Status: Needs work » Needs review
FileSize
705 bytes

Sorry, noob here :-)

Status: Needs review » Needs work

The last submitted patch, 5: 2546212-4-config_names_wrapper_has_translatable.patch, failed testing.

jan.stoeckler’s picture

Status: Needs work » Needs review
FileSize
680 bytes

...and again.

jan.stoeckler’s picture

Status: Needs review » Needs work

The last submitted patch, 7: 2546212-5-config_names_wrapper_has_translatable.patch, failed testing.

jan.stoeckler’s picture

Add a $mapper->populateFromRequest(\Drupal::request()); to ConfigTranslationOverviewAccess::access because without it, $this->getConfigNames() in ConfigNamesMapper::hasTranslatable is (always?) empty.

jan.stoeckler’s picture

Status: Needs work » Needs review

Status change.

Status: Needs review » Needs work

The last submitted patch, 10: 2546212-10-config_names_wrapper_has_translatable.patch, failed testing.

jan.stoeckler’s picture

Status: Needs work » Needs review
FileSize
1.57 KB

Another attempt.

The last submitted patch, 10: 2546212-10-config_names_wrapper_has_translatable.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 13: 2546212-13-config_names_wrapper_has_translatable.patch, failed testing.

The last submitted patch, 3: 2546212-3-config_names_wrapper_has_translatable.patch, failed testing.

The last submitted patch, 5: 2546212-4-config_names_wrapper_has_translatable.patch, failed testing.

The last submitted patch, 7: 2546212-5-config_names_wrapper_has_translatable.patch, failed testing.

tstoeckler’s picture

Title: ConfigNamesMapper::hasTranslatable has flawed logic » [PP-2] ConfigNamesMapper::hasTranslatable has flawed logic
Priority: Normal » Major

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

tstoeckler’s picture

Title: [PP-2] ConfigNamesMapper::hasTranslatable has flawed logic » [PP-1] ConfigNamesMapper::hasTranslatable has flawed logic
robbertnl’s picture

I 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() ?

maxocub’s picture

Status: Needs work » Needs review
FileSize
21.49 KB

Here's a combined patch with the one from #2295255: Convert ConfigMapperInterface::populateFromRequest() to use RouteMatch.

I also:

  • replaced the Route object by the RouteMatchInterface object in the ConfigTranslationOverviewAccess::access and ConfigTranslationFormAccess::access methods
  • replaced $mapper->populateFromRequest(\Drupal::request()) with $mapper->populateFromRouteMatch($route_match)

The previously failing tests now pass on my machine, let's see if the bots agree.

Status: Needs review » Needs work

The last submitted patch, 22: config_names_wrapper_has_translatable-2571655-22.patch, failed testing.

maxocub’s picture

Corrected the unit test, let's see if it pass.
Still trying to figure out why the other one fails...

maxocub’s picture

Status: Needs work » Needs review
tstoeckler’s picture

This looks really great, awesome work @maxocub, especially with the tests.

One minor comment:

+++ b/core/modules/config_translation/src/Access/ConfigTranslationOverviewAccess.php
@@ -54,17 +54,21 @@ public function __construct(ConfigMapperManagerInterface $config_mapper_manager,
+    /** @var \Symfony\Component\Routing\Route $route */
+    $route = $route_match->getRouteObject();

The type hint should not be necessary, as the return value of RouteMatchInterface::getRouteObject() is documented? (I hope :-))

maxocub’s picture

@tstoeckler: You're right, it is documented.

I also corrected the failing test, it was trying to translate a view in the original language.

The last submitted patch, 24: config_names_wrapper_has_translatable-2571655-24.patch, failed testing.

The last submitted patch, 22: config_names_wrapper_has_translatable-2571655-22.patch, failed testing.

The last submitted patch, 24: config_names_wrapper_has_translatable-2571655-24.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Gábor Hojtsy’s picture

Title: [PP-1] ConfigNamesMapper::hasTranslatable has flawed logic » ConfigNamesMapper::hasTranslatable has flawed logic
tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @maxocub!

tstoeckler’s picture

Added a draft change notice.

vijaycs85’s picture

Looks good. +1 for RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 33: config_names_wrapper_has_translatable-2571655-33.patch, failed testing.

maxocub’s picture

Status: Needs work » Needs review

Doesn't looks like it's related.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.67 KB
3.25 KB

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

Status: Needs review » Needs work

The last submitted patch, 41: config_names_wrapper_has_translatable-2571655-41.patch, failed testing.

The last submitted patch, 41: config_names_wrapper_has_translatable-2571655-41.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Reviewed & tested by the community

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

  • effulgentsia committed 6a175c3 on 8.0.x
    Issue #2571655 by jan.stoeckler, maxocub, tstoeckler: ConfigNamesMapper...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

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

Gábor Hojtsy’s picture

Issue tags: -sprint

Great work all, thanks!

Status: Fixed » Closed (fixed)

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

maxocub’s picture

The last submitted patch, 13: 2546212-13-config_names_wrapper_has_translatable.patch, failed testing.

maxocub’s picture