Files: 
CommentFileSizeAuthor
#92 urlgenerator-1946426-92.patch1.39 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,435 pass(es).
[ View ]
#88 language-1946426-88.patch11.72 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,069 pass(es).
[ View ]
#88 interdiff.txt5.2 KBtim.plunkett
#87 1946426-language-config-form-87.patch11.92 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#87 interdiff.txt4.6 KBpfrenssen
#82 1946426-language-config-form-82.patch12.81 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 58,018 pass(es).
[ View ]
#82 interdiff.txt4.77 KBpfrenssen
#80 1946426-language-config-form-80.patch12.1 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 58,150 pass(es).
[ View ]
#80 interdiff.txt4.78 KBpfrenssen
#77 1946426-language-config-form-77.patch12.12 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 57,840 pass(es).
[ View ]
#77 interdiff.txt1.24 KBpfrenssen
#76 1946426-language-config-form-76.patch11.83 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 58,264 pass(es).
[ View ]
#76 interdiff.txt6.16 KBpfrenssen
#72 1946426-language-config-form-72.patch11.16 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 58,051 pass(es).
[ View ]
#72 interdiff.txt11.04 KBpfrenssen
#64 1946426-language-config-form-64.patch13.13 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 57,855 pass(es), 41 fail(s), and 2 exception(s).
[ View ]
#64 1946426-diff-62-64.txt1.55 KBvijaycs85
#62 1946426-language-config-form-62.patch13.22 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 57,792 pass(es), 41 fail(s), and 2 exception(s).
[ View ]
#57 drupal-convert_language_confirm_form-1946426-57.patch14.91 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 56,304 pass(es), 44 fail(s), and 18 exception(s).
[ View ]
#55 convert_language_confirm_form-1946426-55.patch14.93 KBwebflo
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert_language_confirm_form-1946426-55.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#55 interdiff.patch1.01 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 56,394 pass(es).
[ View ]
#47 convert_language_confirm_form-1946426-47.patch14.97 KBwebflo
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert_language_confirm_form-1946426-47.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#43 convert_language_confirm_form-1946426-43.patch14.29 KBwebflo
FAILED: [[SimpleTest]]: [MySQL] 58,534 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#43 interdiff-1946426-34-43.txt5.37 KBwebflo
#42 convert_language_confirm_form-1946426-41.patch14.87 KBramlev
FAILED: [[SimpleTest]]: [MySQL] 57,956 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#42 interdiff.txt4.64 KBramlev
#41 convert_language_confirm_form-1946426-41.patch14.87 KBramlev
FAILED: [[SimpleTest]]: [MySQL] 57,964 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#41 interdiff.txt4.64 KBramlev
#34 convert_language_confirm_form-1946426-34.patch15.63 KBramlev
FAILED: [[SimpleTest]]: [MySQL] 57,936 pass(es), 40 fail(s), and 16 exception(s).
[ View ]
#34 interdiff.txt2.71 KBramlev
#30 convert_language_confirm_form-1946426-30.patch12.77 KBDavid Hernández
FAILED: [[SimpleTest]]: [MySQL] 58,345 pass(es), 37 fail(s), and 129 exception(s).
[ View ]
#30 interdiff.txt1.75 KBDavid Hernández
#28 convert_language_confirm_form-1946426-28.patch12.88 KBDavid Hernández
FAILED: [[SimpleTest]]: [MySQL] 58,014 pass(es), 36 fail(s), and 16 exception(s).
[ View ]
#28 interdiff.txt1.59 KBDavid Hernández
#26 convert_language_confirm_form-1946426-25.patch12.6 KBramlev
FAILED: [[SimpleTest]]: [MySQL] 58,194 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#26 interdiff.txt1.93 KBramlev
#23 convert_language_confirm_form-1946426-22.patch12.05 KBramlev
FAILED: [[SimpleTest]]: [MySQL] 58,574 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#18 convert_language_confirm_form-1946426-18.patch11.94 KBramlev
FAILED: [[SimpleTest]]: [MySQL] 58,241 pass(es), 9 fail(s), and 4 exception(s).
[ View ]
#14 convert_language_confirm_form-1946426-14.patch11.73 KBfoopang
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert_language_confirm_form-1946426-14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 convert_language_confirm_form-1946426-11.patch12.17 KBfoopang
FAILED: [[SimpleTest]]: [MySQL] 53,850 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#8 convert_language_confirm_form-1946426-8.patch11.97 KBfoopang
FAILED: [[SimpleTest]]: [MySQL] 53,867 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#4 convert_language_confirm_form-1946426-4.patch11.92 KBfoopang
FAILED: [[SimpleTest]]: [MySQL] 53,823 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#2 convert_language_confirm_form-1946426-2.patch11.93 KBfoopang
FAILED: [[SimpleTest]]: [MySQL] 53,737 pass(es), 3 fail(s), and 23 exception(s).
[ View ]

