Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plopesc’s picture

Status: Active » Needs review
FileSize
13.31 KB

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

disasm’s picture

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

disasm’s picture

Status: Needs review » Needs work
dawehner’s picture

  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.

plopesc’s picture

Status: Needs work » Needs review
FileSize
2.65 KB
13.53 KB

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.

plopesc’s picture

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

dawehner’s picture

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.

xjm’s picture

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.

disasm’s picture

Status: Needs review » Reviewed & tested by the community

moving back to rtbc, xjm cross posted.

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-base
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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