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.

Files: 
CommentFileSizeAuthor
#22 1985880-config-transltion-routing-22.patch52.89 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 213 pass(es).
[ View ]
#22 1985880-diff-20-22.txt11.81 KBvijaycs85
#20 1985880-config-transltion-routing-20.patch52.76 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 213 pass(es).
[ View ]
#20 1985880-diff-18-20.txt1.47 KBvijaycs85
#18 1985880-config-transltion-routing-18.patch53.29 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 213 pass(es).
[ View ]
#18 1985880-diff-16-18.txt2.53 KBvijaycs85
#16 1985880-config-transltion-routing-16.patch51.68 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 207 pass(es), 5 fail(s), and 122 exception(s).
[ View ]
#16 1985880-diff-14-16.txt2.28 KBvijaycs85
#14 1985880-config-transltion-routing-14.patch51.47 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 203 pass(es), 10 fail(s), and 6 exception(s).
[ View ]
#14 1985880-diff-11-14.txt12.51 KBvijaycs85
#11 1985880-config-transltion-routing-11.patch51.43 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 206 pass(es), 10 fail(s), and 6 exception(s).
[ View ]
#11 1985880-diff-9-11.txt4.45 KBvijaycs85
#9 1985880-config-transltion-routing-9.patch50.4 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 0 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#7 1985880-config-transltion-routing-7.patch7.38 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 215 pass(es).
[ View ]
#6 1985880-config-transltion-routing-6.patch7.38 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 213 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#6 1985880-diff-3-6.txt873 bytesvijaycs85
#3 1985880-config-transltion-routing-3.patch7.16 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 213 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#3 1985880-diff-1-3.txt886 bytesvijaycs85
#1 1985880-config-transltion-routing-1.patch6.29 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 207 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new6.29 KB
PASSED: [[SimpleTest]]: [MySQL] 207 pass(es).
[ View ]

Initial patch for overview page.

Status:Needs review» Needs work

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

StatusFileSize
new886 bytes
new7.16 KB
FAILED: [[SimpleTest]]: [MySQL] 213 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Thanks for the quick review @Gábor Hojtsy. Adding test case.

Status:Needs work» Needs review

I'd also assert a link that goes to the right URL for that as said above :) That should complete this page IMHO.

StatusFileSize
new873 bytes
new7.38 KB
FAILED: [[SimpleTest]]: [MySQL] 213 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Updating assert for the link.

StatusFileSize
new7.38 KB
PASSED: [[SimpleTest]]: [MySQL] 215 pass(es).
[ View ]

Changing folder name from controller to Controller.

Status:Needs review» Active

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

Status:Active» Needs review
StatusFileSize
new50.4 KB
FAILED: [[SimpleTest]]: [MySQL] 0 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 1985880-config-transltion-routing-9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.45 KB
new51.43 KB
FAILED: [[SimpleTest]]: [MySQL] 206 pass(es), 10 fail(s), and 6 exception(s).
[ View ]

oops, forgot to change all reference of subscriber file name change. Adding few routing issue fixes as well.

Status:Needs review» Needs work

The last submitted patch, 1985880-config-transltion-routing-11.patch, failed testing.

I'm not very experienced in this field, so pinged this to a few people and tweeted at https://twitter.com/gaborhojtsy/status/331298082148982785

Status:Needs work» Needs review
StatusFileSize
new12.51 KB
new51.47 KB
FAILED: [[SimpleTest]]: [MySQL] 203 pass(es), 10 fail(s), and 6 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 1985880-config-transltion-routing-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.28 KB
new51.68 KB
FAILED: [[SimpleTest]]: [MySQL] 207 pass(es), 5 fail(s), and 122 exception(s).
[ View ]

Fixing test case fails.

Status:Needs review» Needs work

The last submitted patch, 1985880-config-transltion-routing-16.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.53 KB
new53.29 KB
PASSED: [[SimpleTest]]: [MySQL] 213 pass(es).
[ View ]

Fixing remaining 5 fails...

Status:Needs review» Needs work

It'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 :)

+++ b/lib/Drupal/config_translation/ConfigTranslationBundle.phpundefined
@@ -0,0 +1,28 @@
+    $container->register('config_translation.subscriber', 'Drupal\config_translation\Routing\RouteSubscriber')
+      ->addTag('event_subscriber');
+
+    $container->register('config_translation.access_check', 'Drupal\config_translation\Access\ConfigNameCheck')
+      ->addTag('access_check');

