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

Comments

Bès created an issue. See original summary.

bès’s picture

bès commented
StatusFileSize
new 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 commented
Status: Active » Needs review

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

bès’s picture

bès commented
StatusFileSize
new 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 commented
StatusFileSize
new 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
commented
StatusFileSize
new 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 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 commented

RTBC - except for stylistic fixes needed.

bès’s picture

bès commented
StatusFileSize
new 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 commented
StatusFileSize
new 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 commented
Status: Needs review » Reviewed & tested by the community

RTBC

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Needs a re-roll for 8.2.x

haza’s picture

haza
Primary language French
commented
Status: Needs work » Needs review
StatusFileSize
new code_after-2675648-16.patch2.82 KB

Just re-rolling against 8.2.x

fabianx’s picture

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