Updated: Comment #0

Problem/Motivation

Postponed on #2095289: Make getEditableConfigNames() public and use it in config_translation

In #2095289: Make getEditableConfigNames() public and use it in config_translation a method will be added to return the config names that are associated with a form.

Proposed resolution

Take out the names from the config_translation yml and use the new method. (Make a patch here we can test when/while the core issue has a patch.)

Remaining tasks

Wait for an initial patch in the core issue.

User interface changes

No.

API changes

Uh..

Files: 
CommentFileSizeAuthor
#21 2095291-21-do-not-test.patch65.16 KBtstoeckler
#20 2095291-19-do-not-test.patch52.38 KBtstoeckler
#14 2095291-config-form-14.patch11.57 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 2 pass(es), 3 fail(s), and 0 exception(s). View
#14 2095291-diff-6-14.txt2.08 KBvijaycs85
#6 2095291-config-form-6.patch10.97 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2095291-config-form-6.patch. Unable to apply patch. See the log in the details link for more information. View
#4 2095291-4-config-forms.patch10.95 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2095291-4-config-forms.patch. Unable to apply patch. See the log in the details link for more information. View
#4 2095291-2-4-interdiff.txt8.29 KBtstoeckler
#2 2095291-1-config-forms.patch8.33 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] 392 pass(es), 54 fail(s), and 1 exception(s). View
#2 system-routing.txt1.35 KBtstoeckler
#2 user-routing.txt570 byteststoeckler

Comments

tstoeckler’s picture

tstoeckler’s picture

Status: Postponed » Needs review
FileSize
570 bytes
1.35 KB
8.33 KB
FAILED: [[SimpleTest]]: [MySQL] 392 pass(es), 54 fail(s), and 1 exception(s). View

Here we go.

This basically does the first half of the issue: In case the routes have a 'config_names' default set, it picks that up and generates translate tabs for the forms. It doesn't yet actually set that key automatically. To test this I've provided two patch files which you can use (with -p3) in your active config directory to generate the proper route information. Don't forget to clear caches.

In case these testing instructions aren't clear, please ping me in IRC, I've dug my head in this for a while so for me everything is crystal clear :-)

I have a very long comment in the patch which I would very much appreciate feedback on. Thanks!

Status: Needs review » Needs work

The last submitted patch, 2095291-1-config-forms.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
8.29 KB
10.95 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2095291-4-config-forms.patch. Unable to apply patch. See the log in the details link for more information. View

Here we go. This now actually works. You can try it out in conjunction with #2095289: Make getEditableConfigNames() public and use it in config_translation. It will until that is in, however.

Status: Needs review » Needs work

The last submitted patch, 2095291-4-config-forms.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
10.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2095291-config-form-6.patch. Unable to apply patch. See the log in the details link for more information. View

Re-rolling...

Status: Needs review » Needs work

The last submitted patch, 2095291-config-form-6.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review

Just marking this "Needs review" again, because it *really* needs reviews :-)

Gábor Hojtsy’s picture

