See hook_link_alter() documentation

If you alter the Url object inside $variable['url'] nothing change because it's not used in the code after the alteration.

CommentFileSizeAuthor
#16 code_after-2675648-16.patch2.82 KBHaza
#12 2675648-12.patch2.8 KBBès
#11 make-test-fail-2675648-11.patch2.23 KBBès
#7 2675648-7.patch2.71 KBdawehner
#5 test-hook_link_alter-url-fix.2675648-5.patch2.8 KBBès
#4 test-hook_link_alter-url-fix.2675648-4.patch1.21 KBBès
#2 test-hook_link_alter-url.2675648.patch1.59 KBBès
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Bès created an issue. See original summary.

Bès’s picture

Bès CreditAttribution: Bès at Happyculture commented
FileSize
test-hook_link_alter-url.2675648.patch1.59 KB

Here is a first patch that add a failing test demonstrating that hook_link_alter is not working.

Bès’s picture

Bès CreditAttribution: Bès at Happyculture commented
Status: Active » Needs review

Set to Needs review to have the test bot show the failing test.

Bès’s picture

Bès CreditAttribution: Bès at Happyculture commented
FileSize
test-hook_link_alter-url-fix.2675648-4.patch1.21 KB

Here is a patch that fix the test and make the altered data used by the code.

Bès’s picture

Bès CreditAttribution: Bès at Happyculture commented
FileSize
test-hook_link_alter-url-fix.2675648-5.patch2.8 KB

Good version of the patch with the fix and the test in the same file.

The last submitted patch, 2: test-hook_link_alter-url.2675648.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
FileSize
2675648-7.patch2.71 KB

This is indeed a problem. What about fixing it like this, which would be IMHO a bit easier to read at the end of the day?

chx’s picture

chx CreditAttribution: chx at Smartsheet commented

Two suggestions

$this->getMock('\Drupal\Core\Routing\UrlGenerator', array(), array(), '', FALSE, TRUE, TRUE); use the mock builder please

$options['url'] = (new Url('test_route_1'))->setUrlGenerator($this->urlGenerator);

the fewer references we use the better ; although of course the closure needs &$options but further referencing is unnecessary.

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.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx at Tag1 Consulting commented

RTBC - except for stylistic fixes needed.

Bès’s picture

Bès CreditAttribution: Bès at Happyculture commented
FileSize
make-test-fail-2675648-11.patch2.23 KB

Here is an update of the tests with the fixes suggested by chx that shows the failing tests when the code is not fixed

Bès’s picture

Bès CreditAttribution: Bès at Happyculture commented
FileSize
2675648-12.patch2.8 KB

and the patch with the same test plus the fix.

The last submitted patch, 11: make-test-fail-2675648-11.patch, failed testing.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx at Tag1 Consulting commented
Status: Needs review » Reviewed & tested by the community

RTBC

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work

Needs a re-roll for 8.2.x

Haza’s picture

Haza
Primary language French
CreditAttribution: Haza at Happyculture commented
Status: Needs work » Needs review
FileSize
code_after-2675648-16.patch2.82 KB

Just re-rolling against 8.2.x

Fabianx’s picture

Fabianx CreditAttribution: Fabianx at Tag1 Consulting commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC

  • catch committed 260c6ee on 8.2.x 
    Issue #2675648 by Bès, dawehner, Haza, chx: Code after hook_link_alter...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work

Committed/pushed to 8.2.x, thanks!

The 8.1.x patch no longer applies either, so this will need a re-roll for that too.

Bès’s picture

Bès CreditAttribution: Bès at Happyculture commented
Status: Needs work » Reviewed & tested by the community

Patch on #12 apply without warning on the 8.1.x branch for me. I don't know what to do.

  • catch committed d8688fa on 8.1.x 
    Issue #2675648 by Bès, Haza, dawehner, chx: Code after hook_link_alter...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

No you're right, not sure what went wrong my end.

Committed/pushed to 8.1.x, thanks!

Status: Fixed » Closed (fixed)

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