Issue fork diff-2954352

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

oknate created an issue. See original summary.

oknate’s picture

StatusFileSize
new403 bytes

When you're viewing a diff, the tabs to return to viewing the node or editing the node disappear.

This patch adds tabs on the route diff.revisions_diff.

tabs on diff

oknate’s picture

StatusFileSize
new92.58 KB
akalam’s picture

Status: Active » Needs review

The patch #2 worked for me on Drupal 9.4.5, seems like a simpler solution compared to #3169729: No local tasks on compare revisions page

sokru’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new1.07 KB

Added also tests to make sure the local tabs are visible.

programmerdiego’s picture

Patch #5 worked for my Drupal 9.5.11 site.
I added the following to the composer.json file
"patches": {
"drupal/diff": {
Displays local tasks on revisions sub-tab: "https://www.drupal.org/files/issues/2023-10-05/2954352-diff-show-local-t..."
}
}

tanmayk’s picture

+1 for RTBC

acbramley made their first commit to this issue’s fork.

  • acbramley committed 9ac65140 on 8.x-1.x
    Issue #2954352 by oknate, sokru: Local tasks for node not visible on...
acbramley’s picture

Status: Reviewed & tested by the community » Fixed

Great and simple improvement. Thanks!

acbramley’s picture

Status: Fixed » Needs work

Unfortunately this has caused issues on Node revision pages when there is more than one secondary tab (which is what is required to actually render the link)

Symfony\Component\Routing\Exception\MissingMandatoryParametersException: Some mandatory parameters are missing ("left_revision", "right_revision", "filter") to generate a URL for route "diff.revisions_diff". in Drupal\Core\Routing\UrlGenerator->doGenerate() (line 187 of core/lib/Drupal/Core/Routing/UrlGenerator.php). 

  • acbramley committed 6eef3b1d on 8.x-1.x
    Revert "Issue #2954352 by oknate, sokru: Local tasks for node not...

  • acbramley committed 6eef3b1d on 2.x
    Revert "Issue #2954352 by oknate, sokru: Local tasks for node not...
sokru’s picture

@acbramley could you provide a bit more information about the issue you have faced? I'm not being able to reproduce it on route diff.revisions_diff eg. /node/1/revisions, when selecting also "Show primary tabs" on "Secondary tabs" blocks.

acbramley’s picture

@sokru you need to have at least 1 other secondary tab to trigger it.

liam morland’s picture

Status: Needs work » Needs review
StatusFileSize
new1.36 KB

This patch is a port of the patch in #3169729: No local tasks on compare revisions page. It should avoid the problem mentioned in #12.

acbramley’s picture

Status: Needs review » Needs work

Please use a merge request, patch files are not accepted on this project anymore.

liam morland’s picture

Status: Needs work » Needs review

Merge request with patch #17.

There are also other changes to get phpstan to pass again.

adamzimmermann’s picture

I'm using 2.0.0-beta3 and the changes in MR 64 work perfectly for me and solved a client request. Thank you!

So RTBC from that perspective, but I see that this MR targets the 8.x branch, so I'll refrain from updating the issue Status for now.

liam morland’s picture

Version: 8.x-1.x-dev » 2.x-dev

Rebased on 2.x.

sokru’s picture

Priority: Minor » Normal
Status: Needs review » Needs work
Issue tags: +Needs tests

This needs tests. I'd recommend adding tests from https://www.drupal.org/files/issues/2023-10-05/2954352-diff-show-local-t...
and separate test for issue reported on #12.

adamzimmermann’s picture

StatusFileSize
new695 bytes

I have a commit ready to add the tests, but I don't seem to have access to the MR.

I'm not sure we need a test for the issue reported in #12 since the new approach doesn't have that issue, and from my experience of trying to do it the wrong way too (before I found this issue), the fact that we can load the node edit page earlier in the test proves there isn't an issue.

EDIT: Not sure why I didn't see the get push access button before, but I found it and was able to push my commit.

adamzimmermann’s picture

Status: Needs work » Needs review
sokru’s picture

Status: Needs review » Needs work

This still needs work for issue mentioned on #12 (and #16). So it needs steps to reproduce, fix for that and tests to make sure it won't get regression.

liam morland’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

The merge request avoids the issue in #12 and it has a test.

acbramley changed the visibility of the branch 8.x-1.x to hidden.

acbramley changed the visibility of the branch 2.x to hidden.

acbramley changed the visibility of the branch 2954352-local-tasks-for to hidden.

acbramley’s picture

acbramley’s picture

Status: Needs review » Needs work

I've tested the new approach and while it does fix the issue in #12 I'm not sure the performance overhead of this is something I'd be keen to commit to.

I debugged through and that alter hook is run on every single page load of a revision comparison page. None of the static caching for the tasks or route information works since we're not on the canonical route which means it's doing a lot of work to get those tasks, we also don't have test coverage for the bug in #12.

To reproduce it, you just need another secondary task under Revisions, e.g we have this so the secondary tabs render when on a Revisions route.

entity.node.version_history_sub:
  route_name: entity.node.version_history
  parent_id: entity.node.version_history
  title: 'Revisions'

Core has a similar pattern with Content moderation to render an Overview secondary tab on admin/content so the Moderated content tab shows.