Part of #1971384: [META] Convert page callbacks to controllers

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Files: 
CommentFileSizeAuthor
#133 content_translation-add_page-1987882-133.patch35.54 KBlikin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,423 pass(es).
[ View ]
#130 content_translation-add_page-1987882-130.patch34.43 KBlikin
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,274 pass(es), 12 fail(s), and 0 exception(s).
[ View ]
#126 content_translation-add_page-1987882-126.patch39.54 KBlikin
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,665 pass(es), 75 fail(s), and 11 exception(s).
[ View ]
#124 content_translation-add_page-1987882-124.patch31.51 KBlikin
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,704 pass(es), 73 fail(s), and 11 exception(s).
[ View ]

Comments

Assigned:Unassigned» acrollet

I'm working on this issue.

Assigned:acrollet» Unassigned
Status:Active» Needs work
StatusFileSize
new2.79 KB
FAILED: [[SimpleTest]]: [MySQL] 55,746 pass(es), 2 fail(s), and 8 exception(s).
[ View ]

I need to leave town soon and didn't get as far as I would have liked, but I'm uploading some partial work as a patch. Because dynamic routes are generated for entity types with translation enabled, a routing yml file can not be used, and a RouteSubscriber interface has to be implemented. This patch adds the RouteSubscriber service and creates routes for the translation overviews page, but the controller still needs to be implemented.

Status:Needs work» Needs review
StatusFileSize
new18.89 KB
FAILED: [[SimpleTest]]: [MySQL] 55,658 pass(es), 17 fail(s), and 6 exception(s).
[ View ]

Previous patch incorrectly used wildcards in routes. We've added converters to routes definition and moved translation_entity_overview() code to the separate controller class.

Translation overview page (/{entity_type}/{entity_id}/translations) works fine with attached patch.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new22.13 KB
PASSED: [[SimpleTest]]: [MySQL] 57,673 pass(es).
[ View ]

Previous patch incorrectly generated routes (incorrect route path and access rules).
Also we've added new class for control access to entity translation overview page .

Translation overview page works fine with attached patch.
Tested with all entities that exists in core.

Issue tags:+Needs manual testing, +D8MI

Code looks good, just needs manual testing

Status:Needs review» Needs work

Thanks for working on this conversion!

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Access/EntityTranslationOverviewAccess.phpundefined
@@ -0,0 +1,83 @@
+ * Definition of Drupal\translation_entity\Access\EntityTranslationOverviewAccess.
+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationOverviewController.phpundefined
@@ -0,0 +1,230 @@
+ * Definition of Drupal\translation_entity\EntityTranslationOverviewController.

Should be "Contains \"

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Access/EntityTranslationOverviewAccess.phpundefined
--- /dev/null
+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationOverviewController.phpundefined

