Support from Acquia helps fund testing for Drupal Acquia logo

Comments

BR0kEN created an issue. See original summary.

BR0kEN’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
5.37 KB
valthebald’s picture

Status: Needs review » Needs work
+++ b/language_header.module
@@ -5,41 +5,44 @@
 -   $items = array();
+  $info = array();

1. It's a common convention to call hook_menu items $items. Why change it?
Another question to hook_menu implementation - why change order of array members?

+++ b/language_header.module
@@ -5,41 +5,44 @@
+  $info = array();

2. What improvement is this change?

3. Can you please adjust the patch to test placed in tests folder?

BR0kEN’s picture

@valthebald, $info name of variable has been used because all hooks provide an information about something: menu routes, language providers. Also, a good practice in any programming language, define variables before using them. In addition to this, two hooks have an overall view now (initially hook_menu return a variable and hook_language_negotiation_info - an array directly).

valthebald’s picture

+++ b/language_header.admin.inc
@@ -9,12 +9,22 @@
+    '#description' => t('The name of request HTTP header which should contain !iso language code.', array(

Here's the right way to place links in translatable strings - https://www.drupal.org/node/322774

Arrays returned by (especially) hook_menu() and (almost, because of drupal_language_types()) hook_language_negotiation_info() are string constants, never modified after they are created. I'm not aware of any good practice of naming anonymous variables. If module declares several menu items, then having $items in hook_menu() helps with code readability. But in our case, if perform any change at all, I would remove declaration of $items in language_header_menu().

BR0kEN’s picture

BR0kEN’s picture

Strange interdiff. Please, have a look at the patch - it's not quite big.

By my opinion, better to use !placeholder and replace it by value from l() function which is safe.

valthebald’s picture

Version: 7.x-1.0 » 7.x-1.x-dev
Category: Feature request » Task
Priority: Normal » Minor
Status: Needs review » Needs work

1. This is not a new feature - changed category to 'task'.
2. Changing priority to 'Minor'
3. If you aim to improve code style, why wrap the code in IgnoreCodingStandards tags? Have a look at example from the link at #5:

$BAD_INTERNAL_LINK = t('To get an overview of your administration options, go to !administer in the main menu.', array(
  '!administer' => l(t('the Administer screen'), 'admin'),
);
BR0kEN’s picture

Status: Needs work » Needs review

I saw those examples and not sure that this is our case from "bad links". CS tags has been added because I'm not passing the text, processed by t() function, as a first argument to l() (because we don't need to translate ISO-639-1).

BR0kEN’s picture

valthebald’s picture

Status: Needs review » Needs work
+++ b/language_header.admin.inc
@@ -9,12 +9,20 @@
+    '#description' => t('The name of request HTTP header which should contain !iso language code.', array(
+      // @codingStandardsIgnoreStart
+      '!iso' => l('ISO 639-1', 'https://en.wikipedia.org/wiki/ISO_639-1', array(
+        // @codingStandardsIgnoreEnd
+        'attributes' => array('target' => '_blank'),
+      )),
+    )),
   );

Still incorrect link wrapping


+++ b/language_header.module
@@ -28,18 +30,19 @@ function language_header_menu() {
+  $info = array();
+
+  $info[LANGUAGE_HEADER_PROVIDER] = array(
+    'name' => t('HTTP header'),
+    'weight' => -5,
+    'config' => LANGUAGE_HEADER_CONFIG_PATH,
+    'description' => t('Determine the language from HTTP header'),
+    'callbacks' => array(
+      'language' => 'language_header_language',
     ),
   );
+
+  return $info;
 }

1. Missing 'types' declaration
2. Don't see a reason to declare a variable

BR0kEN’s picture

If types are not defined, then drupal_language_types() will be used. Or do you think it will be obvious to add usage of drupal_language_types() into type definition?

valthebald’s picture

@BR0kEN, I was just reviewing the patch, and noticed types removal. But after consulting with the hook documentation, this is fine, and types is not needed.

BR0kEN’s picture

valthebald’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

When trying to apply the latest patch to 7.x-1.x:

patch -p1 < ~/tmp/language_header-architecture-improvements-2735897-14.patch 
patching file language_header.admin.inc
Hunk #1 FAILED at 9.
1 out of 1 hunk FAILED -- saving rejects to file language_header.admin.inc.rej
patching file language_header.module
Hunk #2 FAILED at 29.
Hunk #3 FAILED at 57.
2 out of 3 hunks FAILED -- saving rejects to file language_header.module.rej
patching file tests/language_header.test
Hunk #2 FAILED at 24.
1 out of 2 hunks FAILED -- saving rejects to file tests/language_header.test.rej

Can you check please?

BR0kEN’s picture

Status: Needs work » Needs review

I guess you are having modified codebase by earlier patches.

git clone git://git.drupal.org/project/language_header.git
cd language_header/
wget https://www.drupal.org/files/issues/language_header-architecture-improvements-2735897-14.patch
patch -p1 < language_header-architecture-improvements-2735897-14.patch

Result:

patching file language_header.admin.inc
patching file language_header.module
patching file tests/language_header.test

  • valthebald committed ce974b5 on 7.x-1.x authored by BR0kEN
    Issue #2735897 by BR0kEN: Improvements for tests and architecture
    
valthebald’s picture

Status: Needs review » Fixed
Issue tags: -Needs reroll

Committed and pushed to 7.x-1.x. Thank you!

Status: Fixed » Closed (fixed)

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