+++ b/lib/Drupal/config_translation/ConfigMapperManager.php
@@ -156,6 +164,77 @@ class ConfigMapperManager extends DefaultPluginManager {
+    $routes = $this->connection->query('SELECT name, route FROM {router}')->fetchAllKeyed();
+    array_walk($routes, function (&$route) { $route = unserialize($route); });

Is querying the route table the best way available to get this info? It looks to me very hackish to do. Is there no API to get this info?

Gábor Hojtsy’s picture

#6: 2095291-config-form-6.patch queued for re-testing.

tstoeckler’s picture

Well, if you look at \Drupal\Core\Routing\RouteProvider it has stuff like getRouteByNames and such, but it doesn't have a getAll() method or similar. I wanted to open an issue for that, but haven't yet (as noted by the in-code @todo). I agree that it's hacky, but I don't think we should make this a blocker, personally

Status: Needs review » Needs work

The last submitted patch, 2095291-config-form-6.patch, failed testing.

Gábor Hojtsy’s picture

Well, I'm not sure it would be committed like this :/

vijaycs85’s picture

FileSize
2.08 KB
11.57 KB
FAILED: [[SimpleTest]]: [MySQL] 2 pass(es), 3 fail(s), and 0 exception(s). View

Ok, added an issue to core to add getAllRoutes method at #2098197: Add getAllRoutes() method to RouteProvider

We got two options:
1. We can wait for core patch to get committed.
2. We can use another callback method for now with yaml discovery.

Attaching patch for 2 (with a re-roll).

vijaycs85’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2095291-config-form-14.patch, failed testing.

tstoeckler’s picture

I'm not sure that's gonna fly, because that would not pick up routes that were added by RouteSubscribers...

tstoeckler’s picture

A quick update here, for anyone following along at home.

In #2098197: Add getAllRoutes() method to RouteProvider the current idea of using the RouteProvider to fetch all routes inside the plugin manager was shot down, because we don't want to load all routes into memory. Instead we will use our RouteSubscriber to react on the route 'alter' event, which gets called batch-wise for a number of routes at a time. There we will check each route if its a translatable form and add the appropriate translation route directly. This gets rid of some indirection, but since we are not using the plugin manager at all then, also causes some other fun:
* To be able to generate the local tasks, we need to find all config translation form routes. We therefore need a method on RouteProvider that fetches all routes that begin with a certain name, in our "config_translation.item."
* All the nice magic we already have for config forms is obsolete with this approach. I *think* we can get the same done - and perhaps more - by checking the routes for an "_entity_form" parameter and then checking whether the entity type is a config entity. I haven't gotten that far yet.

The basic config form stuff works pretty much locally, but I'm not quite able to post a reviewable patch. I'll post one before starting with the config entity stuff. In the meantime, to keep the patch size down over here, I posted the following issues:
* #2111087: Make the translation add and edit forms separate classes
* #2111013: Site information is displayed in the wrong language on the site information translation page
* #2110491: Modernize and clean-up controller and forms
I haven't yet opened an issue for the "RouteProvider::getRoutesByNamePrefix" thing (or whatever...).

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-config

I think it would be important at this point to regroup with @alexpott and @effulgentsia and confirm we are not way over scope here :) I'm not convinced they envisioned such a big set of changes, but they may have :) It would be important for us to focus working on things that have good chances of getting in. I'll write them an email to discuss.

tstoeckler’s picture

FileSize
52.38 KB

Right, I'm uploading an interim patch for that purpose. This is totally not ready for nitpicky final review, but it shows the general approach. Especially the RouteSubscriber and the ConfigTranslationLocalTasks classes are more or less finished, and are the ones that could use some peer review the most.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
65.16 KB

Here we go. The forms part works completely, as far as my testing went. I did not touch the config entity stuff, which means that is definitely broken with this patch.

Since I have worked incrementally on this patch, I'm now going to self-review it in the bigger picture and also try to split of some more parts into their own patches.

Then I'm going to work on the RouteProvider issue.

tstoeckler’s picture

Status: Needs review » Postponed

So this is postponed now until after config_translation.module is in core.
That's a good thing, as this would still take quite some to figure out I fear.

tstoeckler’s picture

Gábor Hojtsy’s picture

Issue tags: +Prague Hard Problems

Adding Prague Hard Problems tag since this was discussed there, BUT this is not considered to be a core blocker anymore.

tstoeckler’s picture

Status: Needs review » Postponed
Issue tags: -Prague Hard Problems

Oops, just noticed I accidentally changed status in #23. This is still postponed.

Gábor Hojtsy’s picture

Issue tags: +Prague Hard Problems

Tag monster.

Gábor Hojtsy’s picture

Title: Remove the config name mapping part of config_translation.config_translation.yml for config forms, use Forms getConfigNames() » Remove *.config_translation.yml for config forms, use form getConfigNames()
Project: (Obsolete) configuration translation for Drupal 8 » Drupal core
Version: 8.x-1.x-dev » 8.x-dev
Component: Code » config_translation.module
Issue summary: View changes

Moving to core now. Still not sure about the feasibility of this. The config_translation.yml API is *very* simple and we only need it a tiny bit :)

mgifford’s picture

Status: Postponed » Active

I think all the issues are fixed now.

tstoeckler’s picture

Status: Active » Postponed
mgifford’s picture

Thanks.. I guess I was looking at the child/related issues.

Gábor Hojtsy’s picture

#2095289: Make getEditableConfigNames() public and use it in config_translation was marked as duplicate of #2392319: Config objects (but not config entities) should by default be immutable which introduces a getEditableConfigNames(). So this would be postponed on that one for now.

YesCT’s picture

Issue summary: View changes
YesCT’s picture

Issue summary: View changes

well... #2095289: Make getEditableConfigNames() public and use it in config_translation was un-duplicated and active, so this is postponed on that now.

tstoeckler’s picture

Status: Postponed » Closed (duplicate)

Right. Since config_translation is now in core and we generally avoid introducing code without any usage I will fix this as part of #2095289: Make getEditableConfigNames() public and use it in config_translation, so marking duplicate.