CommentFileSizeAuthor
#92 urlgenerator-1946426-92.patch1.39 KBtim.plunkett
#88 language-1946426-88.patch11.72 KBtim.plunkett
#88 interdiff.txt5.2 KBtim.plunkett
#87 1946426-language-config-form-87.patch11.92 KBpfrenssen
#87 interdiff.txt4.6 KBpfrenssen
#82 1946426-language-config-form-82.patch12.81 KBpfrenssen
#82 interdiff.txt4.77 KBpfrenssen
#80 1946426-language-config-form-80.patch12.1 KBpfrenssen
#80 interdiff.txt4.78 KBpfrenssen
#77 1946426-language-config-form-77.patch12.12 KBpfrenssen
#77 interdiff.txt1.24 KBpfrenssen
#76 1946426-language-config-form-76.patch11.83 KBpfrenssen
#76 interdiff.txt6.16 KBpfrenssen
#72 1946426-language-config-form-72.patch11.16 KBpfrenssen
#72 interdiff.txt11.04 KBpfrenssen
#64 1946426-language-config-form-64.patch13.13 KBvijaycs85
#64 1946426-diff-62-64.txt1.55 KBvijaycs85
#62 1946426-language-config-form-62.patch13.22 KBvijaycs85
#57 drupal-convert_language_confirm_form-1946426-57.patch14.91 KBdisasm
#55 convert_language_confirm_form-1946426-55.patch14.93 KBwebflo
#55 interdiff.patch1.01 KBwebflo
#47 convert_language_confirm_form-1946426-47.patch14.97 KBwebflo
#43 convert_language_confirm_form-1946426-43.patch14.29 KBwebflo
#43 interdiff-1946426-34-43.txt5.37 KBwebflo
#42 convert_language_confirm_form-1946426-41.patch14.87 KBramlev
#42 interdiff.txt4.64 KBramlev
#41 convert_language_confirm_form-1946426-41.patch14.87 KBramlev
#41 interdiff.txt4.64 KBramlev
#34 convert_language_confirm_form-1946426-34.patch15.63 KBramlev
#34 interdiff.txt2.71 KBramlev
#30 convert_language_confirm_form-1946426-30.patch12.77 KBDavid Hernández
#30 interdiff.txt1.75 KBDavid Hernández
#28 convert_language_confirm_form-1946426-28.patch12.88 KBDavid Hernández
#28 interdiff.txt1.59 KBDavid Hernández
#26 convert_language_confirm_form-1946426-25.patch12.6 KBramlev
#26 interdiff.txt1.93 KBramlev
#23 convert_language_confirm_form-1946426-22.patch12.05 KBramlev
#18 convert_language_confirm_form-1946426-18.patch11.94 KBramlev
#14 convert_language_confirm_form-1946426-14.patch11.73 KBfoopang
#11 convert_language_confirm_form-1946426-11.patch12.17 KBfoopang
#8 convert_language_confirm_form-1946426-8.patch11.97 KBfoopang
#4 convert_language_confirm_form-1946426-4.patch11.92 KBfoopang
#2 convert_language_confirm_form-1946426-2.patch11.93 KBfoopang
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

foopang’s picture

Assigned: Unassigned » foopang

I'll take this task :D

foopang’s picture

Status: Active » Needs review
FileSize
11.93 KB

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.

foopang’s picture

Status: Needs work » Needs review
FileSize
11.92 KB

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.

foopang’s picture

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.

foopang’s picture

Removed trailing whitespace, rerolled the patch.

foopang’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

foopang’s picture

Status: Needs work » Needs review
FileSize
12.17 KB

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.

tim.plunkett’s picture

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

foopang’s picture

Status: Needs work » Needs review
FileSize
11.73 KB

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.

jibran’s picture

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.

ramlev’s picture

Status: Needs work » Needs review
FileSize
11.94 KB

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

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-base

Tagging with some D8MI tags.

Gábor Hojtsy’s picture

Issue tags: +sprint

Also put on sprint.

Gábor Hojtsy’s picture

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

ramlev’s picture

Assigned: foopang » ramlev
ramlev’s picture

Status: Needs work » Needs review
FileSize
12.05 KB

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.

Gábor Hojtsy’s picture

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.

ramlev’s picture

Status: Needs work » Needs review
FileSize
1.93 KB
12.6 KB

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

The implementation is "properly injected" now i hope.

Gábor Hojtsy’s picture

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.

David Hernández’s picture

Status: Needs work » Needs review
FileSize
1.59 KB
12.88 KB

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

Gábor Hojtsy’s picture

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?

David Hernández’s picture

I hope that's fixed now.

David Hernández’s picture

Status: Needs work » Needs review

Updated status

Gábor Hojtsy’s picture

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.

ramlev’s picture

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

ramlev’s picture

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

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

ramlev’s picture

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.

ramlev’s picture

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.

webflo’s picture

Assigned: ramlev » webflo

I am working on it.

ramlev’s picture

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

ramlev’s picture

And heres last attempt on the patch.

webflo’s picture

Status: Needs work » Needs review
FileSize
5.37 KB
14.29 KB

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.
ramlev’s picture

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.

ramlev’s picture

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.

webflo’s picture

Status: Needs work » Needs review
FileSize
14.97 KB

Changed the response code assertion in LocaleTranslationUiTest.

Kristen Pol’s picture

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.

Gábor Hojtsy’s picture

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?

ramlev’s picture

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.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Yay, get this in then!

Kristen Pol’s picture

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

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

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

webflo’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
14.93 KB

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

andypost’s picture

Status: Needs review » Needs work
disasm’s picture

Status: Needs work » Needs review
FileSize
14.91 KB

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

andypost’s picture

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

vijaycs85’s picture

Status: Needs work » Postponed
penyaskito’s picture

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.

Gábor Hojtsy’s picture

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

Add API change tags.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
13.22 KB

Re-rolling...

Status: Needs review » Needs work

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
13.13 KB

Line ending fix...

Status: Needs review » Needs work

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

andypost’s picture

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

tim.plunkett’s picture

language_delete() still has its own logic. :(

Gábor Hojtsy’s picture

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

tim.plunkett’s picture

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

andypost’s picture

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

pfrenssen’s picture

Assigned: webflo » pfrenssen

Going to work on this for a bit.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
11.04 KB
11.16 KB

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.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
jibran’s picture

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.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen

Thanks for the review! Will address the remarks.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
6.16 KB
11.83 KB
+++ 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.

pfrenssen’s picture

Added missing parameter declarations for __construct().

andypost’s picture

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

pfrenssen’s picture

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

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

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
4.78 KB
12.1 KB

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.

jibran’s picture

+++ 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();

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
FileSize
4.77 KB
12.81 KB

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

jibran’s picture

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

andypost’s picture

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?

tim.plunkett’s picture

you can't typehint strings.

pfrenssen’s picture

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.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
4.6 KB
11.92 KB

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

tim.plunkett’s picture

Issue tags: +FormBase
FileSize
5.2 KB
11.72 KB

A couple missing tweaks.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

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

tim.plunkett’s picture

Title: Convert all of confirm_form() in language.admin.inc to the new form interface » HEAD 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.

tim.plunkett’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
1.39 KB
webchick’s picture

Title: HEAD BROKEN: Convert all of confirm_form() in language.admin.inc to the new form interface » Convert 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.

Gábor Hojtsy’s picture

Issue tags: -sprint

This was a long time coming. Thanks!

Gábor Hojtsy’s picture

Sprint tag just does not want to go away :D

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