Controllers should be in the Controller directory.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationOverviewController.phpundefined
@@ -0,0 +1,230 @@
+   * @param EntityManager $manager
+   */
+  public function __construct(EntityManager $manager, ModuleHandlerInterface $module_handler) {

Parameters should be documented and contain the full namespace in the documentation.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationOverviewController.phpundefined
@@ -0,0 +1,230 @@
+    $controller = translation_entity_controller($entity->entityType());

You could just use $this->entityManager->getControllerClass($entity->entityType(), 'translation');

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationOverviewController.phpundefined
@@ -0,0 +1,230 @@
+    $field_ui = module_exists('field_ui') && user_access('administer ' . $entity->entityType() . ' fields');

module_exists can be replaced by $this->moduleHandler->moduleExists()

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -147,12 +147,10 @@ function translation_entity_menu() {
-        'context' => MENU_CONTEXT_PAGE | MENU_CONTEXT_INLINE,

Is there a reason why you remove the context?

Status:Needs work» Needs review
StatusFileSize
new22.61 KB
PASSED: [[SimpleTest]]: [MySQL] 56,049 pass(es).
[ View ]

Thanks for the review!
I've fixed almost all Your notices, but should I really move EntityTranslationOverviewAccess.php from Access to Controller folder? Currently I've kept it in the Access folder. Patch attached.

Files' structure is ok.

StatusFileSize
new37.73 KB
PASSED: [[SimpleTest]]: [MySQL] 55,842 pass(es).
[ View ]

Attached patch also covers #1987878: Convert content_translation_edit_page() to a new style controller and #1987876: Convert content_translation_add_page() to a new style controller issues, but needs some additional work to get rid of the hard dependencies in the code.

I will work with this all in few days.

@InternetDevels - Thanks for working on this! I'm wondering why those other patches got combined here though. I think if possible these patches should be separated into their own issues so they can be reviewed individually.

Status:Needs review» Postponed

Status:Postponed» Needs review

The other one is actually easier to reroll(?)

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Access/EntityTranslationAccessAdd.phpundefined
@@ -0,0 +1,72 @@
+ * Definition of Drupal\translation_entity\Access\EntityTranslationAccessAdd.
+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Access/EntityTranslationAccessEdit.phpundefined
@@ -0,0 +1,70 @@
+ * Definition of Drupal\translation_entity\Access\EntityTranslationAccessEdit.

Should be "Contains \..."

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Access/EntityTranslationAccessAdd.phpundefined
@@ -0,0 +1,72 @@
+class EntityTranslationAccessAdd implements AccessCheckInterface, ControllerInterface {
+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Access/EntityTranslationAccessEdit.phpundefined
@@ -0,0 +1,70 @@
+class EntityTranslationAccessEdit implements AccessCheckInterface, ControllerInterface {

Wouln't it be possible to merge these two together .. .the code looks really similar in most places.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Access/EntityTranslationAccessAdd.phpundefined
@@ -0,0 +1,72 @@
+   * Implements AccessCheckInterface::applies().
...
+   * Implements AccessCheckInterface::access().

This could be just @inheritdoc.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Access/EntityTranslationAccessAdd.phpundefined
@@ -0,0 +1,72 @@
+      return $source->langcode != $target->langcode && isset($languages[$source->langcode])
...
+    return NULL;

Please return static::ALLOW and static::DENY

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Access/EntityTranslationAccessAdd.phpundefined
@@ -0,0 +1,72 @@
+        && translation_entity_access($entity, 'create');
+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Access/EntityTranslationAccessEdit.phpundefined
@@ -0,0 +1,70 @@
+        && translation_entity_access($entity, 'update');

This could be replaced by a controller coming from entityManager. I suggest a method called getControllerInstance($entity_type, $controller_type); which works like every other getAccessController like method.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Access/EntityTranslationAccessEdit.phpundefined
@@ -0,0 +1,70 @@
+   * Implements AccessCheckInterface::applies().
...
+   * Implements AccessCheckInterface::access().

See above.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Access/EntityTranslationAccessEdit.phpundefined
@@ -0,0 +1,70 @@
+      return isset($languages[$language->langcode])
...
+    return NULL;

Same as before.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Controller/EntityTranslationOverviewController.phpundefined
@@ -0,0 +1,291 @@
+   * Initializes an instance of the entity translation overview controller.

Should be better "Constructs a EntityTranslationOverviewController object." (this is the standard.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Controller/EntityTranslationOverviewController.phpundefined
@@ -0,0 +1,291 @@
+    $controller_class = $this->manager->getControllerClass($entity_type, 'translation');
+    $controller = new $controller_class($entity_type, $this->manager->getDefinition($entity_type));

This code then could leverage the new method from above.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Controller/EntityTranslationOverviewController.phpundefined
@@ -0,0 +1,291 @@
+   * @param EntityInterface $entity
...
+   * @param Language $source
...
+   * @param Language $target
...
+   * @param EntityInterface $entity
...
+   * @param Language $language

Type hinting should contain the full namespace as well.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Controller/EntityTranslationOverviewController.phpundefined
@@ -0,0 +1,291 @@
+    return entity_get_form($entity, $operation, $form_state);
...
+    return entity_get_form($entity, $operation, $form_state);

entity_get_form now has a method on the entity manager.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Routing/EntityTranslationRouteSubscriber.phpundefined
@@ -0,0 +1,108 @@
+          array(
+            '_translation_entity_access' => $entity_type,
+            '_permission' => 'translate any entity',
+          ),
...
+          array(
+            '_permission' => 'translate any entity',
+            '_translation_entity_access_add' => $entity_type,
+          ),

Should both apply or just one of them?

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Routing/EntityTranslationRouteSubscriber.phpundefined
@@ -0,0 +1,108 @@
+  }

Here should be a new line, sorry.

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -163,31 +162,20 @@ function translation_entity_menu() {
       $items["$path/translations/add/%language/%language"] = array(
-        'title' => 'Add',
-        'page callback' => 'translation_entity_add_page',
-        'page arguments' => $args,
-        'access callback' => 'translation_entity_add_access',
-        'access arguments' => $args,
-        'type' => MENU_LOCAL_TASK,
-        'weight' => 1,
-      ) + $item;
+        'route_name' => "translation_entity.translation_add.$entity_type",
+        'weight'     => 1,
...
-      $args = array($entity_position, $language_position);
       $items["$path/translations/edit/%language"] = array(
-        'title' => 'Edit',
-        'page callback' => 'translation_entity_edit_page',
-        'page arguments' => $args,
-        'access callback' => 'translation_entity_edit_access',
-        'access arguments' => $args,
-        'type' => MENU_LOCAL_TASK,
-        'weight' => 1,
-      ) + $item;
+        'route_name' => "translation_entity.translation_edit.$entity_type",
+        'weight'     => 1,
+      );

I guess you can't remove the 'title' now? already as well as the type@

StatusFileSize
new35.71 KB
FAILED: [[SimpleTest]]: [MySQL] 57,160 pass(es), 17 fail(s), and 0 exception(s).
[ View ]

Fixed all in #15 except below one:

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Controller/EntityTranslationOverviewController.phpundefined
@@ -0,0 +1,291 @@
+    $controller_class = $this->manager->getControllerClass($entity_type, 'translation');
+    $controller = new $controller_class($entity_type, $this->manager->getDefinition($entity_type));

This code then could leverage the new method from above.

and updated controller & access classes.

StatusFileSize
new22.7 KB

Interdiff after conflict merge....

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new3.56 KB
new36.43 KB
FAILED: [[SimpleTest]]: [MySQL] 55,680 pass(es), 17 fail(s), and 0 exception(s).
[ View ]

As discussed with @dawehner on IRC, updating entityManager to get the controller directly.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new3.01 KB
new36.47 KB
FAILED: [[SimpleTest]]: [MySQL] 58,230 pass(es), 347 fail(s), and 129 exception(s).
[ View ]

Hopefully this patch fixes few test cases...

Status:Needs review» Needs work

Title:Convert translation_entity_overview() to a new style controllerConvert content_translation_overview() to a new style controller
Component:translation_entity.module» content_translation.module

Status:Needs work» Needs review
StatusFileSize
new36.36 KB
FAILED: [[SimpleTest]]: [MySQL] 57,179 pass(es), 322 fail(s), and 217 exception(s).
[ View ]

This is a straight reroll, but a lot of this doesn't make much sense to me.
For one, the RouteSubscriber is written very differently from the others I've seen in core.

Second, the access checker should use StaticAccessCheckInterface instead, and can have injection done via services.yml, I don't think those work with ControllerInterface.

Thirdly, the change to EntityManager::getController() shouldn't be needed, but even if it is, the calls to it make no sense.

Status:Needs review» Needs work
Issue tags:-Needs manual testing, -D8MI, -WSCCI-conversion

The last submitted patch, content_translation-1987882-24.patch, failed testing.

Status:Needs work» Needs review

#24: content_translation-1987882-24.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs manual testing, +D8MI, +WSCCI-conversion

The last submitted patch, content_translation-1987882-24.patch, failed testing.

Issue tags:+Stalking Crell

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Access/ContentTranslationOverviewAccess.phpundefined
@@ -0,0 +1,83 @@
+   * Implements AccessCheckInterface::applies().
...
+   * Implements AccessCheckInterface::access().

{inheritdoc}

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.phpundefined
@@ -0,0 +1,290 @@
+    $field_ui = $this->moduleHandler->moduleExists('field_ui') && user_access('administer ' . $entity_type . ' fields');

$account->hasPermission()

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.phpundefined
@@ -0,0 +1,290 @@
+          $status = '<span class="status">' . $status . '</span>' . (!empty($translation['outdated']) ? ' <span class="marker">' . t('outdated') . '</span>' : '');
...
+            $language_name = t('<strong>@language_name (Original language)</strong>', array('@language_name' => $language_name));

We can use https://drupal.org/node/2019739 now and remove @todo

Also adding to @Crell's list to get a better review.

Status:Needs work» Needs review
Issue tags:-Stalking Crell
StatusFileSize
new4.06 KB
new36.91 KB
FAILED: [[SimpleTest]]: [MySQL] 57,489 pass(es), 322 fail(s), and 216 exception(s).
[ View ]

Updating review changes in #28

Issue tags:+Stalking Crell

restoring tag.

Status:Needs review» Needs work

Thanks @vijaycs85 for fixing the issue one minor thing.

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.phpundefined
@@ -162,8 +168,24 @@ public function overview(EntityInterface $entity) {
-          // @todo Add a theming function here.
-          $status = '<span class="status">' . $status . '</span>' . (!empty($translation['outdated']) ? ' <span class="marker">' . t('outdated') . '</span>' : '');
+
+          $status_tag = array(
+            '#type' => 'html_tag',
+            '#tag' => 'span',
+            '#attributes' => array(
+              'class' => 'status',
+              'content' => $status,
+            )
+          );
+          $status = drupal_render($status_tag);
+
+          if (!empty($translation['outdated'])) {
+            $status_tag['#attributes'] = array (
+              'class' => 'marker',
+              'content' => t('outdated'),
+            );
+            $status .= drupal_render($status_tag);
+          }

Please restore this change. I talked to @tim.plunkett and he explained to me that we are not using '#type' => 'html_tag' anywhere other then <head> in the core so it is not alright to use it here as it is slow and not made for this.

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Access/ContentTranslationManageAccessCheck.phpundefined
@@ -0,0 +1,88 @@
+use Symfony\Component\Routing\Route;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Drupal\Core\Entity\EntityManager;
+use Drupal\Core\Access\AccessCheckInterface;
+use Drupal\Core\Controller\ControllerInterface;
+++ b/core/modules/content_translation/lib/Drupal/content_translation/Access/ContentTranslationOverviewAccess.phpundefined
@@ -0,0 +1,83 @@
+use Symfony\Component\Routing\Route;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Drupal\Core\Entity\EntityManager;
+use Drupal\Core\Access\AccessCheckInterface;
+use Drupal\Core\Controller\ControllerInterface;
+
+++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.phpundefined
@@ -0,0 +1,312 @@
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Drupal\Core\Controller\ControllerInterface;
+use Drupal\Core\Entity\EntityInterface;
+use Drupal\Core\Extension\ModuleHandlerInterface;
+use Drupal\Core\Entity\EntityManager;
+use Drupal\Core\Session\AccountInterface;
+++ b/core/modules/content_translation/lib/Drupal/content_translation/Routing/ContentTranslationRouteSubscriber.phpundefined
@@ -0,0 +1,108 @@
+
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\Routing\Route;
+use Drupal\Core\Routing\RouteBuildEvent;
+use Drupal\Core\Routing\RoutingEvents;
+use Drupal\Core\Entity\EntityManager;

Vendor namespace should be in the end.

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Access/ContentTranslationOverviewAccess.phpundefined
@@ -0,0 +1,83 @@
+        return TRUE;
...
+        return TRUE;
...
+    return NULL;

I think we should use static::ALLOW and static::DENY

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Access/ContentTranslationOverviewAccess.phpundefined
@@ -0,0 +1,83 @@
+      if (user_access($permission)) {

We can get $account from $request and replace user_access with hasPermission

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.phpundefined
@@ -0,0 +1,312 @@
+        $links = _content_translation_get_switch_links($path);

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.phpundefined
@@ -0,0 +1,312 @@
+    content_translation_prepare_translation($entity, $source, $target);
...
+    $controller = content_translation_controller($entity->entityType());

This should be in manger or module file. So that we can remove $this->moduleHandler->loadInclude('content_translation', 'inc', 'content_translation.pages'); then we don't need to inject moduleHandler.

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.phpundefined
@@ -0,0 +1,312 @@
+      foreach (field_info_instances($entity_type, $entity->bundle()) as $instance) {
...
+        $field = field_info_field($field_name);

field.info service injection

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.phpundefined
@@ -0,0 +1,312 @@
+              $path = $entity_path . '/fields';

$path is not required.

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.phpundefined
@@ -0,0 +1,312 @@
+    $this->moduleHandler->loadInclude('content_translation', 'inc', 'content_translation.pages');

Not required here.

The longer we stretch these out the more code changes in the removed/moved code and more potential for error. Is anybody working on this one?

Status:Needs work» Needs review
StatusFileSize
new8.03 KB
new36.95 KB
FAILED: [[SimpleTest]]: [MySQL] 57,730 pass(es), 365 fail(s), and 233 exception(s).
[ View ]

@Gábor Hojtsy, trying to give a try :)

Thanks for the confirmation @jibran, rolled back #32.

On #33:

#33.1 - FIXED: Vendor namespace should be in the end.
#33.2 - FIXED: I think we should use static::ALLOW and static::DENY
#33.3 - FIXED: We can get $account from $request and replace user_access with hasPermission
#33.4 - NOT FIXED: This should be in manger or module file. So that we can remove $this->moduleHandler->loadInclude('content_translation', 'inc', 'content_translation.pages'); then we don't need to inject moduleHandler. - Not sure why would we need to do this here now. Can't we clean up on follow up?
#33.5 - FIXED: field.info service injection
#33.6 - NOT FIXED: $path is not required. - as per #33.4, we aren't fixing it here.
#33.7 - NOT FIXED: Not required here. - as per #33.4, we aren't fixing it here.

Status:Needs review» Needs work
  1. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Access/ContentTranslationManageAccessCheck.php
    @@ -0,0 +1,88 @@
    +    return array_key_exists('_content_translation_manage_access', $route->getRequirements());

    By convention, most access check tags have been _access_*, not *_access.

  2. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Access/ContentTranslationManageAccessCheck.php
    @@ -0,0 +1,88 @@
    +          $source = language_load($request->attributes->get('source'));

    Does language_load() not have an injectable alternative yet?

  3. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Access/ContentTranslationOverviewAccess.php
    @@ -0,0 +1,86 @@
    +    return array_key_exists('_content_translation_access', $route->getRequirements());

    As above.

  4. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -0,0 +1,306 @@
    +      t('Language'),

    t() should be an injected service.

  5. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -0,0 +1,306 @@
    +   * Page callback for the translation addition page.

    Controller methods shouldn't be called "page callbacks", since that's not what they are anymore.

  6. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -0,0 +1,306 @@
    +    $this->moduleHandler->loadInclude('content_translation', 'inc', 'content_translation.pages');

    Just move code from that file back to the .module file. Don't bother with loadInclude() anymore. It's a ten year old hack. :-)

  7. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Routing/ContentTranslationRouteSubscriber.php
    @@ -0,0 +1,108 @@
    +  /**
    +   * The entity type manager.
    +   *
    +   * @var \Drupal\Core\Entity\EntityManager
    +   */
    +  protected $manager;
    +

    This should be called $entityManager. "$manager" tells me nothing about what it actually is.

Status:Needs work» Needs review
StatusFileSize
new20.12 KB
new43.98 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php.
[ View ]

This patch has Crell's suggestions implemented except for 2 because I couldn't find any evidence of such a thing... git log -p -Slanguage_load didn't show any case where it was removed for something else.

Some of those tests that had numerous failures have less now.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new805 bytes
new43.98 KB
FAILED: [[SimpleTest]]: [MySQL] 57,685 pass(es), 345 fail(s), and 233 exception(s).
[ View ]

derp

Status:Needs review» Needs work

The last submitted patch, 1987882-40.patch, failed testing.

Title:Convert content_translation_overview() to a new style controllerConvert content_translation routes to a new style controller

Renaming to prevent confusion.

Assigned:Unassigned» piyuesh23

Looking into this.

StatusFileSize
new3.71 KB
new44.05 KB
FAILED: [[SimpleTest]]: [MySQL] 57,157 pass(es), 22 fail(s), and 0 exception(s).
[ View ]

Straightforward re-roll.

Status:Needs work» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs review

oops

Status:Needs review» Needs work

The last submitted patch, 1987882-41.patch, failed testing.

Assigned:piyuesh23» Unassigned
Status:Needs work» Needs review
StatusFileSize
new44.1 KB
FAILED: [[SimpleTest]]: [MySQL] 57,621 pass(es), 349 fail(s), and 214 exception(s).
[ View ]

This one isn't likely to work any better than the previous patch. It appears /user can't be loaded, which causes some tests to fail. Can't make any sense of it, but I'm re-rolling the patch onto the ContainerInjectionInterface change.

Status:Needs review» Needs work

The last submitted patch, 1987882-48.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.95 KB
new42.57 KB
FAILED: [[SimpleTest]]: [MySQL] 58,153 pass(es), 343 fail(s), and 232 exception(s).
[ View ]

The last patch was still using ControllerInterface on the controller. Rerolling using ControllerBase instead. This should put us back at the same state as #44.

Status:Needs review» Needs work

The last submitted patch, drupal8.content-translation.module.1987882-50.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new20.22 KB
new44.48 KB
FAILED: [[SimpleTest]]: [MySQL] 58,694 pass(es), 408 fail(s), and 114 exception(s).
[ View ]

This is a massive overhaul:

  1. Adds getTranslationController to EntityManager instead of making getController public
  2. Moves title to RouteSubscriber
  3. Cleans up access checks (they don't need ContainerInjection and create, since params are passed via services.yml)
  4. Gets rid of case in access checker, since code between create and update are identical, aside from $operation, which is a variable.
  5. Refactors access check to have same logic as previous access callback
  6. Used $this->currentUser if no account is passed to overview
  7. langcode -> id
  8. Adds a LanguageConverter to upcast language codes
  9. fixes entity param to convert properly using parameters
  10. sets default values in route

Adds getTranslationController to EntityManager instead of making getController public

I did not look at the patch in details, but I don't think this is correct: CT should be coded as it were a contrib module, the entity system should not depend explictly on it nor know of interfaces it provides. If we have a use case to make getController() public, then other modules exposing their own entity controllers might need that. I'd suggest to revert this change.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new9.03 KB
new45.56 KB
FAILED: [[SimpleTest]]: [MySQL] 58,952 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

restored the public, and removed the custom method, although this controller can't use the public method, because it's constructor takes two arguments, so getting the class and creating a controller manually. Also, noticed that create and update are not completely identical. They have some different variable names, so fixed the access check. Local tests were passing with a couple exceptions that make no sense to me (illegal string offset menu_base_path, which I checked and is defined in the entity, so I'm not sure if it's a fluke or not.

Status:Needs review» Needs work

The last submitted patch, drupal8.content-translation.module.1987882-55.patch, failed testing.

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

In short, we have 3 more days to get this to RTBC or we'll need to split this to two issues and do it in 2 steps. Anybody wanna jump on this? :)

Issue tags:+language-content

  1. +++ w/core/modules/content_translation/lib/Drupal/content_translation/Access/ContentTranslationOverviewAccess.php
    @@ -0,0 +1,77 @@
    +      // Get account details from request.
    +      $account = $request->attributes->get('_account');

    // @todo revisit after https://drupal.org/node/2048223
    $account = \Drupal::curentUser();
    $this->prepareUser($account)

  2. +++ w/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -0,0 +1,281 @@
    +  }
    +}

    need empty line before end of class

Status:Needs work» Needs review
StatusFileSize
new3.04 KB
new45.57 KB
FAILED: [[SimpleTest]]: [MySQL] 59,168 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Fixes most test failures. There's still an issue with translator not having permission.

Status:Needs review» Needs work

The last submitted patch, drupal8.content-translation.module.1987882-61.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.06 KB
new46.51 KB
PASSED: [[SimpleTest]]: [MySQL] 58,763 pass(es).
[ View ]

Finally!!! The operation was wrong in access check (should be update, not edit). Also, the edit() method of controller needed a type hint for Language to properly upcast. The test that failed above passed green for me locally.

Status:Needs review» Needs work

+++ w/core/modules/content_translation/content_translation.module
@@ -1044,3 +995,91 @@ function content_translation_save_settings($settings) {
+/**
+ * Form constructor for the translation deletion confirmation.
+ */
+function content_translation_delete_confirm(array $form, array $form_state, EntityInterface $entity, Language $language) {
+  $langcode = $language->id;
+  $controller = content_translation_controller($entity->entityType());
+
+  return confirm_form(
+    $form,
+    t('Are you sure you want to delete the @language translation of %label?', array('@language' => $language->name, '%label' => $entity->label())),
+    $controller->getEditPath($entity),
+    t('This action cannot be undone.'),
+    t('Delete'),
+    t('Cancel')
+  );
+}
+
+/**
+ * Form submission handler for content_translation_delete_confirm().
+ */
+function content_translation_delete_confirm_submit(array $form, array &$form_state) {
+  list($entity, $language) = $form_state['build_info']['args'];
+  $controller = content_translation_controller($entity->entityType());
+
+  // Remove the translated values.
+  $controller->removeTranslation($entity, $language->id);
+  $entity->save();
+
+  // Remove any existing path alias for the removed translation.
+  // @todo This should be taken care of by the Path module.
+  if (module_exists('path')) {
+    $conditions = array('source' => $controller->getViewPath($entity), 'langcode' => $language->id);
+    Drupal::service('path.crud')->delete($conditions);
+  }
+
+  $form_state['redirect'] = $controller->getBasePath($entity) . '/translations';
+}

Let's convert this to OO or revert this.

Status:Needs work» Needs review
StatusFileSize
new9.87 KB
new51.02 KB
PASSED: [[SimpleTest]]: [MySQL] 58,793 pass(es).
[ View ]

converted!

just minor

  1. +++ w/core/modules/content_translation/lib/Drupal/content_translation/Form/ContentTranslationDeleteForm.php
    @@ -0,0 +1,168 @@
    +    $form_state['entity'] = $entity;
    +    $form_state['language'] = $language;
    ...
    +    $entity = $form_state['entity'];
    +    $language = $form_state['language'];

    We have $this->entity and $this->language I think we don't need these.

  2. +++ w/core/modules/content_translation/lib/Drupal/content_translation/Routing/ContentTranslationRouteSubscriber.php
    @@ -124,6 +124,30 @@ public function routes(RouteBuildEvent $event) {
    +

    no need for this line.

StatusFileSize
new1.97 KB
new51.01 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.content-translation.module.1987882-67.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Storing the entity and language on the object is necessary for the questions and cancel route to be able to access them. Because the submit is a fresh request, it doesn't have access to those variables, so we pass them via form_state. Patch addresses some blank line issues.

Issue tags:-Needs manual testing

Removing manual testing tag. This got plenty of manual testing in debugging test failures.

Status:Needs review» Needs work
Issue tags:+D8MI, +language-content, +Stalking Crell, +WSCCI-conversion

The last submitted patch, drupal8.content-translation.module.1987882-67.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new50.13 KB
FAILED: [[SimpleTest]]: [MySQL] 58,268 pass(es), 349 fail(s), and 90 exception(s).
[ View ]

Due to #2089635: Convert non-test non-form page callbacks to routes and controllers going into core some of this work has already been done, which also caused to patch to not apply anymore.

I have tried to resolve the merge conflicts I got putting the patch on core as it is right now.
I also removed the change to the EntityManager class which made the getController method public instead of private. This change might make sense, but I didn't see how it had any reference to this issue, so I removed that part of the patch.

Status:Needs review» Needs work

The last submitted patch, drupal8.content-translation.module.1987882-71.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.29 KB
new50.06 KB
FAILED: [[SimpleTest]]: [MySQL] 58,581 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Fixed the issue that caused the tests to fail.

Status:Needs review» Needs work

The last submitted patch, drupal8.content-translation.module.1987882-73.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.37 KB
new50.16 KB
PASSED: [[SimpleTest]]: [MySQL] 58,447 pass(es).
[ View ]

Fixed the issue with the test cases - accessing unpublished translations and fixed a notice I came upon.

Status:Needs review» Needs work

+++ b/core/modules/content_translation/content_translation.moduleundefined
@@ -1011,3 +977,53 @@ function content_translation_save_settings($settings) {
+ * @returns
+ *   A renderable array of language switch links.
+ */

@return instead of @returns. Also don't forget to specify the type of data that is returned.

+++ b/core/modules/content_translation/content_translation.moduleundefined
@@ -1011,3 +977,53 @@ function content_translation_save_settings($settings) {
+ *   The entitiy being translated.

Spelling: entitiy.

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Access/ContentTranslationManageAccessCheck.phpundefined
@@ -8,15 +8,15 @@
- * Access check for entity translation CRUD operation.
+ * Access check for entity translation CURD operation.
  */

Why change CRUD to CURD?

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Access/ContentTranslationManageAccessCheck.phpundefined
@@ -57,31 +57,28 @@ public function access(Route $route, Request $request) {
   }
-
}

Don't remove this empty line. The curly brace that closes a class should be preceded by a blank line to distinguish it from one that closes a method.

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Access/ContentTranslationOverviewAccess.phpundefined
@@ -8,14 +8,16 @@
use Drupal\Core\Entity\EntityManager;
-use Drupal\Core\Access\StaticAccessCheckInterface;
+use Drupal\Core\Access\AccessCheckInterface;
+use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
use Symfony\Component\Routing\Route;
use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\DependencyInjection\ContainerInterface;

Keep these in alphabetical order, with Drupal namespaces at the top, and third parties at the bottom.

+++ b/core/modules/content_translation/lib/Drupal/content_translation/ContentTranslationController.phpundefined
@@ -235,8 +235,11 @@ public function entityFormAlter(array &$form, array &$form_state, EntityInterfac
+        if (!($new_translation)) {

This looks strange. Use empty($new_translation) or !$new_translation.

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.phpundefined
@@ -7,38 +7,276 @@
+use Drupal\Core\Controller\ControllerBase;
+use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
use Drupal\Core\Entity\EntityInterface;
+use Drupal\Core\Session\AccountInterface;
+use Drupal\field\FieldInfo;

Keep the Core classes together, and in alphabetical order.

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.phpundefined
@@ -7,38 +7,276 @@
+   * @param \Drupal\Core\Entity\EntityInterface $entity
+   *   The entity to be translationd.

Has to be 'translated' instead of 'translationd' :)

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.phpundefined
@@ -7,38 +7,276 @@
+    if (language_multilingual()) {

It seems dirty to fall back to the procedural functions here. You should be able ti get the language_manager service from the dependency injection container and call isMultilingual() on it.

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Form/ContentTranslationDeleteForm.phpundefined
@@ -0,0 +1,168 @@
+use Drupal\Core\Form\ConfirmFormBase;
+use Drupal\Core\Entity\EntityManager;
+use Drupal\Core\Extension\ModuleHandlerInterface;
+use Drupal\Core\Path\Path;
+use Drupal\Core\Entity\EntityInterface;
+use Drupal\Core\Language\Language;
+use Symfony\Component\DependencyInjection\ContainerInterface;

Alphabetical ordering.

Status:Needs work» Needs review
StatusFileSize
new38.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.content-translation.module.1987882-77.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

straight reroll. Some of the comments above are addressed by the access checks and route already being in core with quick-and-dirty conversions.

Status:Needs review» Needs work

The last submitted patch, drupal8.content-translation.module.1987882-77.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new38.28 KB
FAILED: [[SimpleTest]]: [MySQL] 58,406 pass(es), 329 fail(s), and 185 exception(s).
[ View ]

looks like something changed since I pulled last night. Here's another.

Status:Needs review» Needs work

The last submitted patch, drupal8.content-translation.module.1987882-79.patch, failed testing.

StatusFileSize
new9.91 KB
new76.37 KB
FAILED: [[SimpleTest]]: [MySQL] 58,424 pass(es), 380 fail(s), and 173 exception(s).
[ View ]

Working off of #75, removing the access/route changes, the ones done in the quick convert are better than the ones existing in this old patch.

Status:Needs work» Needs review

Working off of #75, removing the access/route changes, the ones done in the quick convert are better than the ones existing in this old patch.

Status:Needs review» Needs work

The last submitted patch, drupal8.content-translation.module.1987882-81.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new43.59 KB
new42.89 KB
FAILED: [[SimpleTest]]: [MySQL] 58,244 pass(es), 19 fail(s), and 12 exception(s).
[ View ]

removed some language_load calls and fixes hook_menu entry so Edit link shows up correctly so tests pass. Also, general cleanup including removing some temporary files accidentally committed in merge. As for comment above regarding use order, unless something has changed, to my knowledge there isn't a consensus as to order being alphabetical or not. My preference is towards using the order it shows up in the code, but I'm flexible if a decision is made one way or the other. I find having the thing it extends/implements, followed by the params to the constructor make the most sense to me, with all the Symfony stuff below the Drupal stuff.

Status:Needs review» Needs work

The last submitted patch, drupal8.content-translation.module.1987882-84.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2 KB
new44.41 KB
FAILED: [[SimpleTest]]: [MySQL] 58,767 pass(es), 19 fail(s), and 0 exception(s).
[ View ]

fixes route subscriber to use delete form and removes the wrapper controller.

Status:Needs review» Needs work

The last submitted patch, drupal8.content-translation.module.1987882-86.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.2 KB
new43.21 KB
PASSED: [[SimpleTest]]: [MySQL] 58,886 pass(es).
[ View ]

fixes test failures.

Beware: I made a misstep in #2091691: Convert test non-form page callbacks to routes and controllers with some of my changes to content_translation that is being addressed in #2102777-11: Allow theme_links to use routes as well as href. See the interdiff "2102777-11-tim.txt"

Issue summary:View changes
Status:Needs review» Needs work
Issue tags:+Needs reroll

Assigned:Unassigned» pfrenssen

This needs some work after #2106349: Comment translation overview has broken local tasks got in, which now retrieves the entity from the request in ContentTranslationController, instead of having it passed in as an argument.

Assigned:pfrenssen» Unassigned

There are some test failures on deleting translations, but I am unable to debug it since I keep hitting #2129039: Integrity constraint violation when translating body field and can't even get to a point where I have a valid translation to delete.

The LanguageConverter part seems overly complex. Upcasters are extremely expensive to run, if it can be avoided let's please do it.

Also, #2086479: Convert content_translation_delete_confirm() to the new form interface was marked as a dupe, but that contains one of the last 2 confirm_forms(). I might steal that one back...

Okay because I want to kill confirm_form ASAP, I reopened #2086479: Convert content_translation_delete_confirm() to the new form interface.

So #2086479: Convert content_translation_delete_confirm() to the new form interface was swiftly committed. Those parts would need to be removed from this patch. As for the upcasting, 'language_entity' upcasting would be done by the existing upcasting infrastructure without further work. That does not upcast to an object that we use for type hinting elsewhere though :/ All the fault of CMI's circular dependency on language stuff and storing languages as config entities.

Status:Needs work» Needs review
StatusFileSize
new24.52 KB
FAILED: [[SimpleTest]]: [MySQL] 58,944 pass(es), 373 fail(s), and 123 exception(s).
[ View ]

Re-roll + Removing delete_confirm() part

Status:Needs review» Needs work

The last submitted patch, 97: 1987882-content-translation-97.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new26.49 KB
FAILED: [[SimpleTest]]: [MySQL] 59,869 pass(es), 22 fail(s), and 0 exception(s).
[ View ]

Reroll...

Status:Needs review» Needs work

The last submitted patch, 99: 1987882-content-translation-99.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.94 KB
new26.51 KB
FAILED: [[SimpleTest]]: [MySQL] 59,857 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Fixing the URL issue...

Status:Needs review» Needs work

The last submitted patch, 102: 1987882-content-translation-102.patch, failed testing.

The last submitted patch, 102: 1987882-content-translation-102.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new26.51 KB
FAILED: [[SimpleTest]]: [MySQL] 59,994 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new1.94 KB

Still test failing because of $entity missing in edit. But has further improvements from #102

The last submitted patch, 105: 1987882-content-translation-105.patch, failed testing.

StatusFileSize
new26.48 KB
FAILED: [[SimpleTest]]: [MySQL] 59,790 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.1 KB

Fixing test fails...

The last submitted patch, 107: 1987882-content-translation-107.patch, failed testing.

The last submitted patch, 107: 1987882-content-translation-107.patch, failed testing.

StatusFileSize
new26.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987882-content-translation-110.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.53 KB

Minor fix on edit() method for the test fail. Not bug, but an observation is we don't really use content translation edit path (e.g. /node/1/translations/edit/fr) and use the language prefixed path of edit (e.g. /fr/node/1/edit).

Status:Needs review» Needs work

The last submitted patch, 110: 1987882-content-translation-110.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new26.62 KB
FAILED: [[SimpleTest]]: [MySQL] 62,803 pass(es), 336 fail(s), and 94 exception(s).
[ View ]

This is a straight re-roll of #110.

Status:Needs review» Needs work

The last submitted patch, 113: 1987882-content-translation-113.patch, failed testing.

The last submitted patch, 113: 1987882-content-translation-113.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new26.75 KB
FAILED: [[SimpleTest]]: [MySQL] 63,896 pass(es), 380 fail(s), and 96 exception(s).
[ View ]

LAst patch rerolled. Now the patch applies successfully.

Status:Needs review» Needs work

The last submitted patch, 116: content_translation_improved.patch, failed testing.

Assigned:Unassigned» Sill

Issue tags:-Needs reroll

Patch from #116 still applies successfully.

Status:Needs work» Needs review
StatusFileSize
new27.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,539 pass(es), 427 fail(s), and 233 exception(s).
[ View ]
new7.61 KB

Rerolled.

Status:Needs review» Needs work

+++ b/core/modules/content_translation/content_translation.module
@@ -178,11 +178,47 @@ function content_translation_entity_base_field_info_alter(&$fields, EntityTypeIn
+function content_translation_menu() {

There ain't no hook_menu for that, afaik we dropped all that code anyway./

The last submitted patch, 120: content_translation-add_page-1987882-120.patch, failed testing.

+++ b/core/modules/content_translation/content_translation.module
@@ -178,11 +178,47 @@ function content_translation_entity_base_field_info_alter(&$fields, EntityTypeIn
-function content_translation_field_info_alter(&$info) {

This should not be removed!!!

Status:Needs work» Needs review
StatusFileSize
new31.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,704 pass(es), 73 fail(s), and 11 exception(s).
[ View ]

I've rerolled the path 110, have modified it. It works while the manual testing. I see some tests fails. I hope somebody will make useful comments.

Status:Needs review» Needs work

The last submitted patch, 124: content_translation-add_page-1987882-124.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new39.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,665 pass(es), 75 fail(s), and 11 exception(s).
[ View ]

Manual testing works well. Some tests fail. I guess somebody give an advise.

Status:Needs review» Needs work

The last submitted patch, 126: content_translation-add_page-1987882-126.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 126: content_translation-add_page-1987882-126.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new34.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,274 pass(es), 12 fail(s), and 0 exception(s).
[ View ]

It seems work. It is not finished yet. I need get some comments about.

Status:Needs review» Needs work

The last submitted patch, 130: content_translation-add_page-1987882-130.patch, failed testing.

Assigned:Sill» Unassigned
Status:Needs work» Needs review
StatusFileSize
new47.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,569 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

StatusFileSize
new35.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,423 pass(es).
[ View ]

The last submitted patch, 132: content_translation-add_page-1987882-132.patch, failed testing.