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.
Problem/Motivation
'path' variable in $this->moduleHandler->alter('language_switch_links', $result, $type, $path);
line 427 is undefined.
Line 412
/**
* {@inheritdoc}
*/
public function getLanguageSwitchLinks($type, Url $url) {
$links = FALSE;
if ($this->negotiator) {
foreach ($this->negotiator->getNegotiationMethods($type) as $method_id => $method) {
$reflector = new \ReflectionClass($method['class']);
if ($reflector->implementsInterface('\Drupal\language\LanguageSwitcherInterface')) {
$result = $this->negotiator->getNegotiationMethodInstance($method_id)->getLanguageSwitchLinks($this->requestStack->getCurrentRequest(), $type, $url);
if (!empty($result)) {
// Allow modules to provide translations for specific links.
$this->moduleHandler->alter('language_switch_links', $result, $type, $path);
$links = (object) array('links' => $result, 'method_id' => $method_id);
break;
}
}
}
}
return $links;
}
Proposed resolution
Pass $url
instead of $path
to hook_language_switch_links_alter
.
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Create a patch | Instructions | Done | |
Update the issue summary | Instructions | Done | |
Draft a change record for the API changes | Instructions | Done | |
Manually test the patch | Novice | Instructions | Done |
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Done | Instructions |
User interface changes
None.
API changes
hook_language_switch_links_alter
third param is now a \Drupal\Core\Url
object.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#28 | 2585821-27.patch | 4.38 KB | blazey |
#22 | 2585821-22-test-only.patch | 2.61 KB | blazey |
Comments
Comment #2
benelori CreditAttribution: benelori commentedThe $path is also mentioned in language.api.php and represents the current path. In this getLanguageSwitchLinks method, this is actually the $url parameter.
I changed the documentation as well, to match the one in LanguageManagerInterface.
Comment #3
GoZ CreditAttribution: GoZ at Centarro commentedComment #4
GoZ CreditAttribution: GoZ at Centarro commentedThanks @benelori.
Miss @param types like
@param \Drupal\Core\Url $url
, especially for Url, but also for each params.Comment #5
marvin_B8 CreditAttribution: marvin_B8 as a volunteer and at comm-press commentedComment #6
marvin_B8 CreditAttribution: marvin_B8 as a volunteer and at comm-press commentedComment #7
GoZ CreditAttribution: GoZ at Centarro commentedMiss
Url
cast before $url variable like http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal.php#n553Comment #8
GoZ CreditAttribution: GoZ at Centarro commentedComment #9
GoZ CreditAttribution: GoZ at Centarro commentedComment #10
DuaelFrWe should use typehints to ensure that the $url param is an Url object.
---
Aside, the testbot being happy with that patch shows us that our test coverage is quite bad. We might extend tests to cover that function.
Comment #11
DuaelFrI'm too slow ^^
Sorry for the duplicate demand ;)
Comment #12
sdstyles CreditAttribution: sdstyles at FFW commentedComment #13
GoZ CreditAttribution: GoZ at Centarro commented@DuaelFr said :
But i don't find examples of this kind of tests, so i let you this part.
Comment #17
blazey CreditAttribution: blazey at Amazee Labs commentedAttaching a unit test that checks the arguments passed to hook_language_switch_links_alter.
Comment #19
GoZ CreditAttribution: GoZ at Centarro commentedThanks blazey for tests.
I create a draft for change record https://www.drupal.org/node/2854519.
I move issue to 8.4.x because it needs an API change record.
Comment #20
GoZ CreditAttribution: GoZ at Centarro commentedPatch apply and do what is expected
Comment #21
alexpottI've spent some time looking at the test coverage. Nice work on mocking out what needed to be mocked to test this. That said, I'm uneasy about introducing such a test for hook. Hooks are integration points and therefore not simple targets for unit tests. I think a kernel test using a test module would be far appropriate and less fragile to unrelated changes to language negotiation.
Also I'm not sure that this change requires a CR and can't be backported to 8.3.x. Since $path was originally untyped and always NULL. If current implementations have a $path now it'll have a Url object in it but nothing with break and they can't be relying on it because it wasn't working.
Comment #22
blazey CreditAttribution: blazey at Amazee Labs commentedThanks for feedback @GoZ @alexpott. Attaching a new kernel test for the hook.
Comment #23
blazey CreditAttribution: blazey at Amazee Labs commentedComment #24
blazey CreditAttribution: blazey at Amazee Labs commentedComment #26
Leksat CreditAttribution: Leksat at Amazee Labs commented@blazey I guess you should use
\Drupal\Core\Url $url
in the hook signature. At least other *.api.php files use fully qualified class names (example).From my point of view, this is the last thing before RTBC :)
Comment #27
GoZ CreditAttribution: GoZ at Centarro commentedNeeds works see #26
Comment #28
blazey CreditAttribution: blazey at Amazee Labs commentedThanks for review @leksat. Attaching updated patch.
Comment #29
Leksat CreditAttribution: Leksat at Amazee Labs commentedLooks good to me now. Thanks for the patch!
Comment #32
catchTest coverage looks much better, like the comment :)
Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!
Comment #34
quietone CreditAttribution: quietone at PreviousNext commentedpublished the change record