Comments

Assigned:Unassigned» foopang

I'll take this task :D

Status:Active» Needs review
StatusFileSize
new11.93 KB
FAILED: [[SimpleTest]]: [MySQL] 53,737 pass(es), 3 fail(s), and 23 exception(s).
[ View ]

Initial patch attached, probably need to fix the route access control.

Status:Needs review» Needs work

The last submitted patch, convert_language_confirm_form-1946426-2.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new11.92 KB
FAILED: [[SimpleTest]]: [MySQL] 53,823 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Fix: Argument 3 passed to Drupal\language\Form\LanguageAdminDeleteForm::buildForm() must be an instance of Drupal\Core\Language\Language, string given

Status:Needs review» Needs work
Issue tags:-FormInterface, -WSCCI-conversion

The last submitted patch, convert_language_confirm_form-1946426-4.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+FormInterface, +WSCCI-conversion

The last submitted patch, convert_language_confirm_form-1946426-4.patch, failed testing.

StatusFileSize
new11.97 KB
FAILED: [[SimpleTest]]: [MySQL] 53,867 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Removed trailing whitespace, rerolled the patch.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, convert_language_confirm_form-1946426-8.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new12.17 KB
FAILED: [[SimpleTest]]: [MySQL] 53,850 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Help! I don't see how it can't pass the test.

Status:Needs review» Needs work

The last submitted patch, convert_language_confirm_form-1946426-11.patch, failed testing.

#1939660: Use YAML as the primary means for service registration went in, you'll have to make it a language.services.yml now instead of LanguageBundle.

Status:Needs work» Needs review
StatusFileSize
new11.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert_language_confirm_form-1946426-14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

OK, I've fixed the patch.

Status:Needs review» Needs work
Issue tags:-FormInterface, -WSCCI-conversion

The last submitted patch, convert_language_confirm_form-1946426-14.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+FormInterface, +WSCCI-conversion

The last submitted patch, convert_language_confirm_form-1946426-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new11.94 KB
FAILED: [[SimpleTest]]: [MySQL] 58,241 pass(es), 9 fail(s), and 4 exception(s).
[ View ]

Have changed the path, added the Request object as parameter to the delete forms.

Issue tags:+D8MI, +language-base

Tagging with some D8MI tags.

Issue tags:+sprint

Also put on sprint.

Status:Needs review» Needs work

+++ b/core/modules/language/language.services.ymlundefined
@@ -5,3 +5,7 @@ services:
+  access_check.language.edit_or_delete:
+    class: Drupal\language\Access\EditOrDeleteAccessCheck
+    tags:
+      - { name: access_check }

I assume this is planned to be reused as an Edit access controller in the other related issue :)

+++ b/core/modules/language/lib/Drupal/language/Form/AdminDeleteForm.phpundefined
@@ -0,0 +1,99 @@
+    if (language_default()->langcode == $langcode) {
+      drupal_set_message(t('The default language cannot be deleted.'));
+      drupal_goto('admin/config/regional/language');
+    }

drupal_goto() just got removed recently; See https://drupal.org/node/2023537 - you'll need the properly injected version applied to this situation

if you look at the removed code, it uses the procedural approach (post drupal_goto()), this added code is a bit older actually

I also checked the code otherwise but I did not find other outdated snippets in the added code (which is the main danger with patches like this one).

Assigned:foopang» ramlev

Status:Needs work» Needs review
StatusFileSize
new12.05 KB
FAILED: [[SimpleTest]]: [MySQL] 58,574 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Have removed the drupal_goto() method and implemented the RedirectReponse.

