localgovdrupal / localgov

Installation profile for the LocalGov Drupal distribution.
GNU General Public License v2.0
83 stars 19 forks source link

Patch Preview Link for bug where references to other entities being previewed don't display #785

Closed stephen-cox closed 1 month ago

stephen-cox commented 1 month ago

What does this change?

Patched Preview Link for issue #771, drupal.org issue: https://www.drupal.org/project/preview_link/issues/3481523

The patch changes the node access hook so Preview Links access handler is run for all pages included in a preview link entity. Without the hook it only runs for the page being viewed.

How to test

See https://github.com/localgovdrupal/localgov/issues/771#issuecomment-2373991933 on how to reproduce the issue this should fix.

How can we measure success?

Previewing more than one page becomes easier and works as people expect.

Have we considered potential risks?

This changes an access handler so has the potential to allow more than it should.

stephen-cox commented 1 month ago

Tests failures are fixed with https://github.com/localgovdrupal/localgov/pull/787

ekes commented 1 month ago

To clarify if I understand what your patch is doing: Previously the access check override looked at the route, checked if it is in preview mode, checked if the route entity is one that can be previewed. Now the access check, looks if it's in preview mode, runs through all entities that can be previewed, and sees if there's a match?

ekes commented 1 month ago

Hit wrong button, was just the question :-)

stephen-cox commented 1 month ago

Previously the access check override looked at the route, checked if it is in preview mode, checked if the route entity is one that can be previewed. Now the access check, looks if it's in preview mode, runs through all entities that can be previewed, and sees if there's a match?

That's right - previously is only checked access for the entity being previewed but now checks whether the entity is included in a related preview link.

finnlewis commented 1 month ago

@ekes is happy with this, let's merge it!