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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Status: Active » Needs review
FileSize
6.29 KB

Initial patch for overview page.

Gábor Hojtsy’s picture

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.

vijaycs85’s picture

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

vijaycs85’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

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

vijaycs85’s picture

Updating assert for the link.

vijaycs85’s picture

Changing folder name from controller to Controller.

Gábor Hojtsy’s picture

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.

vijaycs85’s picture

Status: Active » Needs review
FileSize
50.4 KB

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
4.45 KB
51.43 KB

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.

Gábor Hojtsy’s picture

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
12.51 KB
51.47 KB

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
2.28 KB
51.68 KB

Fixing test case fails.

Status: Needs review » Needs work

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
2.53 KB
53.29 KB

Fixing remaining 5 fails...

dawehner’s picture

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
52.76 KB

moving to .services.yml

Gábor Hojtsy’s picture

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
11.81 KB
52.89 KB

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;
  }
Gábor Hojtsy’s picture

Status: Needs review » Fixed

Looks great, yay! Committed and pushed :)

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