Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
List of improvements (patch can be applied ONLY after patches from related issues):
- General style for module hooks
- Constants for maximum re-usage
- Configuration form improvements
- SimpleTest improvements
Comment | File | Size | Author |
---|---|---|---|
#14 | interdiff-2735897-10-14.txt | 2.6 KB | BR0kEN |
#14 | language_header-architecture-improvements-2735897-14.patch | 6.96 KB | BR0kEN |
Comments
Comment #2
BR0kENComment #3
valthebald1. 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?
2. What improvement is this change?
3. Can you please adjust the patch to test placed in tests folder?
Comment #4
BR0kEN@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 (initiallyhook_menu
return a variable andhook_language_negotiation_info
- an array directly).Comment #5
valthebaldHere'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().
Comment #6
BR0kENComment #7
BR0kENStrange 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 froml()
function which is safe.Comment #8
valthebald1. 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:
Comment #9
BR0kENI 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 tol()
(because we don't need to translateISO-639-1
).Comment #10
BR0kENComment #11
valthebaldStill incorrect link wrapping
1. Missing 'types' declaration
2. Don't see a reason to declare a variable
Comment #12
BR0kENIf
types
are not defined, thendrupal_language_types()
will be used. Or do you think it will be obvious to add usage ofdrupal_language_types()
into type definition?Comment #13
valthebald@BR0kEN, I was just reviewing the patch, and noticed
types
removal. But after consulting with the hook documentation, this is fine, andtypes
is not needed.Comment #14
BR0kENComment #15
valthebaldWhen trying to apply the latest patch to 7.x-1.x:
Can you check please?
Comment #16
BR0kENI guess you are having modified codebase by earlier patches.
Result:
Comment #18
valthebaldCommitted and pushed to 7.x-1.x. Thank you!