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.

CommentFileSizeAuthor
#225 1987882-content-translation-route.interdiff.219-225.txt1005 bytespenyaskito
#225 1987882-content-translation-route-225.patch42.3 KBpenyaskito
#3 translation_entity-translation_entity_overview_controller-1987882-3.patch2.79 KBacrollet
#4 translation_entity-translation_entity_overview_controller-1987882-4.patch18.89 KBInternetDevels
#6 translation_entity-translation_entity_overview_controller-1987882-6.patch22.13 KBInternetDevels
#9 translation_entity-translation_entity_overview_controller-1987882-9.patch22.61 KBInternetDevels
#11 translation-entity-form-interface-1987882-11.patch37.73 KBInternetDevels
#16 1987882-translation_entity-translation_entity_overview_controller-16.patch35.71 KBvijaycs85
#17 1987882-diff-11-16.txt22.7 KBvijaycs85
#19 1987882-diff-16-19.txt3.56 KBvijaycs85
#19 1987882-translation_entity-translation_entity_overview_controller-19.patch36.43 KBvijaycs85
#21 1987882-diff-19-21.txt3.01 KBvijaycs85
#21 1987882-translation_entity-translation_entity_overview_controller-21.patch36.47 KBvijaycs85
#24 content_translation-1987882-24.patch36.36 KBtim.plunkett
#29 1987882-diff-24-49.txt4.06 KBvijaycs85
#29 1987882-translation_entity-translation_entity_overview_controller-29.patch36.91 KBvijaycs85
#35 1987882-diff-29-35.txt8.03 KBvijaycs85
#35 1987882-translation_entity-translation_entity_overview_controller-35.patch36.95 KBvijaycs85
#38 interdiff.txt20.12 KBwamilton
#38 1987882-translation_entity-translation_entity_overview_controller-38.patch43.98 KBwamilton
#40 interdiff.txt805 byteswamilton
#40 1987882-40.patch43.98 KBwamilton
#44 1987882-40_41_interdiff.txt3.71 KBLetharion
#44 1987882-41.patch44.05 KBLetharion
#48 1987882-48.patch44.1 KBLetharion
#50 interdiff.txt9.95 KBdisasm
#50 drupal8.content-translation.module.1987882-50.patch42.57 KBdisasm
#52 interdiff.txt20.22 KBdisasm
#52 drupal8.content-translation.module.1987882-52.patch44.48 KBdisasm
#55 interdiff.txt9.03 KBdisasm
#55 drupal8.content-translation.module.1987882-55.patch45.56 KBdisasm
#61 interdiff.txt3.04 KBdisasm
#61 drupal8.content-translation.module.1987882-61.patch45.57 KBdisasm
#63 interdiff.txt3.06 KBdisasm
#63 drupal8.content-translation.module.1987882-63.patch46.51 KBdisasm
#65 interdiff.txt9.87 KBdisasm
#65 drupal8.content-translation.module.1987882-64.patch51.02 KBdisasm
#67 interdiff.txt1.97 KBdisasm
#67 drupal8.content-translation.module.1987882-67.patch51.01 KBdisasm
#71 drupal8.content-translation.module.1987882-71.patch50.13 KBgoogletorp
#73 interdiff.txt1.29 KBgoogletorp
#73 drupal8.content-translation.module.1987882-73.patch50.06 KBgoogletorp
#75 interdiff.txt2.37 KBgoogletorp
#75 drupal8.content-translation.module.1987882-75.patch50.16 KBgoogletorp
#77 drupal8.content-translation.module.1987882-77.patch38.21 KBdisasm
#79 drupal8.content-translation.module.1987882-79.patch38.28 KBdisasm
#81 interdiff.txt9.91 KBdisasm
#81 drupal8.content-translation.module.1987882-81.patch76.37 KBdisasm
#84 interdiff.txt43.59 KBdisasm
#84 drupal8.content-translation.module.1987882-84.patch42.89 KBdisasm
#86 interdiff.txt2 KBdisasm
#86 drupal8.content-translation.module.1987882-86.patch44.41 KBdisasm
#89 interdiff.txt3.2 KBdisasm
#89 drupal8.content-translation.module.1987882-89.patch43.21 KBdisasm
#97 1987882-content-translation-97.patch24.52 KBvijaycs85
#99 1987882-content-translation-99.patch26.49 KBvijaycs85
#102 1987882-diff-99-102.txt1.94 KBvijaycs85
#102 1987882-content-translation-102.patch26.51 KBvijaycs85
#105 1987882-content-translation-105.patch26.51 KBvijaycs85
#105 1987882-diff-102-105.txt1.94 KBvijaycs85
#107 1987882-content-translation-107.patch26.48 KBvijaycs85
#107 1987882-diff-105-107.txt1.1 KBvijaycs85
#110 1987882-content-translation-110.patch26.63 KBvijaycs85
#110 1987882-diff-107-110.txt1.53 KBvijaycs85
#113 1987882-content-translation-113.patch26.62 KBkim.pepper
#116 content_translation_improved.patch26.75 KBneetu morwani
#120 content_translation-add_page-1987882-120.patch27.42 KBvictor-shelepen
#120 content_translation-add_page-1987882-116-120.txt7.61 KBvictor-shelepen
#124 content_translation-add_page-1987882-124.patch31.51 KBvictor-shelepen
#126 content_translation-add_page-1987882-126.patch39.54 KBvictor-shelepen
#130 content_translation-add_page-1987882-130.patch34.43 KBvictor-shelepen
#132 content_translation-add_page-1987882-132.patch47.91 KBvictor-shelepen
#133 content_translation-add_page-1987882-133.patch35.54 KBvictor-shelepen
#139 content_translation-add_page-1987882-139.patch36.34 KBvictor-shelepen
#141 content_translation-add_page-1987882-139-141.txt1.06 KBvictor-shelepen
#141 content_translation-add_page-1987882-141.patch36.26 KBvictor-shelepen
#143 interdiff.1987882.141.143.txt1.71 KBYesCT
#143 content_translation-add_page-1987882-143.patch35.72 KBYesCT
#143 space.png389.26 KBYesCT
#143 newline.png742.71 KBYesCT
#144 interdiff.1987882.143.144.txt4.1 KBYesCT
#144 content_translation-add_page-1987882-144.patch35.73 KBYesCT
#147 content_translation-1987882-147.patch33.17 KBdawehner
#147 interdiff.txt11.39 KBdawehner
#149 content_translation-1987882-149.patch32.07 KBdawehner
#150 interdiff.txt1.11 KBdawehner
#155 1987882-content_transaltion_controller-154.patch31.51 KBvijaycs85
#157 1987882-content_transaltion_controller-157.patch31.63 KBvijaycs85
#157 1987882-diff-155-157.txt4.21 KBvijaycs85
#163 content_translation-add_page-1987882-163.patch30.5 KBvictor-shelepen
#163 content_translation-add_page-1987882-157-163.txt3.32 KBvictor-shelepen
#168 content_translation-add_page-1987882-168.patch31.04 KBvictor-shelepen
#168 content_translation-add_page-1987882-163-168.txt2.53 KBvictor-shelepen
#170 content_translation-add_page-1987882-170.patch30.96 KBvictor-shelepen
#170 content_translation-add_page-1987882-168-170.diff2.24 KBvictor-shelepen
#172 1987882-content-translation-route-172.2.patch32.65 KBkim.pepper
#172 interdiff.txt13.19 KBkim.pepper
#174 1987882-content-translation-route-173.patch33.29 KBkim.pepper
#174 interdiff.txt657 byteskim.pepper
#176 1987882-content-translation-route-176.patch33.07 KBkatbailey
#176 interdiff.txt2.25 KBkatbailey
#179 1987882-content-translation-route-179.patch34.62 KBkim.pepper
#179 interdiff.txt6.38 KBkim.pepper
#181 1987882-content-translation-route-181.patch34.22 KBkim.pepper
#181 interdiff.txt8.4 KBkim.pepper
#183 1987882-content-translation-route-183.patch34.57 KBkim.pepper
#183 interdiff.txt1.98 KBkim.pepper
#185 1987882-content-translation-route-185.patch40.04 KBkim.pepper
#185 interdiff.txt8.37 KBkim.pepper
#187 1987882-content-translation-route-187.patch39.37 KBkim.pepper
#187 interdiff.txt2.3 KBkim.pepper
#189 1987882-content-translation-route-189.patch28.77 KBkim.pepper
#189 interdiff.txt712 byteskim.pepper
#190 interdiff.1987882.189.190.txt6.91 KBYesCT
#190 1987882-content-translation-route-190.patch27.96 KBYesCT
#195 1987882-content-translation-route-195.patch28.05 KBpenyaskito
#195 interdiff.txt3.07 KBpenyaskito
#197 1987882-content-translation-route-197.patch28.43 KBpenyaskito
#197 interdiff.txt1.33 KBpenyaskito
#199 1987882-content-translation-route-199.patch28.5 KBpenyaskito
#199 interdiff-197-199.txt690 bytespenyaskito
#200 1987882-content-translation-route-200.patch28.54 KBpenyaskito
#200 interdiff-199-200.txt5.15 KBpenyaskito
#202 1987882-content-translation-route-202.patch27.35 KBpenyaskito
#202 interdiff.txt6.01 KBpenyaskito
#206 1987882-content-translation-route-206.patch27.2 KBkim.pepper
#206 interdiff.txt5.07 KBkim.pepper
#207 1987882-content-translation-route-207.patch27.3 KBpenyaskito
#210 1987882-content-translation-route-210.patch27.85 KBpenyaskito
#210 1987882-content-translation-route.interdiff.207-210.txt897 bytespenyaskito
#213 1987882-content-translation-route-213.patch38.77 KBkim.pepper
#213 interdiff.txt10.92 KBkim.pepper
#215 1987882-content-translation-route-215.patch39.96 KBpenyaskito
#215 1987882-content-translation-route.interdiff.213-215.txt11.67 KBpenyaskito
#217 1987882-content-translation-route-217.patch41.46 KBpenyaskito
#217 1987882-content-translation-route.interdiff.215-217.txt1.88 KBpenyaskito
#219 1987882-content-translation-route-219.patch42.3 KBpenyaskito
#219 1987882-content-translation-route.interdiff.217-219.txt865 bytespenyaskito
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