Just a short note: you could use a config_translation.services.yml instead.

Status:Needs work» Needs review
StatusFileSize
new1.47 KB
new52.76 KB
PASSED: [[SimpleTest]]: [MySQL] 213 pass(es).
[ View ]

moving to .services.yml

Status:Needs review» Needs work

Looks very good, mostly minor cleanup comments :)

+++ b/lib/Drupal/config_translation/Access/ConfigNameCheck.phpundefined
@@ -0,0 +1,56 @@
+
+/**
+ * @file
+ * Contains \Drupal\config_translation\Access\FormatDisableCheck.
+ */
+
...
+
+/**
+ * Checks access for disabling text formats.
+ */

Copy-paste leftovers here :)

+++ b/lib/Drupal/config_translation/Access/ConfigNameCheck.phpundefined
@@ -0,0 +1,56 @@
+  /**
+   * Implements \Drupal\Core\Access\AccessCheckInterface::applies().
+   */
...
+  /**
+   * Implements \Drupal\Core\Access\AccessCheckInterface::access().
+   */

This should currently be {@inheritdoc} as elsewhere in the patch.

+++ b/lib/Drupal/config_translation/Access/ConfigNameCheck.phpundefined
@@ -0,0 +1,56 @@
+    $language = NULL;
+    if ($langcode) {
+      $language = language_load($langcode);
+    }

Make this a ternary? Like

$language = empty($langcode) ? NULL : language_load($langcode);

