Files: 
CommentFileSizeAuthor
#5 language_browser_form-2082071-5.patch13.53 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 58,987 pass(es).
[ View ]
#5 interdiff.txt2.65 KBplopesc
#1 language_browser_form-2082071-1.patch13.31 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 58,335 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new13.31 KB
PASSED: [[SimpleTest]]: [MySQL] 58,335 pass(es).
[ View ]

Hello, attaching patch for this issue.

There are a couple of issues related to language_get_browser_drupal_langcode_mappings() and language_set_browser_drupal_langcode_mappings() functions, used in the current form implementation.

Given that these functions assume that we are calling it from procedural code, they call to Drupal::config(). I replaced it in the new form class by calls to $this->configFactory() using dependency injection. Then I found that NegotiationBrowserDeleteForm class uses both functions, I don't know if you preferred this approach in these cases.

IMHO, we should use the dependency injection in form classes. What do you think?

Regards

+++ b/core/modules/language/lib/Drupal/language/Form/NegotiationBrowserForm.php
@@ -0,0 +1,195 @@
+    $this->configFactory = $config_factory;
+    $this->configFactory->enterContext($context);

replace with parent::__construct($config_factory, $context);

As for dependency injection, your approach is better than the way it was done in NegotiationBrowserDeleteForm. Don't fix that here though, that can be done in a separate issue.

Status:Needs review» Needs work

  1. +++ b/core/modules/language/language.module
    @@ -119,10 +119,7 @@ function language_menu() {
       $items['admin/config/regional/language/detection/browser'] = array(
         'title' => 'Browser language detection configuration',
    +++ b/core/modules/language/language.routing.yml
    @@ -33,6 +33,13 @@ language_delete:
    +language_negotiation_browser:
    +  pattern: 'admin/config/regional/language/detection/browser'
    +  defaults:
    +    _form: '\Drupal\language\Form\NegotiationBrowserForm'
    +  requirements:
    +    _permission: 'administer languages'
    +

    Let's add title to the defaults->_title route definition.

  2. +++ b/core/modules/language/lib/Drupal/language/Form/NegotiationBrowserForm.php
    @@ -0,0 +1,195 @@
    +   * @var \Drupal\Core\Extension\ModuleHandler
    ...
    +   * @param \Drupal\Core\Extension\ModuleHandler $module_handler
    ...
    +  public function __construct(ConfigFactory $config_factory, ContextInterface $context, ModuleHandler $module_handler) {

    We should use the ModuleHandlerInterface instead of just the ModuleHandler.

  3. +++ b/core/modules/language/lib/Drupal/language/Form/NegotiationBrowserForm.php
    @@ -0,0 +1,195 @@
    +
    +  protected function language_get_browser_drupal_langcode_mappings() {
    +    $config = $this->configFactory->get('language.mappings');

    Missing documentation.

Status:Needs work» Needs review
StatusFileSize
new2.65 KB
new13.53 KB
PASSED: [[SimpleTest]]: [MySQL] 58,987 pass(es).
[ View ]

Hello

Attaching patch including suggestions in #2 and #4.

@dawehner: I added _title in routing file. However, I didn't remove it in hook_menu, because in that case, this page doesn't show the title. Am I missing something?

Regards.

@dawehner: Maybe HtmlFormController does not take into account _title property?

HtmlPageController code tries to find it, but HtmlFormController just return drupal_build_form. I was not able to find where _title is processed in this case.

Regards.

Status:Needs review» Reviewed & tested by the community

Mh right, I know that aspilicious was working on a fix, so let's keep it like that.

Status:Reviewed & tested by the community» Needs review

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.

Status:Needs review» Reviewed & tested by the community

moving back to rtbc, xjm cross posted.

Issue tags:+D8MI, +language-base

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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