acrollet’s picture

Assigned: Unassigned » acrollet
acrollet’s picture

I'm working on this issue.

acrollet’s picture

Assigned: acrollet » Unassigned
Status: Active » Needs work
FileSize
2.79 KB

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.

InternetDevels’s picture

Status: Needs work » Needs review
FileSize
18.89 KB

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
InternetDevels’s picture

Status: Needs work » Needs review
FileSize
22.13 KB

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.

andypost’s picture

Issue tags: +Needs manual testing, +D8MI

Code looks good, just needs manual testing

dawehner’s picture

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?

InternetDevels’s picture

Status: Needs work » Needs review
FileSize
22.61 KB

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.

andypost’s picture

Files' structure is ok.

InternetDevels’s picture

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.

star-szr’s picture

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

vijaycs85’s picture

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

Status: Postponed » Needs review

The other one is actually easier to reroll(?)

dawehner’s picture

+++ 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@

vijaycs85’s picture

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.

vijaycs85’s picture

FileSize
22.7 KB

Interdiff after conflict merge....

Status: Needs review » Needs work
vijaycs85’s picture

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

Status: Needs review » Needs work
vijaycs85’s picture

Hopefully this patch fixes few test cases...

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

Title: Convert translation_entity_overview() to a new style controller » Convert content_translation_overview() to a new style controller
Component: translation_entity.module » content_translation.module
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
36.36 KB

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.

tim.plunkett’s picture

Status: Needs work » Needs review

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

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

jibran’s picture

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.

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: -Stalking Crell
FileSize
4.06 KB
36.91 KB

Updating review changes in #28

vijaycs85’s picture

Issue tags: +Stalking Crell

restoring tag.

Status: Needs review » Needs work
jibran’s picture

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.

jibran’s picture

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

Gábor Hojtsy’s picture

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?

vijaycs85’s picture

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

Crell’s picture

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.

wamilton’s picture

Status: Needs work » Needs review
FileSize
20.12 KB
43.98 KB

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
wamilton’s picture

Status: Needs work » Needs review
FileSize
805 bytes
43.98 KB

derp

Status: Needs review » Needs work

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

disasm’s picture

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

Renaming to prevent confusion.

piyuesh23’s picture

Assigned: Unassigned » piyuesh23

Looking into this.

Letharion’s picture

Straightforward re-roll.

disasm’s picture

Status: Needs work » Reviewed & tested by the community
disasm’s picture

Status: Reviewed & tested by the community » Needs review

oops

Status: Needs review » Needs work

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

Letharion’s picture

Assigned: piyuesh23 » Unassigned
Status: Needs work » Needs review
FileSize
44.1 KB

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
9.95 KB
42.57 KB

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
20.22 KB
44.48 KB

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
plach’s picture

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.

plach’s picture

Status: Needs review » Needs work
disasm’s picture

Status: Needs work » Needs review
FileSize
9.03 KB
45.56 KB

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.

xjm’s picture

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.

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Issue tags: +language-content
andypost’s picture

  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

disasm’s picture

Status: Needs work » Needs review
FileSize
3.04 KB
45.57 KB

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
3.06 KB
46.51 KB

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.

jibran’s picture

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
9.87 KB
51.02 KB

converted!

jibran’s picture

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.

disasm’s picture

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.

disasm’s picture

Issue tags: -Needs manual testing

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

googletorp’s picture

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.

googletorp’s picture

Status: Needs work » Needs review
FileSize
50.13 KB

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.

googletorp’s picture

Status: Needs work » Needs review
FileSize
1.29 KB
50.06 KB

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.

googletorp’s picture

