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

Contributor tasks needed
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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

GoZ created an issue. See original summary.

benelori’s picture

Status: Active » Needs review
FileSize
1.56 KB

The $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.

GoZ’s picture

Issue summary: View changes
GoZ’s picture

Status: Needs review » Needs work

Thanks @benelori.

+++ b/core/lib/Drupal/Core/Language/language.api.php
@@ -166,10 +166,10 @@
+ * @param $url

Miss @param types like @param \Drupal\Core\Url $url, especially for Url, but also for each params.

marvin_B8’s picture

marvin_B8’s picture

Status: Needs work » Needs review
GoZ’s picture

+++ b/core/lib/Drupal/Core/Language/language.api.php
@@ -162,14 +162,14 @@
+function hook_language_switch_links_alter(array &$links, $type, $url) {

Miss Url cast before $url variable like http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal.php#n553

GoZ’s picture

Issue summary: View changes
GoZ’s picture

Status: Needs review » Needs work
DuaelFr’s picture

Issue summary: View changes
Issue tags: -Novice +Needs tests
+++ b/core/lib/Drupal/Core/Language/language.api.php
@@ -162,14 +162,14 @@
+function hook_language_switch_links_alter(array &$links, $type, $url) {

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

DuaelFr’s picture

I'm too slow ^^
Sorry for the duplicate demand ;)

sdstyles’s picture

Status: Needs work » Needs review
FileSize
1.89 KB
784 bytes
GoZ’s picture

Status: Needs review » Needs work

@DuaelFr said :

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.

But i don't find examples of this kind of tests, so i let you this part.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

blazey’s picture

Attaching a unit test that checks the arguments passed to hook_language_switch_links_alter.

The last submitted patch, 17: 2585821-17-test-only.patch, failed testing.

GoZ’s picture

Version: 8.3.x-dev » 8.4.x-dev
Issue summary: View changes
Issue tags: +API change, +Needs change record

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

GoZ’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Patch apply and do what is expected

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I'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.

blazey’s picture

Thanks for feedback @GoZ @alexpott. Attaching a new kernel test for the hook.

blazey’s picture

Status: Needs work » Needs review
blazey’s picture

Version: 8.4.x-dev » 8.3.x-dev

The last submitted patch, 22: 2585821-22-test-only.patch, failed testing.

Leksat’s picture

@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 :)

GoZ’s picture

Status: Needs review » Needs work

Needs works see #26

blazey’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs change record
FileSize
4.38 KB
834 bytes

Thanks for review @leksat. Attaching updated patch.

Leksat’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me now. Thanks for the patch!

  • catch committed 801f6ca on 8.4.x
    Issue #2585821 by blazey, marvin_B8, sdstyles, benelori, GoZ, DuaelFr:...

  • catch committed 9e1d3b3 on 8.3.x
    Issue #2585821 by blazey, marvin_B8, sdstyles, benelori, GoZ, DuaelFr:...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Test coverage looks much better, like the comment :)

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

Status: Fixed » Closed (fixed)

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

quietone’s picture

published the change record