Problem/Motivation

Since #2281619: Convert most direct usages within module code of routing related request attributes to use RouteMatchInterface instead and friends usage of the request object to retrieve attributes from it is discouraged in favor of using the RouteMatch object. Because other subsystems have already performed this switch configuration mappers cannot currently be used there. The route access system is one such subsystem: Custom access checks are passed the route match object if they provide an appropriate type-hint, but they cannot access the correct request object. This was exposed in #2571655: ConfigNamesMapper::hasTranslatable has flawed logic, which is now blocked on this issue. That in turn blocks #2546212: [PP-1] Entity view/form mode formatter/widget settings have no translation UI which is a major bug.

Proposed resolution

  1. Instead of a populateFromRequest() provide a populateFromRouteMatch() method
  2. Because we cannot easily set arbitrary data on a RouteMatch, add a setLangcode() method to support the use-case of ConfigTranslationController::itemPage()

Remaining tasks

User interface changes

None.

API changes

  1. ConfigMapperInterface::populateFromRequest() is removed in favor of ::populateFromRouteMatch()
  2. ConfigMapperInterface::setLangcode() is added

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task (This blocks bugs, so it could be said, this makes it a bug)
Issue priority Major because this blocks a major bug
Prioritized changes Prioritized because this is required to fix a major bug
Disruption Disruptive for some modules extending Config Translation's API. The addition of ConfigMapperInterface::setLangcode() is only a break for modules which do not extend ConfigNamesMapper, which should be very, very few in practice. The switch from *Request() to *RouteMatch() is only a break for modules that A) have a dynamic configuration name list, which is not based on configuration entities which is a very far-fetched use-case or for modules or B) manage configuration entities but do not extend ConfigEntityMapper which also should be very, very few in practice. Mappers extending ConfigEntityMapper will override setEntity() instead, so will not notice the change. If this is still considered too disruptive we could leave populateFromRequest() in place and just add populateFromRouteMatch() but due to problem described above, we would be pretending to support an API which we do not actually support in all cases.
Files: 
CommentFileSizeAuthor
#16 convert-2295255-16.patch17.82 KBk4v
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,719 pass(es). View
#13 2295255-12-config-translation-route-match.patch16.61 KBtstoeckler
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,899 pass(es), 1 fail(s), and 3 exception(s). View
#13 2295255-9-12-interdiff.txt3.21 KBtstoeckler
#2 2295255.patch1.04 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,387 pass(es), 93 fail(s), and 0 exception(s). View

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
1.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,387 pass(es), 93 fail(s), and 0 exception(s). View

.

Status: Needs review » Needs work

The last submitted patch, 2: 2295255.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
11.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,290 pass(es), 178 fail(s), and 69 exception(s). View
11.26 KB

Let's work on it.

dawehner’s picture

Forget the interdiff

Status: Needs review » Needs work

The last submitted patch, 4: 2295255-4.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
11.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,773 pass(es), 184 fail(s), and 75 exception(s). View

just a reroll

Status: Needs review » Needs work

The last submitted patch, 7: 2295255-7.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.07 KB
13.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,842 pass(es), 96 fail(s), and 42 exception(s). View

This should fix a couple of those problems.

Status: Needs review » Needs work

The last submitted patch, 9: 2295255-9.patch, failed testing.

tstoeckler’s picture

Assigned: Unassigned » tstoeckler
Category: Task » Bug report
Priority: Normal » Major
tstoeckler’s picture

Status: Needs work » Needs review
Issue tags: -RouteMatch +sprint, +D8MI, +language-config

Rebased and fixed something. Let's see.

tstoeckler’s picture

FileSize
3.21 KB
16.61 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,899 pass(es), 1 fail(s), and 3 exception(s). View

#fail

Status: Needs review » Needs work

The last submitted patch, 13: 2295255-12-config-translation-route-match.patch, failed testing.

The last submitted patch, 13: 2295255-12-config-translation-route-match.patch, failed testing.

k4v’s picture

FileSize
17.82 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,719 pass(es). View
k4v’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

This is apparently blocking another issue, that should be stated in the IS.
Otherwise, this patch is perfect.
Once the BE and IS are in place, I feel confident in marking this RTBC.

tstoeckler’s picture

Thanks a lot @tim.plunkett !

tstoeckler’s picture

Issue summary: View changes
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Wonderful. Let's get it in!

Gábor Hojtsy’s picture

Issue tags: +blocker

Add blocker since this blocks #2546212: [PP-1] Entity view/form mode formatter/widget settings have no translation UI that is many things don't have a config translation UI due to this.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record
+++ b/core/modules/config_translation/src/ConfigEntityMapper.php
@@ -110,9 +110,9 @@ public static function create(ContainerInterface $container, array $configuratio
    */
-  public function populateFromRequest(Request $request) {
-    parent::populateFromRequest($request);
-    $entity = $request->attributes->get($this->entityType);
+  public function populateFromRouteMatch(RouteMatchInterface $route_match) {
+    parent::populateFromRouteMatch($route_match);
+    $entity = $route_match->getParameter($this->entityType);
     $this->setEntity($entity);
   }

Patch looks great, but isn't this an API change that needs a change record?

tstoeckler’s picture

Status: Needs work » Reviewed & tested by the community

Ahh sorry about that. Totally forgot. Wrote one now, back to RTBC.

Gábor Hojtsy’s picture

Issue tags: -Needs change record

The change notice makes sense to me. It does not explain the reasons for the change, but it does not need to. Its in the issue summary. This way its concise and easy to follow what is to be changed.

  • catch committed 5976b79 on 8.0.x
    Issue #2295255 by dawehner, tstoeckler, k4v, kgoel: Convert...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

Gábor Hojtsy’s picture

Status: Fixed » Closed (fixed)

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