I have tried to create an interdiff.txt file, but i cant apply the old patch to the code, so it's kinda difficult to create.

I think you'd need the *properly injected* example from the change notice, this is a controller that we are doing the redirection from. You can talk to @katbailey or @YesCT about examples for that if you need more guidance.

Status:Needs review» Needs work

The last submitted patch, convert_language_confirm_form-1946426-22.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.93 KB
new12.6 KB
FAILED: [[SimpleTest]]: [MySQL] 58,194 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Annnnd i tihnk i understand DI a bit more now :)

The implementation is "properly injected" now i hope.

Status:Needs review» Needs work

The actual code changes look good, there are missing docs and whitespace.

+++ b/core/modules/language/lib/Drupal/language/Form/AdminDeleteForm.phpundefined
@@ -25,6 +28,19 @@ class AdminDeleteForm extends ConfirmFormBase {
+  protected $urlGenerator;
+
+  public function __construct(PathBasedGeneratorInterface $url_generator = NULL) {
+    $this->urlGenerator = $url_generator;
+  }
+
+
+  public static function create(ContainerInterface $container) {
+    return new static(
+      $container->get('url_generator')
+    );
+  }

- All need docblocks.

- The constructor should call the parent constructor as well after the url generator is saved.

- There is extra whitespace before create() that should not be there.

Status:Needs work» Needs review
StatusFileSize
new1.59 KB
new12.88 KB
FAILED: [[SimpleTest]]: [MySQL] 58,014 pass(es), 36 fail(s), and 16 exception(s).
[ View ]

Here we go. Fixed the extra whitespaces, added comments and called parent constructor.

Status:Needs review» Needs work

+++ b/core/modules/language/lib/Drupal/language/Access/EditOrDeleteAccessCheck.phpundefined
@@ -0,0 +1,35 @@
+   * Implements AccessCheckInterface::applies().
...
+   * Implements AccessCheckInterface::access().
+++ b/core/modules/language/lib/Drupal/language/Form/AdminDeleteForm.phpundefined
@@ -0,0 +1,131 @@
+   * Implements \Drupal\Core\Controller\ControllerInterface::create().

These should be {@inheritdoc} like the others in the patch.

+++ b/core/modules/language/lib/Drupal/language/Form/AdminDeleteForm.phpundefined
@@ -0,0 +1,131 @@
+   * The urlGenerator container.

Is this a container really? I think it is a service, no?

StatusFileSize
new1.75 KB
new12.77 KB
FAILED: [[SimpleTest]]: [MySQL] 58,345 pass(es), 37 fail(s), and 129 exception(s).
[ View ]

I hope that's fixed now.

Status:Needs work» Needs review

Updated status

Status:Needs review» Reviewed & tested by the community

All right, those look good to me. Great collaboration, thanks all!

Status:Reviewed & tested by the community» Needs work

The last submitted patch, convert_language_confirm_form-1946426-30.patch, failed testing.

StatusFileSize
new2.71 KB
new15.63 KB
FAILED: [[SimpleTest]]: [MySQL] 57,936 pass(es), 40 fail(s), and 16 exception(s).
[ View ]

Have fixed the bug in the tests, so everything should be ready for merging.

Status:Needs work» Needs review

Why would we need the q there, I don't see.

I dont either, but can figure out where it get the q from, it's, as fas i have been able to search back, somewhere in either RegionalForm or SystemConfigFormBase classes submitForm methods, because when the form is submitted the q parameter is attached (also through the browser)

Status:Needs review» Needs work

The last submitted patch, convert_language_confirm_form-1946426-34.patch, failed testing.

It seems that the only thing missing right now is to get LanguageTestController test case to work.

The issue here and where im stuck is that the test case test three links created by the language_test module, and these links should have an active class added to the anchor tag when then the correct language is selected, but seems, that no active class is set on anchor tags, dont really know to fix this problem.

Assigned:ramlev» webflo

I am working on it.

StatusFileSize
new4.64 KB
new14.87 KB
FAILED: [[SimpleTest]]: [MySQL] 57,964 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

Heres the patch which fixes a few more issues, but missing the LanguageSwitchingTest case fix.

StatusFileSize
new4.64 KB
new14.87 KB
FAILED: [[SimpleTest]]: [MySQL] 57,956 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

And heres last attempt on the patch.

Status:Needs work» Needs review
StatusFileSize
new5.37 KB
new14.29 KB
FAILED: [[SimpleTest]]: [MySQL] 58,534 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This patch is based on ramlevs from comment 34.

I had to changed the behaviour for the test. Because language is not an config entity and we dont have upcasting for non entity objects. We will change the behaviour to "Not found" after langauge is a config entity.

     $this->drupalGet('admin/config/regional/language/delete/' . $langcode);
-    $this->assertResponse(404, 'Language no longer found.');
+    $this->assertResponse(403, 'Language no longer found.');
     // Make sure the "language_count" variable has been updated correctly.

About #43, i have made an attempt to fix that in #42 by checking for if the language is existing, if not, throw an 404. By checking for 403 we're getting an access denied, and that could be another issue i think.

The testcase which is failing right now is the LanguageSwitchingTest with the 'active' class on anchor tags missing

Status:Needs review» Needs work

The last submitted patch, convert_language_confirm_form-1946426-43.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert_language_confirm_form-1946426-47.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Changed the response code assertion in LocaleTranslationUiTest.

I applied the patch and got this error when trying to delete a language:

Notice: Undefined index: language_admin_delete_form in drupal_retrieve_form() (line 824 of core/includes/form.inc).
Warning: call_user_func_array() expects parameter 1 to be a valid callback, function 'language_admin_delete_form' not found or invalid function name in drupal_retrieve_form() (line 868 of core/includes/form.inc).

on the "Confirm delete" page.

Are you sure you properly applied the patch? We have test coverage for deleting a language and the form ID is provided in the class. Can you try with the patch applied and then a fresh install?

I have just cloned 8.x from git.drupal.org applied the patch and added the danish language. After that deleted that language with no errors or warnings.

Status:Needs review» Reviewed & tested by the community

Yay, get this in then!

Yep. All good with super fresh install (patch applied first)! RTBC++

Status:Reviewed & tested by the community» Needs work

This will not pass anymore due to the langcode => id property change.

Status:Needs work» Needs review
StatusFileSize
new1.01 KB
PASSED: [[SimpleTest]]: [MySQL] 56,394 pass(es).
[ View ]
new14.93 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert_language_confirm_form-1946426-55.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled after #1754246: Languages should be configuration entities got it. Changed langcode to id. The interdiff is a diff between to patch files.

Status:Needs work» Needs review
StatusFileSize
new14.91 KB
FAILED: [[SimpleTest]]: [MySQL] 56,304 pass(es), 44 fail(s), and 18 exception(s).
[ View ]

I couldn't find a commit the patch in #55 applied to, so rerolled 47. Attached.

Status:Needs review» Needs work

+++ b/core/modules/language/lib/Drupal/language/Access/EditOrDeleteAccessCheck.phpundefined
@@ -0,0 +1,36 @@
+  public function access(Route $route, Request $request) {
...
+    return FALSE;

Related #2031277: Implement checkCreateAccess on LanguageAccessController

+++ b/core/modules/language/lib/Drupal/language/Form/AdminDeleteForm.phpundefined
@@ -0,0 +1,128 @@
+class AdminDeleteForm extends ConfirmFormBase implements ControllerInterface {
...
+  protected $language;

Should be EntityControllerInterface because language now config entity

Also please name it LanguageDeleteForm for consistency

Status:Needs work» Postponed

Status:Postponed» Needs work

#2005778: Convert language_admin_overview_form to a Controller landed, including a LanguageAccessController, so I think this is not postponed anymore.

Issue tags:+API change, +API clean-up

Add API change tags.

Status:Needs work» Needs review
StatusFileSize
new13.22 KB
FAILED: [[SimpleTest]]: [MySQL] 57,792 pass(es), 41 fail(s), and 2 exception(s).
[ View ]

Re-rolling...

Status:Needs review» Needs work

The last submitted patch, 1946426-language-config-form-62.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.55 KB
new13.13 KB
FAILED: [[SimpleTest]]: [MySQL] 57,855 pass(es), 41 fail(s), and 2 exception(s).
[ View ]

Line ending fix...

Status:Needs review» Needs work

The last submitted patch, 1946426-language-config-form-64.patch, failed testing.

  1. @@ -95,11 +95,7 @@ function language_menu() {
    -    'access callback' => 'language_access_language_edit_or_delete',
    @@ -115,7 +115,7 @@ function testLanguageList() {
    -    $this->assertResponse(404, 'Language no longer found.');
    +    $this->assertResponse(403, 'Language no longer found.');
    @@ -132,7 +132,7 @@ function testLanguageList() {
    -    $this->assertResponse(404, 'Language no longer found.');
    +    $this->assertResponse(403, 'Language no longer found.');
    @@ -180,7 +180,7 @@ function testStringTranslation() {
    -    $this->assertResponse(404, 'Language no longer found.');
    +    $this->assertResponse(403, 'Language no longer found.');

    Should be 404!
    Probably special access check should be added to services.yml

  2. @@ -132,10 +128,7 @@ function language_menu() {
       $items['admin/config/regional/language/detection/browser/delete/%'] = array(
    ...
    -    'type' => MENU_CALLBACK,
    ...
    +    'route_name' => 'language_negotiation_browser_delete',

    Callback - so this item should be removed from hook_menu

  3. @@ -0,0 +1,127 @@
    +class AdminDeleteForm extends ConfirmFormBase implements ControllerInterface {

    Please LanguageDeleteForm

  4. @@ -0,0 +1,127 @@
    +  public function __construct(PathBasedGeneratorInterface $url_generator = NULL) {
    +    $this->urlGenerator = $url_generator;

    translation should be injected too

  5. @@ -0,0 +1,127 @@
    +  public function submitForm(array &$form, array &$form_state) {
    +    $success = language_delete($this->language->id);

    suppose $this->entity->delete()

language_delete() still has its own logic. :(

We can also do so much at once. I am a big fan of iterative steps and moving forward with changes faster vs. doing everything at once :)

I meant, we can't fix that here, so don't try to do what #66.5 suggests :)

Yes, #66.5 should be ignored

Related - access controlled needs #2031277-11: Implement checkCreateAccess on LanguageAccessController but this change is not needed for delete form so #2003592-16: Convert language_admin_add_form and language_admin_edit_form to a Controllers

Assigned:webflo» pfrenssen

Going to work on this for a bit.

Status:Needs work» Needs review
StatusFileSize
new11.04 KB
new11.16 KB
PASSED: [[SimpleTest]]: [MySQL] 58,051 pass(es).
[ View ]

Addressed remarks from #66. I also changed the word "posts" to "content" in the following sentence:

Deleting a language will remove all interface translations associated with it, and posts in this language will be set to be language neutral.

Interdiff is quite large since AdminDeleteForm was renamed to LanguageDeleteForm.

Assigned:pfrenssen» Unassigned

Status:Needs review» Needs work
  1. +++ b/core/modules/language/language.module
    @@ -95,11 +95,7 @@ function language_menu() {
         'title' => 'Confirm delete',

    We can move this title to route.yml file now and remvoe this menu item. See drupal_set_title() replaced by returning the title in the render array.

  2. +++ b/core/modules/language/language.routing.yml
    @@ -25,3 +25,17 @@ language_admin_overview:
    +    _form: '\Drupal\language\Form\LanguageDeleteForm'

    This should be _entity_form: 'language_entity.delete'.

  3. +++ b/core/modules/language/lib/Drupal/language/Form/LanguageDeleteForm.php
    @@ -0,0 +1,128 @@
    +use Drupal\Core\Form\ConfirmFormBase;
    +use Drupal\Core\Controller\ControllerInterface;
    +use Drupal\language\Plugin\Core\Entity\Language;
    +use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
    +use Symfony\Component\HttpFoundation\Request;
    +use Drupal\Core\Routing\PathBasedGeneratorInterface;
    +use Symfony\Component\HttpFoundation\RedirectResponse;
    +use Symfony\Component\DependencyInjection\ContainerInterface;

    Sorting order is all wrong.

  4. +++ b/core/modules/language/lib/Drupal/language/Form/LanguageDeleteForm.php
    @@ -0,0 +1,128 @@
    +class LanguageDeleteForm extends ConfirmFormBase implements ControllerInterface {

    I think we can use EntityConfirmFormBase instead of ConfirmFormBase

  5. +++ b/core/modules/language/lib/Drupal/language/Form/LanguageDeleteForm.php
    @@ -0,0 +1,128 @@
    +   * Constructs a new LanguageDeleteForm.

    Doc block should be according to function definition.

  6. +++ b/core/modules/language/lib/Drupal/language/Form/LanguageDeleteForm.php
    @@ -0,0 +1,128 @@
    +  public function __construct(PathBasedGeneratorInterface $url_generator = NULL, Language $language_entity = NULL) {

    Why are we initializing here with NULL?

  7. +++ b/core/modules/language/lib/Drupal/language/Form/LanguageDeleteForm.php
    @@ -0,0 +1,128 @@
    +      $container->get('request')->attributes->get('language_entity')

    We don't have to set it here. We can set it in buildForm.

Assigned:Unassigned» pfrenssen

Thanks for the review! Will address the remarks.

Assigned:pfrenssen» Unassigned
Status:Needs work» Needs review
StatusFileSize
new6.16 KB
new11.83 KB
PASSED: [[SimpleTest]]: [MySQL] 58,264 pass(es).
[ View ]

+++ b/core/modules/language/language.module
@@ -95,11 +95,7 @@ function language_menu() {
     'title' => 'Confirm delete',

We can move this title to route.yml file now and remvoe this menu item. See drupal_set_title() replaced by returning the title in the render array.

If I remove the entry in hook_menu() the help text, local tasks and local actions from the parent menu item appear on the page. This is being discussed in #2049759: Routing within subpath of another route may inherit content from parent route. and @dawehner proposes to leave the router path in hook_menu() for the moment.

But this title was never actually used, instead the title is "Are you sure you want to delete language X?". I will just remove the title from hook_menu().

+++ b/core/modules/language/lib/Drupal/language/Form/LanguageDeleteForm.php
@@ -0,0 +1,128 @@
+   * Constructs a new LanguageDeleteForm.

Doc block should be according to function definition.

I improved the definition a bit, but it's not entirely clear to me what you want changed.

Addressed the rest of the remarks.

StatusFileSize
new1.24 KB
new12.12 KB
PASSED: [[SimpleTest]]: [MySQL] 57,840 pass(es).
[ View ]

Added missing parameter declarations for __construct().

Related issue #2003592: Convert language_admin_add_form and language_admin_edit_form to a Controllers

  1. @@ -94,12 +94,7 @@ function language_menu() {
    +    'route_name' => 'language_admin_delete',
    @@ -25,3 +25,17 @@ language_admin_overview:
    +language_admin_delete:

    language_delete is enough

  2. @@ -880,3 +868,10 @@ function language_system_regional_settings_form_submit($form, &$form_state) {
    + * Implements hook_entity_info().
    ...
    +function language_entity_info(&$entity_info) {
    +  $entity_info['language_entity']['controllers']['form']['delete'] = 'Drupal\language\Form\LanguageDeleteForm';

    You need to update lib/Drupal/language/Entity/Language entity annotation for that

  3. @@ -0,0 +1,138 @@
    +   * @param \Drupal\Core\Entity\EntityManager $entity_manager
    +   *   The entity manager.
    ...
    +    $this->entityManager = $entity_manager;
    ...
    +      $container->get('plugin.manager.entity'),

    there's no usage of entity manager, please remove

  4. @@ -0,0 +1,138 @@
    +    return t('Are you sure you want to delete the language %language?', array('%language' => $this->entity->get('label')));
    ...
    +    return t('Deleting a language will remove all interface translations associated with it, and content in this language will be set to be language neutral. This action cannot be undone.');
    ...
    +    return t('Delete');
    ...
    +      drupal_set_message(t('The default language cannot be deleted.'));
    ...
    +      drupal_set_message(t('The %language (%langcode) language has been removed.', $t_args));
    @@ -0,0 +1,71 @@
    +    return t('Are you sure you want to delete %browser_langcode?', array(

    could be ignored, but probably will need a string translation service somehow #2059245: Add a FormBase class containing useful methods

  5. @@ -0,0 +1,138 @@
    +    else {
    +      return parent::buildForm($form, $form_state, $request);
    +    }

    else is not needed

Assigned:Unassigned» pfrenssen
Status:Needs review» Needs work

Thanks a lot for the review, will address the remarks during my flight home this afternoon.

Status:Needs work» Needs review
StatusFileSize
new4.78 KB
new12.1 KB
PASSED: [[SimpleTest]]: [MySQL] 58,150 pass(es).
[ View ]

I've addressed all comments except the t() conversions, I worked on this on during a flight and had no internet connection. I could not find any examples of injecting a translation service in the recent commits list. I will do these conversions later today.

+++ b/core/modules/language/lib/Drupal/language/Form/LanguageDeleteForm.php
@@ -0,0 +1,124 @@
+    return t('Are you sure you want to delete the language %language?', array('%language' => $this->entity->get('label')));
...
+      $t_args = array('%language' => $this->entity->get('label'), '%langcode' => $this->entity->id());

This can be $this->entity->label();

Assigned:pfrenssen» Unassigned
StatusFileSize
new4.77 KB
new12.81 KB
PASSED: [[SimpleTest]]: [MySQL] 58,018 pass(es).
[ View ]

Injected the translation service, and used label() as suggested by @jibran.

I am still using the "old" translation method as #2059245: Add a FormBase class containing useful methods is not in yet. I think it is rather odd that it is an instance of TranslatorInterface, but is calling the method translate() which is not defined in the interface, but in TranslationManager. This means that we can't swap out this translator with any other class implementing TranslatorInterface, as they might not have the translate() method if I'm not mistaken. That is not for this issue to solve though :)

This is RTBC for me. Let's wait for @andypost.

I think translation better to revert to t() and use it after #2059245: Add a FormBase class containing useful methods because both confirm forms affected please unify them to use

t()

according to #2003592-24: Convert language_admin_add_form and language_admin_edit_form to a Controllers #3

  1. +++ b/core/modules/language/lib/Drupal/language/Form/LanguageDeleteForm.php
    @@ -0,0 +1,139 @@
    +   * @var \Drupal\Core\StringTranslation\Translator\TranslatorInterface
    ...
    +   * @param \Drupal\Core\StringTranslation\Translator\TranslatorInterface $translator

    Drupal/Core/StringTranslation/TranslationInterface is in #2049709: TranslationManager::translate() should be on an interface please remove it

  2. +++ b/core/modules/language/lib/Drupal/language/Form/NegotiationBrowserDeleteForm.php
    @@ -0,0 +1,71 @@
    +  public function buildForm(array $form, array &$form_state, $browser_langcode = NULL, Request $request = NULL) {

    maybe add 'string' type-hint?

you can't typehint strings.

Assigned:Unassigned» pfrenssen
Status:Needs review» Needs work

Thanks for linking the issue about translate() not being on an interface. Nice to see that it got addressed. Will update patch.

Assigned:pfrenssen» Unassigned
Status:Needs work» Needs review
StatusFileSize
new4.6 KB
new11.92 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Addressed remarks, and made necessary changes to accommodate for #2059245: Add a FormBase class containing useful methods.

Issue tags:+FormBase
StatusFileSize
new5.2 KB
new11.72 KB
PASSED: [[SimpleTest]]: [MySQL] 58,069 pass(es).
[ View ]

A couple missing tweaks.

Status:Needs review» Reviewed & tested by the community

Awesome #84 is addressed. Thanks @pfrenssen for finishing this up.

Status:Reviewed & tested by the community» Fixed
Issue tags:-API change

Committed eddf21e and pushed to 8.x. Thanks!

This change is approved as part of the meta so removing "api change" tag

Title:Convert all of confirm_form() in language.admin.inc to the new form interfaceHEAD BROKEN: Convert all of confirm_form() in language.admin.inc to the new form interface
Category:task» bug
Priority:Normal» Critical

Working on a fix.

Status:Fixed» Reviewed & tested by the community
StatusFileSize
new1.39 KB
PASSED: [[SimpleTest]]: [MySQL] 58,435 pass(es).
[ View ]

Title:HEAD BROKEN: Convert all of confirm_form() in language.admin.inc to the new form interfaceConvert all of confirm_form() in language.admin.inc to the new form interface
Category:bug» task
Priority:Critical» Normal
Status:Reviewed & tested by the community» Fixed

Rock, thanks!

Committed and pushed to 8.x.

Issue tags:-sprint

This was a long time coming. Thanks!

Sprint tag just does not want to go away :D

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