Status: Needs work » Needs review
FileSize
2.37 KB
50.16 KB

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

pfrenssen’s picture

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
38.21 KB

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
38.28 KB

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.

disasm’s picture

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.

disasm’s picture

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
43.59 KB
42.89 KB

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
2 KB
44.41 KB

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.

vijaycs85’s picture

disasm’s picture

Status: Needs work » Needs review
FileSize
3.2 KB
43.21 KB

fixes test failures.

tim.plunkett’s picture

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"

pfrenssen’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll
pfrenssen’s picture

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.

pfrenssen’s picture

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.

tim.plunkett’s picture

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

tim.plunkett’s picture

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

Gábor Hojtsy’s picture

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
24.52 KB

Re-roll + Removing delete_confirm() part

Status: Needs review » Needs work

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
26.49 KB

Reroll...

Status: Needs review » Needs work

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

kim.pepper’s picture

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.94 KB
26.51 KB

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
26.51 KB
1.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.

vijaycs85’s picture

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.

vijaycs85’s picture

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

kim.pepper’s picture

Status: Needs review » Needs work

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
26.62 KB

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.

neetu morwani’s picture

Status: Needs work » Needs review
FileSize
26.75 KB

LAst patch rerolled. Now the patch applies successfully.

Status: Needs review » Needs work

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

Sill’s picture

Assigned: Unassigned » Sill
Alumei’s picture

Issue tags: -Needs reroll

Patch from #116 still applies successfully.

victor-shelepen’s picture

Rerolled.

dawehner’s picture

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.

andypost’s picture

+++ 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!!!

victor-shelepen’s picture

Status: Needs work » Needs review
FileSize
31.51 KB

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.

victor-shelepen’s picture

Status: Needs work » Needs review
FileSize
39.54 KB

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.

victor-shelepen’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

victor-shelepen’s picture

Status: Needs work » Needs review
FileSize
34.43 KB

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.

victor-shelepen’s picture

Assigned: Sill » Unassigned
Status: Needs work » Needs review
FileSize
47.91 KB
victor-shelepen’s picture

FileSize
35.54 KB

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

victor-shelepen’s picture

It duplicates this one https://drupal.org/node/2224607. We need an authorized person who is able to take a right decision.

victor-shelepen’s picture

Status: Needs review » Needs work
andypost’s picture

Closed #2224607: Move all functions from content_translation.pages.inc file to Drupal\content_translation\Controller\ContentTranslationController class as duplicate of this

