The module uses legacy hook_menu() paths. We should figure out ways to convert to the routing system and see how to still attach to the proper places to expose tabs as needed. This may or may not have complications regarding how to make this work with the mix of new routing based modules vs. old hook_menu based modules. Hopefully no specific issues there though.
See the WSCII conversion guide at http://drupal.org/node/1953342 as well as the http://drupal.org/project/issues/search/drupal?status[]=Open&issue_tags=... tag.
Comment | File | Size | Author |
---|---|---|---|
#22 | 1985880-config-transltion-routing-22.patch | 52.89 KB | vijaycs85 |
#22 | 1985880-diff-20-22.txt | 11.81 KB | vijaycs85 |
#20 | 1985880-config-transltion-routing-20.patch | 52.76 KB | vijaycs85 |
#20 | 1985880-diff-18-20.txt | 1.47 KB | vijaycs85 |
#18 | 1985880-config-transltion-routing-18.patch | 53.29 KB | vijaycs85 |
Comments
Comment #1
vijaycs85Initial patch for overview page.
Comment #2
Gábor HojtsyWoot, passes tests. Although I don't think we have any test coverage for this do we? :D Can we add a 3 line test that checks that this page displays "Site information" or "Account settings" and has a translate link that goes to the right link? We already test the translation pages themselves, so that should be fine.
Such a short test can ensure this works :) I think we should commit that then and then move on to the foreach-ed stuff.
Comment #3
vijaycs85Thanks for the quick review @Gábor Hojtsy. Adding test case.
Comment #4
vijaycs85Comment #5
Gábor HojtsyI'd also assert a link that goes to the right URL for that as said above :) That should complete this page IMHO.
Comment #6
vijaycs85Updating assert for the link.
Comment #7
vijaycs85Changing folder name from controller to Controller.
Comment #8
Gábor HojtsyThe only change I made was to change @inherit to @inheritdoc which I believe is the correct way now. Otherwise committed! :)
Now for the dynamic paths? :) Marking active since no patch to review ATM.
Comment #9
vijaycs85Adding patch for dynamic paths...
TODO:
1. Fix delete form - Need to find a way to get $request inside buildForm.
2. Fix route_name - as of now it is string from Mapper Class.
3. Confirm the apporach of config_translation_get_groups vs configTranslationController::getTranslationGroups() and remove method/function.
Comment #11
vijaycs85oops, forgot to change all reference of subscriber file name change. Adding few routing issue fixes as well.
Comment #13
Gábor HojtsyI'm not very experienced in this field, so pinged this to a few people and tweeted at https://twitter.com/gaborhojtsy/status/331298082148982785
Comment #14
vijaycs85Fixed TODOs mentioned in #11
1. Fix delete form - Need to find a way to get $request inside buildForm. - Now form getting generated from controller.
2. Fix route_name - as of now it is string from Mapper Class. - Added ConfigMapperInterface::getRouteName() to get route_name
3. Confirm the apporach of config_translation_get_groups vs configTranslationController::getTranslationGroups() and remove method/function. - Remove configTranslationController::getTranslationGroups() to use helper function from .module file.
Comment #16
vijaycs85Fixing test case fails.
Comment #18
vijaycs85Fixing remaining 5 fails...
Comment #19
dawehnerIt's great to see that even contrib already starts to port using routes. Well, maybe it's not fair, because the module has been written for d8 :)
Just a short note: you could use a config_translation.services.yml instead.
Comment #20
vijaycs85moving to .services.yml
Comment #21
Gábor HojtsyLooks very good, mostly minor cleanup comments :)
Copy-paste leftovers here :)
This should currently be {@inheritdoc} as elsewhere in the patch.
Make this a ternary? Like
$language = empty($langcode) ? NULL : language_load($langcode);
The new system will only be new for so long :) Let's just say "Router name for this group in the routing system".
The code should decide whether its a router or a route. (My first hunch was that its a route?). The set method says setRouteName, the get method and the property say route*r*.
Document here that the type is the entity type if the mapper is for an entity type, otherwise NULL.
Too many stars there :)
Too many spaces on the first line.
Also how would the return value be mixed? This is returning drupal_get_form() output, that would always be an array, right?
What would this do for non-entity mappers? It looks suspicious. I know it passes tests for the account settings translation stuff, so it should work, but I'm wondering how does this work then? :)
Document when it returns bool and what :)
Comment copy-paste? :)
Not stdClasses, right?
Also the comment on $group in the manageform is incorrect, it is not about deletion :)
Document this :)
{@inheritdoc}
Lack of whitespace between methods.
One line of whitespace is enough :)
@file missing
Definition of => Contains
One too many lines of whitespace :)
Comment #22
vijaycs85Thanks for the review @Gábor Hojtsy.
1. Q:
What would this do for non-entity mappers? It looks suspicious. I know it passes tests for the account settings translation stuff, so it should work, but I'm wondering how does this work then? :)
A: ConfigGroupMapper has default NULL
Comment #23
Gábor HojtsyLooks great, yay! Committed and pushed :)