+++ b/lib/Drupal/config_translation/ConfigEntityMapper.phpundefined
@@ -64,6 +64,14 @@ class ConfigEntityMapper implements ConfigMapperInterface {
+  /**
+   * Router name of the new routing system.
+   *
+   * @var bool
+   */
+++ b/lib/Drupal/config_translation/ConfigGroupMapper.phpundefined
@@ -50,6 +50,13 @@ class ConfigGroupMapper implements ConfigMapperInterface {
+   * Router name of the new routing system.
+   *
+   * @var bool
+   */

The new system will only be new for so long :) Let's just say "Router name for this group in the routing system".

+++ b/lib/Drupal/config_translation/ConfigEntityMapper.phpundefined
@@ -93,6 +101,7 @@ class ConfigEntityMapper implements ConfigMapperInterface {
+    $this->setRouteName();
   }
   /**
@@ -154,4 +163,26 @@ class ConfigEntityMapper implements ConfigMapperInterface {
@@ -154,4 +163,26 @@ class ConfigEntityMapper implements ConfigMapperInterface {
     return new ConfigGroupMapper($base_path, $title, $names, $this->menu_type, $this->add_edit_tab);
   }
+  /**
+   * {@inheritdoc}
+   */
+  public function setRouteName() {
+    $search = array('/', '-', '{', '}');
+    $replace = array('.', '_', '_', '_');
+    $this->router_name = 'config_translation.item.' .   str_replace($search, $replace, $this->base_path);
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getRouterName() {
+    return $this->router_name;
+  }

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

+++ b/lib/Drupal/config_translation/ConfigMapperInterface.phpundefined
@@ -52,4 +52,25 @@ interface ConfigMapperInterface {
+  /**
+   * Helper to provide type of the group.
+   *
+   * @return mixed
+   */

Document here that the type is the entity type if the mapper is for an entity type, otherwise NULL.

+++ b/lib/Drupal/config_translation/Controller/ConfigTranslationController.phpundefined
@@ -71,4 +76,183 @@ class ConfigTranslationController implements ControllerInterface {
+   * Language translations overview page for a configuration name.
+   *
+   ** @param Request $request

Too many stars there :)

+++ b/lib/Drupal/config_translation/Controller/ConfigTranslationController.phpundefined
@@ -71,4 +76,183 @@ class ConfigTranslationController implements ControllerInterface {
+  /**
+   *  Renders translation item manage form.
+   *
+   * @param Request $request
+   *   Page request object.
+   * @param string $action
+   *   Action identifier, either 'add' or 'edit'. Used to provide proper
+   *   labeling on the screen.
+   * @param ConfigMapperInterface $mapper
+   *   Configuration mapper.
+   *
+   * @return array|mixed

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?

+++ b/lib/Drupal/config_translation/Controller/ConfigTranslationController.phpundefined
@@ -71,4 +76,183 @@ class ConfigTranslationController implements ControllerInterface {
+   *
+   * @return ConfigGroupMapper
+   */
+  protected function getConfigGroup(Request $request, ConfigMapperInterface $mapper) {
+    // Get configuration group for this mapper.
+    $entity = $request->attributes->get($mapper->getType());
+    return $mapper->getConfigGroup($entity);

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? :)

+++ b/lib/Drupal/config_translation/Controller/ConfigTranslationController.phpundefined
@@ -71,4 +76,183 @@ class ConfigTranslationController implements ControllerInterface {
+   *
+   * @return bool|\Drupal\core\Language\Language
+   */
+  protected function getLanguage(Request $request, ConfigMapperInterface $mapper) {

Document when it returns bool and what :)

+++ b/lib/Drupal/config_translation/Form/ConfigTranslationDeleteForm.phpundefined
@@ -0,0 +1,91 @@
+/**
+ * Builds a form to delete an action.
+ */
+class ConfigTranslationDeleteForm extends ConfirmFormBase {
+++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.phpundefined
@@ -0,0 +1,236 @@
+/**
+ * Provides a form for configuring an action.
+ */
+class ConfigTranslationManageForm implements FormInterface {

Comment copy-paste? :)

+++ b/lib/Drupal/config_translation/Form/ConfigTranslationDeleteForm.phpundefined
@@ -0,0 +1,91 @@
+  /**
+   * The group of config translation to be deleted.
+   *
+   * @var \stdClass
+   */
+  protected $group;
+
+  /**
+   * The language of config translation.
+   *
+   * @var \stdClass
+   */
+  protected $language;
+++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.phpundefined
@@ -0,0 +1,236 @@
+  /**
+   * The group of config translation to be deleted.
+   *
+   * @var \stdClass
+   */
+  protected $group;
+
+  /**
+   * The language of config translation.
+   *
+   * @var \stdClass
+   */
+  protected $language;

Not stdClasses, right?

Also the comment on $group in the manageform is incorrect, it is not about deletion :)

+++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.phpundefined
@@ -0,0 +1,236 @@
+
+  protected $base_config = array();
+

Document this :)

+++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.phpundefined
@@ -0,0 +1,236 @@
+  /**
+   * Implements \Drupal\Core\Form\FormInterface::getFormID().
+   */
+  public function getFormID() {
+    return 'config_translation_form';
+  }

{@inheritdoc}

+++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.phpundefined
@@ -0,0 +1,236 @@
+  }
+  /**
+   * Build configuration form with metadata and values.
+   */
+  public function buildForm(array $form, array &$form_state, ConfigMapperInterface $group = NULL, Language $language = NULL, array $base_config = NULL) {

Lack of whitespace between methods.

+++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.phpundefined
@@ -0,0 +1,236 @@
+  }
+
+
+  /**

One line of whitespace is enough :)

+++ b/lib/Drupal/config_translation/Routing/RouteSubscriber.phpundefined
@@ -0,0 +1,82 @@
+/**
+ * Definition of \Drupal\config_translation\Routing\RouteSubscriber.
+ */

@file missing

Definition of => Contains

+++ b/lib/Drupal/config_translation/Routing/RouteSubscriber.phpundefined
@@ -0,0 +1,82 @@
+      $collection->add($group->getRouterName(), $route);
+
+
+      $route = new Route($path . '/translate/add/{langcode}', array(

One too many lines of whitespace :)

Status:Needs work» Needs review
StatusFileSize
new11.81 KB
new52.89 KB
PASSED: [[SimpleTest]]: [MySQL] 213 pass(es).
[ View ]

Thanks for the review @Gábor Hojtsy.

1. Q:

+++ b/lib/Drupal/config_translation/Controller/ConfigTranslationController.phpundefined
@@ -71,4 +76,183 @@ class ConfigTranslationController implements ControllerInterface {
+   *
+   * @return ConfigGroupMapper
+   */
+  protected function getConfigGroup(Request $request, ConfigMapperInterface $mapper) {
+    // Get configuration group for this mapper.
+    $entity = $request->attributes->get($mapper->getType());
+    return $mapper->getConfigGroup($entity);

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

  /**
   * Implements \Drupal\config_translation\ConfigMapperInterface::getConfigGroup().
   */
  public function getConfigGroup($arg = NULL) {
    // This is already a config group.
    return $this;
  }

Status:Needs review» Fixed

Looks great, yay! Committed and pushed :)

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