Closed (duplicate)
Project:
Drupal core
Version:
8.0.x-dev
Component:
config_translation.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
22 Sep 2013 at 15:14 UTC
Updated:
18 Jan 2015 at 18:40 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
tstoecklerMarked #2095837: Automatically discover mappers from config forms as a duplicate.
Comment #2
tstoecklerHere 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!
Comment #4
tstoecklerHere 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.
Comment #6
vijaycs85Re-rolling...
Comment #8
tstoecklerJust marking this "Needs review" again, because it *really* needs reviews :-)
Comment #9
gábor hojtsyIs 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?
Comment #10
gábor hojtsy#6: 2095291-config-form-6.patch queued for re-testing.
Comment #11
tstoecklerWell, 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
Comment #13
gábor hojtsyWell, I'm not sure it would be committed like this :/
Comment #14
vijaycs85Ok, 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).
Comment #15
vijaycs85Comment #17
tstoecklerI'm not sure that's gonna fly, because that would not pick up routes that were added by RouteSubscribers...
Comment #18
tstoecklerA 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...).
Comment #19
gábor hojtsyI 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.
Comment #20
tstoecklerRight, 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.
Comment #21
tstoecklerHere 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.
Comment #22
tstoecklerSo 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.
Comment #23
tstoecklerOpened #2113283: Small clean-ups all around..., #2113503: Editing the source configuration should lead back to the translation overview and #2113701: ConfigTranslationFormBase should implement BaseFormIdInterface in the meantime
Comment #24
tstoecklerOh, and #2113687: RouteProvider should have a method to find all routes whose route names begin with a certain string as well
Comment #25
gábor hojtsyAdding Prague Hard Problems tag since this was discussed there, BUT this is not considered to be a core blocker anymore.
Comment #26
tstoecklerOops, just noticed I accidentally changed status in #23. This is still postponed.
Comment #27
gábor hojtsyTag monster.
Comment #28
gábor hojtsyMoving 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 :)
Comment #29
mgiffordI think all the issues are fixed now.
Comment #30
tstoecklerSadly #2095289: Make getEditableConfigNames() public and use it in config_translation never made it thus far...
Comment #31
mgiffordThanks.. I guess I was looking at the child/related issues.
Comment #32
gábor hojtsy#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.
Comment #33
yesct commentedComment #34
yesct commentedwell... #2095289: Make getEditableConfigNames() public and use it in config_translation was un-duplicated and active, so this is postponed on that now.
Comment #35
tstoecklerRight. 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.