Looks mostly RTBC just minor nitpicks:

  1. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -7,41 +7,292 @@
    +     * ¶
    

    trailing whitespace

  2. +++ b/core/modules/language/language.services.yml
    @@ -16,3 +16,7 @@ services:
    \ No newline at end of file
    

    please add new line

  3. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.php
    @@ -101,7 +101,7 @@ protected function createEntity($values, $langcode, $bundle_name = NULL) {
    -      return t('Save and keep published') . $this->getFormSubmitSuffix($entity, $langcode);
    +      return t('Save and publish') . $this->getFormSubmitSuffix($entity, $langcode);
    
    @@ -117,7 +117,7 @@ protected function doTestPublishedStatus() {
    -      t('Save and keep published'),
    +      t('Save and publish'),
    

    any reason for?

victor-shelepen’s picture

3. <- I fixed tests because they are failed. I looked what tests had generated, and I modified tests. It works.
1. <- I can not understand. Sorry.

victor-shelepen’s picture

Status: Needs work » Needs review
FileSize
36.34 KB

Status: Needs review » Needs work

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

victor-shelepen’s picture

andypost’s picture

Just minor nitpicks

  1. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -7,41 +7,301 @@
    +     * ¶
    

    trailing whitespace

  2. +++ b/core/modules/language/language.routing.yml
    @@ -80,4 +80,4 @@ language.content_settings_page:
    -    _permission: 'administer languages'
    +    _permission: 'administer languages'
    \ No newline at end of file
    
    +++ b/core/modules/language/language.services.yml
    @@ -16,3 +16,7 @@ services:
    +  language_converter:
    +    class: Drupal\language\LanguageConverter
    +    tags:
    +      - { name: paramconverter }
    \ No newline at end of file
    

    needs new line

YesCT’s picture

fixing whitespace nits mentioned in #137 and #142.

@likin attaching screenshots to maybe help explain why this bothered andypost. you should be able to do a git diff and look to see unintended changes like this, also looking at your patch in dreditor after you upload it to an issue might be useful. https://dreditor.org/

I have other comments I'll make separatedly.

YesCT’s picture

just pointing out and fixing some nits.

  1. +++ b/core/modules/content_translation/content_translation.module
    @@ -885,3 +882,41 @@ function content_translation_save_settings($settings) {
    +  }
    +}
    +
    

    another very small nit, I'll just fix.

    too many newlines at end of file,
    and at end of class, should be a newline before the closing class curly brace.

    https://drupal.org/node/608152#indenting
    "Leave an empty line between end of method and end of class definition:"

  2. +++ /dev/null
    @@ -1,234 +0,0 @@
    -  // @todo Exploit the upcoming hook_entity_prepare() when available.
    
    +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -7,41 +7,301 @@
    +    // @todo Exploit the upcoming hook_entity_prepare() when available.
    

    similar moving of an @todo (we can create the issue todo that, and add the link). (or do a follow-up to do all those @todo's moved in this issue, but not created by this issue)

  3. +++ b/core/modules/content_translation/lib/Drupal/content_translation/ContentTranslationHandler.php
    @@ -197,8 +197,11 @@ public function entityFormAlter(array &$form, array &$form_state, EntityInterfac
    -        foreach ($entity->translation as $translation) {
    -          $published += $translation['status'];
    +        // When creating a brand new translation, $entity->translation is not set.
    +        if (!($new_translation)) {
    +          foreach ($entity->translation as $translation) {
    

    comment should wrap at 80 chars.

    "Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over,"
    https://drupal.org/coding-standards/docs#drupal

  4. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -7,41 +7,301 @@
    +use Drupal\Core\Url;
    +use Drupal\field\FieldInfo;
    +use Drupal\Core\Language\Language;
    +use Symfony\Component\DependencyInjection\ContainerInterface;
    

    since all these use's are new, might as well add them in alphabetical order.

  5. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -7,41 +7,301 @@
    -   * @todo Remove content_translation_overview().
    

    wow. this seems pretty huge. I wonder if there was another issue for this @todo.

    This @todo came from #2089635: Convert non-test non-form page callbacks to routes and controllers.

    Nothing wrong here, just will link to that related issue.

  6. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -7,41 +7,301 @@
    +   * Translations overview page builder.
    ...
       public function overview(Request $request) {
    

    This is probably an english confusion.

    Standards say:
    "summary lines must start with a third person singular present tense verb, and they must explain what the function does. Example: "Calculates the maximum weight for a list.""
    https://drupal.org/node/1354#functions

    This first word does end in an 's" but it's not a verb saying what it does.

    fixing this to say "Builds ..."

  7. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -7,41 +7,301 @@
    +          $status = !empty($translation['status']) ? $this->t('Published') : $this->t('Not published');
    +          // @todo Add a theming function here.
    +          $status = '<span class="status">' . $status . '</span>' . (!empty($translation['outdated']) ? ' <span class="marker">' . $this->t('outdated') . '</span>' : '');
    

    this @todo either needs to be taken care of in this issue, or a separate issue should be opened for it, and a link added in the comment for that issue number.

    sigh. this code already existed, and is just being moved as part of this conversion. but still.. would be good to see if an issue already exists for this, and if not, make one.

  8. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -7,41 +7,301 @@
    -   * @todo Remove content_translation_add_page().
    +   * Translation addition page builder.
    

    similar third person verb change here.

  9. +++ b/core/modules/language/lib/Drupal/language/LanguageConverter.php
    @@ -0,0 +1,41 @@
    + * Contains Drupal\language\LanguageConverter.
    

    got distracted actually reading the patch.

    trying to get back to just nits, and then reread it later.

    here, \Drupal
    https://drupal.org/node/1354#file

posting just the nits, following is coming a review (just reading the patch)

YesCT’s picture

  1. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -7,41 +7,301 @@
    -   * @todo Remove content_translation_edit_page().
    +   * Page callback for the edit translation page.
    ...
    -  public function edit(Request $request, $language) {
    ...
    +  public function edit(Language $language, Request $request) {
    

    a.
    I was wondering about the re-ordering of parameters, and when I looked at other places in core, it does seem like when building page, we do put the request as the last param. ok.

    b.
    but, I do not see a default value for the $language, so this isn't optional...
    there are a lot of places like this. not fixing it here.

    Should do a separate patch/interdiff for those.

    c.
    I'm also super unsure about what the correct thing to do here is. Should these be "forms"?
    I dont see many other add edit methods on controllers in core, or as children of the convert meta this belongs to.

  2. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -7,41 +7,301 @@
    +    // @todo: $operation = isset($info['default_operation']) ? $info['default_operation'] : 'default';
    ...
    +     * @todo Decide what operation should we use?
    +     *
    +     * $info = $entity->entityInfo();
    +     * $operation = isset($info['default_operation']) ? $info['default_operation'] : 'default';
    

    this todo is a new one in this issue, is it intended to be fixed before this patch is committed?

    is it related to this other @todo?

  3. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    --- a/core/modules/content_translation/lib/Drupal/content_translation/Routing/ContentTranslationRouteSubscriber.php
    +++ b/core/modules/content_translation/lib/Drupal/content_translation/Routing/ContentTranslationRouteSubscriber.php
    
    +++ b/core/modules/content_translation/lib/Drupal/content_translation/Routing/ContentTranslationRouteSubscriber.php
    @@ -88,6 +88,12 @@ protected function alterRoutes(RouteCollection $collection, $provider) {
    +            'source' => array(
    +              'type' => 'language',
    +            ),
    

    hm. wonder if #2139135: Unit test \Drupal\config_translation\Routing\RouteSubscriber is relates or will be effected.

  4. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Tests/ContentTranslationUITest.php
    @@ -35,6 +35,7 @@
       function testTranslationUI() {
         $this->doTestBasicTranslation();
    +    return;
         $this->doTestTranslationOverview();
    

    huh?!

    we stop and do not do the rest of this test?

  5. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Tests/ContentTranslationUITest.php
    @@ -78,10 +79,11 @@ protected function doTestBasicTranslation() {
    -    $edit = array('source_langcode[source]' => $source_langcode);
    +    $edit = $this->getEditValues($values, $source_langcode);
    +    $edit['source_langcode[source]'] = $source_langcode;
         $path = $langcode . '/' . $content_translation_path . '/add/' . $default_langcode . '/' . $langcode;
         $this->drupalPostForm($path, $edit, t('Change'));
    
    +++ b/core/modules/content_translation/lib/Drupal/content_translation/Tests/ContentTranslationWorkflowsTest.php
    @@ -69,7 +69,12 @@ protected function setupEntity() {
    -    $this->drupalPostForm($add_translation_path, array(), t('Save'));
    +    $data = array (
    +      'name' => $this->randomName(16),
    +      'user_id' => $this->translator->id(),
    +      $this->fieldName . '[0][value]' => $this->randomName(16),
    +    );
    +    $this->drupalPostForm($add_translation_path, $data, t('Save'));
    

    why do we have to change this test? why do we have to provide data now?

  6. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Tests/ContentTranslationUITest.php
    @@ -78,10 +79,11 @@ protected function doTestBasicTranslation() {
    -    $this->assertFieldByXPath("//input[@name=\"{$this->fieldName}[0][value]\"]", $values[$source_langcode][$this->fieldName][0]['value'], 'Source language correctly switched.');
    +    $this->assertText('Source language set to: Italian');
    

    I dont understand why we have to change this part of the test either.

  7. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Tests/ContentTranslationWorkflowsTest.php
    @@ -176,7 +181,7 @@ protected function assertWorkflows(UserInterface $user, $expected_status) {
    -    $this->assertResponse($expected_status['edit_translation'], format_string('The @user_label has the expected translation creation access.', $args));
    +    $this->assertResponse($expected_status['edit_translation'], format_string('The @user_label has the expected translation edit access.', $args));
    

    ok. this is fixing an already existing copy and paste problem.

    really, maybe this should be a novice follow-up as it is not related to doing this conversion.

  8. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.php
    @@ -101,7 +101,7 @@ protected function createEntity($values, $langcode, $bundle_name = NULL) {
    -      return t('Save and keep published') . $this->getFormSubmitSuffix($entity, $langcode);
    +      return t('Save and publish') . $this->getFormSubmitSuffix($entity, $langcode);
    
    @@ -117,7 +117,7 @@ protected function doTestPublishedStatus() {
    -      t('Save and keep published'),
    +      t('Save and publish'),
    

    this implies we are not editing (saving something already published) but are creating... why changing from creating to editing?

YesCT’s picture

1. I was looking closely at just edit(). (should do the same for add())

1a.

-/**
- * Page callback for the translation edit page.
- *
- * @param \Drupal\Core\Entity\EntityInterface $entity
- *   The entity being translated.
- * @param \Drupal\Core\Language\Language $language
- *   (optional) The language of the translated values. Defaults to the current
- *   content language.
- *
- * @return array
- *   A processed form array ready to be rendered.
- *
- * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
- *   Use \Drupal\content_translation\Controller\ContentTranslationController::edit().
- */
-function content_translation_edit_page(EntityInterface $entity, Language $language = NULL) {
-  $language = !empty($language) ? $language : \Drupal::languageManager()->getCurrentLanguage(Language::TYPE_CONTENT);
-  $form_state['langcode'] = $language->id;
-  $form_state['content_translation']['translation_form'] = TRUE;
-  return \Drupal::service('entity.form_builder')->getForm($entity, 'default', $form_state);
-}

1b.

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
+   * Builds the edit translation page.
+   *
+   * @param \Drupal\language\Plugin\Condition\Language $language
+   *   (optional) The language of the translated values. Defaults to the current
+   *   content language.
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   The request object from which to extract the entity type.
+   *
+   * @return array
+   *   A processed form array ready to be rendered.
    */
-  public function edit(Request $request, $language) {
-    $entity = $request->attributes->get($request->attributes->get('_entity_type_id'));
-    module_load_include('pages.inc', 'content_translation');
-    $language = language_load($language);
-    return content_translation_edit_page($entity, $language);
+  public function edit(Language $language, Request $request) {
+    $entity_type = $request->attributes->get('_entity_type_id');
+    $entity = $request->attributes->get($entity_type);
+
+    /**
+     * @todo Decide what operation should we use?
+     *
+     * $info = $entity->entityInfo();
+     * $operation = isset($info['default_operation']) ? $info['default_operation'] : 'default';
+     */
+
+    $operation = 'default';
+    $form_state['langcode'] = $language->id;
+    $form_state['content_translation']['translation_form'] = TRUE;
+
+    return $this->entityFormBuilder()->getForm($entity, $operation, $form_state);
   }

2. wrong namespace for the Language class for the $language param.

3. Also, the method content_translation_edit_page from the .inc file had default values for the params. I dont know why we are changing that here.

--------------

4.
I started to do this:

diff --git a/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
index e644d31..c17ebc3 100644
--- a/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
+++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
@@ -277,7 +277,7 @@ public function add(Language $source, Language $target,  Request $request) {
   /**
    * Builds the edit translation page.
    *
-   * @param \Drupal\language\Plugin\Condition\Language $language
+   * @param \Drupal\Core\Language\Language $language
    *   (optional) The language of the translated values. Defaults to the current
    *   content language.
    * @param \Symfony\Component\HttpFoundation\Request $request
@@ -286,7 +286,7 @@ public function add(Language $source, Language $target,  Request $request) {
    * @return array
    *   A processed form array ready to be rendered.
    */
-  public function edit(Language $language, Request $request) {
+  public function edit(Language $language = NULL, Request $request) {
     $entity_type = $request->attributes->get('_entity_type_id');
     $entity = $request->attributes->get($entity_type);

4a. But I'm *really* not sure about the change to not have default values.

4b. Why did we make them required?

4c. Is that change what rippled through other changes here (including tests and ContentTranslationRouteSubscriper::alterRoutes() ?)

4d. And if they really are, then we should update the docs taking out (optional).

dawehner’s picture

I was wondering about the re-ordering of parameters, and when I looked at other places in core, it does seem like when building page, we do put the request as the last param. ok.

I do like using Request as last, as the passed in Language is more important. On top of that this makes it more consistent with the add method for example.

As talked in IRC, the default values are moved onto the routing object.

I'm also super unsure about what the correct thing to do here is. Should these be "forms"?
I dont see many other add edit methods on controllers in core, or as children of the convert meta this belongs to.

They are indeed something like entity forms, but they aren't really, as they wrap existing entity forms and aren't new ones. I consider the current code as okay for now.

this todo is a new one in this issue, is it intended to be fixed before this patch is committed?

The problem existed before. Nothing ensures that the 'default' operation is "default", but afaik all core entity types provide some form controller type called 'default'.
Improved the comment by better describe what is actually going on.

hm. wonder if #2139135: Unit test \Drupal\config_translation\Routing\RouteSubscriber is relates or will be effected.

Afaik not really. THis is the content translation module, not config.

huh?!

we stop and do not do the rest of this test?

This probably improves the performance of the tests but is not intended!

On top of that I removed the changes in the tests, because we really don't have to, ... additional improvements can always be done later.

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
32.07 KB

@yesct++

dawehner’s picture

FileSize
1.11 KB

Forgot the interdiff.

andypost’s picture

Status: Reviewed & tested by the community » Needs work

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

vijaycs85’s picture

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

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +MANCHESTER_2014_APRIL
FileSize
31.51 KB
penyaskito’s picture

Awesome work!

  1. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -7,41 +7,306 @@
    +          // @todo Add a theming function here.
    

    Do we need an issue here?

  2. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -7,41 +7,306 @@
    +   * @param \Drupal\Core\Language\Language $source
    

    Shouldn't we use LanguageInterface when possible?

vijaycs85’s picture

thanks @penyaskito. here is the update:

#156.1 - Added #2250841: Adding a inline template for content translation status
#156.2 - Fixed them all in ContentTranslationController.

Status: Needs review » Needs work

The last submitted patch, 157: 1987882-content_transaltion_controller-157.patch, failed testing.

vijaycs85’s picture

tim.plunkett’s picture

Status: Needs work » Needs review
penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

Everyone concerns have been already addressed.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/content_translation/content_translation.module
    @@ -885,3 +885,41 @@ function content_translation_save_settings($settings) {
    +  $links = language_negotiation_get_switch_links(Language::TYPE_CONTENT, $path);
    +  if (empty($links)) {
    +    // If content language is set up to fall back to the interface language,
    +    // then there will be no switch links for Language::TYPE_CONTENT, ergo we
    +    // also need to use interface switch links.
    +    $links = language_negotiation_get_switch_links(Language::TYPE_INTERFACE, $path);
    +  }
    +  return $links;
    +}
    

    No longer used and language_negotiation_get_switch_links() does not exist either (since #1862202: Objectify the language system). This patch removes all the usages so it should remove the function. Hah... I see this patch is not copying the code correctly - this is so stale... In HEAD this is calling \Drupal::languageManager()->getLanguageSwitchLinks. Anyhow we can just remove.

  2. +++ b/core/modules/content_translation/content_translation.module
    @@ -885,3 +885,41 @@ function content_translation_save_settings($settings) {
    +function content_translation_prepare_translation(EntityInterface $entity, Language $source, Language $target) {
    +  if ($entity instanceof ContentEntityInterface) {
    +    $source_translation = $entity->getTranslation($source->id);
    +    $entity->addTranslation($target->id, $source_translation->getPropertyValues());
    +  }
    

    This function is no longer used as this patch removes all the usages. Let's get rid.

  3. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -7,41 +7,306 @@
    +              $url = new Url(
    +                'field_ui.overview_' . $entity->getEntityTypeId(),
    +                array (
    +                  'bundle' => $entity->bundle(),
    +                )
    +              );
    +              $url = new Url('language.content_settings_page');
    +
    +              // Link directly to the fields tab to make it easier to find the
    +              // setting to enable translation on fields.
    +              $links['nofields'] = array(
    +                'title' => $this->t('No translatable fields'),
    +              ) + $url->toArray();
    

    Hmmm? I'm guessing the first $url assignment can be removed. But then again the comment says link the fields tab? What is desired here?

  4. +++ b/core/modules/language/lib/Drupal/language/LanguageConverter.php
    @@ -0,0 +1,58 @@
    +use Drupal\Core\Language\Language;
    

    Not used.

victor-shelepen’s picture

#162

  1. - It was deleted, it had not been used. It is to simple.
  2. - moved to the class defenition of the Controller.
  3. - Done. There were two variants because the first variant had been define at D7, but it crashed the related test. Second test passed.
  4. - Done.
victor-shelepen’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

victor-shelepen’s picture

The last submitted patch, 157: 1987882-content_transaltion_controller-157.patch, failed testing.

dawehner’s picture

  1. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Access/ContentTranslationManageAccessCheck.php
    @@ -66,8 +66,8 @@ public function access(Route $route, Request $request, AccountInterface $account
    -          $language = !empty($language) ? $language : \Drupal::languageManager()->getCurrentLanguage(Language::TYPE_CONTENT);
    ...
    +          $language = !empty($language) ? $language : language(Language::TYPE_CONTENT);
    

    This change seems to be a step back.

  2. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -7,41 +7,304 @@
    +          $link = isset($links->links[$langcode]['href']) ? $links->links[$langcode] : array ('href' => $entity->getSystemPath());
    +          $row_title = l($label, $link['href'], $link);
    

    I wonder whether we can already use the some route here?

  3. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Controller/ContentTranslationController.php
    @@ -7,41 +7,304 @@
    +      $header = array(t('Language'), t('Translation'), t('Source language'), t('Status'), t('Operations'));
    ...
    +      $header = array(t('Language'), t('Translation'), t('Status'), t('Operations'));
    

    Those ones could be $this->t()-ified

victor-shelepen’s picture

The comment #169 has been processed.
1, 3 - resolved.

2 - It invokes some logic problems. Entity has methods: url and getSystemRoute. But They return a string not a route object. Yes we can generate an Url object from url-string and than to a renderable array. It is weird.

Status: Needs review » Needs work

The last submitted patch, 170: content_translation-add_page-1987882-168-170.diff, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
32.65 KB
13.19 KB

Did a re-roll for PSR4. Also injected some more dependencies and updated for some recent Entity API changes. Code style fixes too.

Status: Needs review » Needs work

The last submitted patch, 172: 1987882-content-translation-route-172.2.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
33.29 KB
657 bytes

Forgot to add @language_manager to the service definition.

Crell’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/content_translation/src/Access/ContentTranslationManageAccessCheck.php
    @@ -58,19 +69,19 @@ public function __construct(EntityManagerInterface $manager) {
    +          $source = $request->attributes->get('source') ? : $entity->language();
    +          $target = $request->attributes->get('target') ? : $this->languageManager->getCurrentLanguage(Language::TYPE_CONTENT);
    

    Er, shouldn't this just be gotten from the method parameters?

  2. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -7,41 +7,325 @@
    +    $entity_type = $request->attributes->get('_entity_type_id');
    

    We're trying to eliminate access to request attributes from controllers et al. What's _entity_type_id? If it's an attribute, you can just get it in the method signature by having a parameter of the same name.

    Which suggests it should be entity_type_id, no _.

The controller method is way too long and involved, but that's just a straight port of the old callback function so we shouldn't block on that.

katbailey’s picture

Status: Needs work » Needs review
FileSize
33.07 KB
2.25 KB

This addresses Crell's first point. I have no idea how to address the second :(

I did notice that the delete link was using the edit route so I fixed that. Oh and renamed a variable.

Status: Needs review » Needs work

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

kim.pepper’s picture

Assigned: Unassigned » kim.pepper
  1. I think we need to check if there is a value in the request, and set it otherwise. I believe crell was referring to using $request->attributes() when we should be passing it in anyway
  2. I have a fix for this
kim.pepper’s picture

Status: Needs work » Needs review
FileSize
34.62 KB
6.38 KB

Fixes for #175

Status: Needs review » Needs work

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
34.22 KB
8.4 KB

Discussed with tim.plunkett and current convention is to use params with underscores (e.g. $_entity_type_id) rather than pulling it out of $request->attributes.

This reverts my previous changes of using $entity_type instead of $entity_type_id

Status: Needs review » Needs work

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
34.57 KB
1.98 KB

I removed the call to the procedural content_translation_controller() and instead call $this->entityManager->getController($entity->getEntityTypeId(), 'translation');

Also, fixed an issue where strings are being passed in, but the code still expects a language object.

Status: Needs review » Needs work

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
40.04 KB
8.37 KB

This fixes a number of issues, including loading languages correctly, ensuring that translation routes are admin paths, and using ->getId() instead of the property directly.

Also, the logic in ContentTranslationManageAccessCheck is pretty complex, so adding a unit test in here.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@kim: let's keep focusing on the scope of this issue. If you make changes unrelated it makes it harder to review and adds more to debate about, that is farther from the goal. See #2276387: Translate routes should properly inherit admin path use from edit route for an RTBC issue that makes the translation paths admin or not admin based on proper admin status inheritance from the edit route. That is the correct solution to that problem. Eg. comments don't have any admin pages, if you edit them, delete them, etc. are all frontend, so the translation should be as well. Please remove these unrelated changes. Let's focus on making the controller conversion happen. This has been lingering for so so long.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
39.37 KB
2.3 KB

@Gabor, sorry. I thought it was a mistake.

This also puts back in a fix the @katbailey made in #176 that somehow I left out in my re-roll.

Status: Needs review » Needs work

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
28.77 KB
712 bytes

Re-roll after #2246665: Typehint with Drupal\Core\Language\LanguageInterface instead Drupal\Core\Language\Language went in.

Also, fixes "Route "content_translation.translation_delete_node" does not exist." test fails.

YesCT’s picture

  1. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -7,41 +7,330 @@
    +    // @todo Exploit the upcoming hook_entity_prepare() when available.
    +    $this->prepareTranslation($entity, $source, $target);
    

    does this refer to #1537452: Generalize hook_node_prepare() with hook_entity_prepare()
    or maybe referring to hook_entity_prepare_form() now
    or
    EntityForm::prepareEntity()
    ?

  2. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -7,41 +7,330 @@
    +    // @todo: $operation = isset($info['default_operation']) ? $info['default_operation'] : 'default';
    +    $operation = 'default';
    

    I asked in #145 and @dawehner answered in #147:

    this todo is a new one in this issue, is it intended to be fixed before this patch is committed?

    The problem existed before. Nothing ensures that the 'default' operation is "default", but afaik all core entity types provide some form controller type called 'default'.
    Improved the comment by better describe what is actually going on.

    but the comment is still there.
    Is there an issue we can link to in the todo to ensure there is a default operation?

    Do we need to create the issue,
    or just take out the @todo, and continue to assume it's always 'default'?

  3. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -7,41 +7,330 @@
    +
    +    $operation = 'default';
    

    looks like this is the same todo. So maybe I should just open the issue and replace these two @todos.

    Is this related to #2006348: Remove default/fallback entity form operation ?

    Well, made #2284043: Provide a way to figure out the default form operation with a default_operation annotation on the entity.

===============
small clean ups and removing out of scope changes.

kim.pepper’s picture

I noticed the Language param converter isn't converting the language params for the access checker. Not sure if it's meant to? or is it just for the routes?

kim.pepper’s picture

Assigned: kim.pepper » Unassigned

Sorry, I don't really have answers to the questions in #190. Unassigning.

plach’s picture

Status: Needs review » Needs work

@YesCT:

1: That should refer to #1810394: Site configuration with domain based language negotiation results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface language.
2: That should refer to #2006348: Remove default/fallback entity form operation.
3: Yes, those comment should match and refer to #2006348: Remove default/fallback entity form operation.

  1. +++ b/core/modules/content_translation/src/Access/ContentTranslationManageAccessCheck.php
    @@ -52,38 +63,42 @@ public function __construct(EntityManagerInterface $manager) {
    +   * @param string $_entity_type_id
    

    Why we have an underscore prefix? Is this a WSCCI convention?

  2. +++ b/core/modules/content_translation/src/Access/ContentTranslationManageAccessCheck.php
    @@ -52,38 +63,42 @@ public function __construct(EntityManagerInterface $manager) {
    +          $source_language = $this->languageManager->getLanguage($source) ?: $entity->language();
    +          $target_language = $this->languageManager->getLanguage($target) ?: $this->languageManager->getCurrentLanguage(LanguageInterface::TYPE_CONTENT);
    

    In other parts of this patch we are using just $source and $target. Please let's be consistent and pick one of the two forms.

  3. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
    @@ -197,8 +197,12 @@ public function entityFormAlter(array &$form, array &$form_state, EntityInterfac
    +        if (!($new_translation)) {
    

    Those parentheses wrapping the variable are unnecessary, are they supposed to improve readability?

  4. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -7,41 +7,332 @@
    +    $controller = $this->entityManager()
    

    We are inside a routing controller, please let's call the variable $handler to avoid confusion :)

  5. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -7,41 +7,332 @@
    +          $rows[] = array($language_name, $row_title, $status, $operations);
    

    For consistency let's use a multiline array here too.

  6. +++ b/core/modules/language/src/LanguageConverter.php
    @@ -0,0 +1,57 @@
    +    if (!empty($definition['type']) && $definition['type'] == 'language') {
    

    We can simplify this code by just returning !empty($definition['type']) && $definition['type'] == 'language'

kim.pepper’s picture

  1. Yes. I discussed this with @tim.plunkett and we're using underscores for parameters that are not part of the request.
penyaskito’s picture

Status: Needs work » Needs review
FileSize
28.05 KB
3.07 KB

Reroll of #190.

I'm not sure if we should use 'entity' or $entity_type_id, but tests will tell.

Status: Needs review » Needs work

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

penyaskito’s picture

Status: Needs work » Needs review
FileSize
28.43 KB
1.33 KB

It should be $entity_type_id. One test namespace was wrong too, this one should be green.

Status: Needs review » Needs work

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

penyaskito’s picture

Status: Needs work » Needs review
FileSize
28.5 KB
690 bytes

And now after changing the namespace the use statement is missing.

penyaskito’s picture

#193.2: $source and $target are already used on this scope. Do we really want to override them?
I don't have answers for #191, sorry.

The attached patch adds the issue urls for todos, and fixes #193.3, 4 and 6. The other remarks in #193 were already answered or not relevant anymore.

tim.plunkett’s picture

  1. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -7,41 +7,332 @@
    +class ContentTranslationController extends ControllerBase implements ContainerInjectionInterface {
    

    This implements ContainerInjectionInterface is redundant

  2. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -7,41 +7,332 @@
    +   * The field info service.
    +   *
    +   * @var \Drupal\Core\Entity\EntityManagerInterface
    ...
    +  protected $entityManager;
    +
    +  /**
    +   * The language manager.
    +   *
    +   * @var \Drupal\Core\Language\LanguageManagerInterface
    +   */
    +  protected $languageManager;
    +
    +  /**
    +   * Constructs a ContentTranslationController object.
    +   *
    +   * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
    +   *   The entity manager service.
    +   * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
    +   *   The language manager.
    +   */
    +  public function __construct(EntityManagerInterface $entity_manager, LanguageManagerInterface $language_manager) {
    +    $this->entityManager = $entity_manager;
    +    $this->languageManager = $language_manager;
    ...
    +  public static function create(ContainerInterface $container) {
    +    return new static(
    +      $container->get('entity.manager'),
    +      $container->get('language_manager')
    +    );
    

    You don't need to inject either of these, it's provided by ControllerBase.

  3. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -7,41 +7,332 @@
    +    if ($entity instanceof ContentEntityInterface) {
    

    When is this not true?

  4. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -7,41 +7,332 @@
    +    $handler = $this->entityManager()
    +      ->getController($_entity_type_id, 'translation');
    ...
    +    $field_ui = $this->moduleHandler()->moduleExists('field_ui')
    +      && $account->hasPermission('administer ' . $_entity_type_id . ' fields');
    ...
    +    return $this->entityFormBuilder()
    +      ->getForm($entity, $operation, $form_state);
    ...
    +    return $this->entityFormBuilder()
    +      ->getForm($entity, $operation, $form_state);
    

    Please put these on one line.

  5. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -7,41 +7,332 @@
    +        $links = & $operations['data']['#links'];
    

    No space between the & and $

  6. +++ b/core/modules/content_translation/tests/src/Access/ContentTranslationManageAccessCheckTest.php
    @@ -0,0 +1,101 @@
    + * Tests for content translation manage check.
    ...
    +  public static function getInfo() {
    

    We don't use getInfo() anymore, please use an annotation on the class.

penyaskito’s picture

Thanks for reviewing Tim.

Attended every comment from #201. For #201.3, I think it is never true in core (see ContentTranslationRouteSubscriber) so I removed the if clause.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

I think this is good to go. There's still some ugly code in here, but that's just being moved around. The new bits all look good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -7,41 +7,289 @@
    +use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
    ...
    +use Drupal\Core\Entity\EntityManagerInterface;
    ...
    +use Drupal\Core\Language\LanguageManagerInterface;
    ...
    +use Symfony\Component\DependencyInjection\ContainerInterface;
    

    Not used

  2. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -7,41 +7,289 @@
    +  public function prepareTranslation(EntityInterface $entity, LanguageInterface $source, LanguageInterface $target) {
    

    Couldn't we type hint here on ContentEntityInterface?

  3. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -7,41 +7,289 @@
    +    $original = $entity->getUntranslated()->language()->id;
    ...
    +        $langcode = $language->id;
    ...
    +            'target' => $language->id,
    ...
    +            'language' => $language->id,
    ...
    +            'language' => $language->id,
    ...
    +          $source = $entity->language()->id;
    ...
    +    $form_state['langcode'] = $language->id;
    

    use ->getid() instead of ->id

kim.pepper’s picture

Assigned: Unassigned » kim.pepper

These look like quick fixes. I can take a look.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
27.2 KB
5.07 KB

Fixes for #204.

penyaskito’s picture

Rerolled #206, automatic merge.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

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

penyaskito’s picture

A route name was wrong, not sure what changed or why didn't fail before.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

so back to rtbc

Crell’s picture

Status: Reviewed & tested by the community » Needs work

Let's go ahead and remove the page callback function we're replacing, too. Should be an easy reroll.

kim.pepper’s picture

Assigned: kim.pepper » Unassigned
Status: Needs work » Needs review
FileSize
38.77 KB
10.92 KB

This removes content_translation.pages.inc which contains the callback functions as per #212 as well as helper functions that were used by those callbacks.

andypost’s picture

Great clean-up! Just minor nitpicks, form state uses methods consistently in core

  1. +++ b/core/modules/content_translation/src/Access/ContentTranslationManageAccessCheck.php
    @@ -52,38 +63,42 @@ public function __construct(EntityManagerInterface $manager) {
    +   * @param string $_entity_type_id
    ...
    +  public function access(Route $route, Request $request, AccountInterface $account, $source = NULL, $target = NULL, $language = NULL, $_entity_type_id = NULL) {
    ...
    +    if ($entity = $request->attributes->get($_entity_type_id)) {
    
    +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -7,41 +7,285 @@
    +   * @param string $_entity_type_id
    ...
    +  public function overview(Request $request, $_entity_type_id = NULL) {
    ...
    +    $handler = $this->entityManager()->getController($_entity_type_id, 'translation');
    ...
    +   * @param string $_entity_type_id
    ...
    +  public function add(LanguageInterface $source, LanguageInterface $target, Request $request, $_entity_type_id = NULL) {
    ...
    +   * @param string $_entity_type_id
    ...
    +  public function edit(LanguageInterface $language, Request $request, $_entity_type_id = NULL) {
    

    any reason for underscore prefix? $entity_type_id all over core

  2. +++ b/core/modules/content_translation/src/Access/ContentTranslationManageAccessCheck.php
    @@ -52,38 +63,42 @@ public function __construct(EntityManagerInterface $manager) {
    +      /* @var \Drupal\content_translation\ContentTranslationHandlerInterface $controller */
    +      $controller = $this->entityManager->getController($entity->getEntityTypeId(), 'translation');
    

    let's name it properly - $handler

  3. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -7,41 +7,285 @@
    +    $form_state['langcode'] = $target->getId();
    +    $form_state['content_translation']['source'] = $source;
    +    $form_state['content_translation']['target'] = $target;
    +    $form_state['content_translation']['translation_form'] = !$entity->access('update');
    ...
    +    $form_state['langcode'] = $language->getId();
    +    $form_state['content_translation']['translation_form'] = TRUE;
    

    please, use methods

penyaskito’s picture

Fixed #214.1, 214.2.

About 214.3, it is not a FormStateInterface object but $form_state_addition array. Renamed the variable for clarifying.
This is confusing, should we create another issue for changing that parameter to a proper FormStateInterface?

Status: Needs review » Needs work

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

penyaskito’s picture

This should work, the routing is dynamically generated and forgot to look at the subscriber.

Status: Needs review » Needs work

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

penyaskito’s picture

penyaskito’s picture

Status: Needs work » Needs review
kim.pepper’s picture

any reason for underscore prefix? $entity_type_id all over core

This is a symfony convention. For route callbacks, any special request attribute that is not a parameter has an underscore prefix.

penyaskito’s picture

@kim.pepper: You are right about Symfony convention. I did a quick research on core code and we are not really enforcing this convention.
Most of the request attributes don't use underscored names. I would say that we move forward on this issue, and if we want to discuss a policy about this let's open a new issue. There aren't too much cases looking at $request->attributes->get() calls.

EDIT: s/All/Most of.

Crell’s picture

It's a little more subtle than that. _-prefixed attributes are reserved for things that have special magical meaning to the system. Unprefixed values that are "meant" to be passed to the controller as arguments are fine.

andypost’s picture

Nice clean-up, first!

Now the code uncover another wtf - how you get entity from entity_type_id and what it's actually is?

  1. +++ b/core/modules/content_translation/src/Access/ContentTranslationManageAccessCheck.php
    @@ -52,39 +63,43 @@ public function __construct(EntityManagerInterface $manager) {
    +   * @param string $entity_type_id
    +   *   (optional) The entity type ID.
    ...
    +    /* @var $entity \Drupal\Core\Entity\ContentEntityInterface */
    +    if ($entity = $request->attributes->get($entity_type_id)) {
    ...
    +      /* @var \Drupal\content_translation\ContentTranslationHandlerInterface $handler */
    +      $handler = $this->entityManager->getController($entity->getEntityTypeId(), 'translation');
    

    is this an entity ID or entity type ID?

    Also order of name and type in @var is deffirent #2305593: [policy] Set a standard for @var inline variable type declarations

  2. +++ b/core/modules/content_translation/src/Access/ContentTranslationOverviewAccess.php
    @@ -46,7 +46,7 @@ public function __construct(EntityManagerInterface $manager) {
    -    $entity_type = $request->attributes->get('_entity_type_id');
    +    $entity_type = $request->attributes->get('entity_type_id');
         if ($entity = $request->attributes->get($entity_type)) {
    

    the logic here is different...

  3. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
    @@ -7,41 +7,289 @@
    +   * @param string $entity_type_id
    +   *   (optional) The entity type ID.
    ...
    +  public function overview(Request $request, $entity_type_id = NULL) {
    +    $entity = $request->attributes->get($entity_type_id);
    ...
    +  public function add(LanguageInterface $source, LanguageInterface $target, Request $request, $entity_type_id = NULL) {
    +    $entity = $request->attributes->get($entity_type_id);
    ...
    +   * @param string $entity_type_id
    +   *   (optional) The entity type ID.
    ...
    +    $entity = $request->attributes->get($entity_type_id);
    

    the same

penyaskito’s picture

Fixed the @var order.

The difference between the access code and the controller is because access subscriber runs with a higher priority than convert params subscriber, so the conversion is not done when access code runs.

This should be ready to RTBC.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Suppose it's ready

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 77c666c and pushed to 8.0.x. Thanks!

diff --git a/core/modules/content_translation/src/Controller/ContentTranslationController.php b/core/modules/content_translation/src/Controller/ContentTranslationController.php
index d4efb59..8d5a306 100644
--- a/core/modules/content_translation/src/Controller/ContentTranslationController.php
+++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
@@ -11,7 +11,6 @@
 use Drupal\Core\Entity\ContentEntityInterface;
 use Drupal\Core\Language\LanguageInterface;
 use Drupal\Core\Url;
-use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\HttpFoundation\Request;
 
 /**

Fixed during commit.

  • alexpott committed 77c666c on 8.0.x
    Issue #1987882 by disasm, vijaycs85, penyaskito, kim.pepper, likin,...
Gábor Hojtsy’s picture

Status: Fixed » Closed (fixed)

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