Views UI preview does not respect changed 'path' option until saving

Confirmed this is still a bug.

Thanks for confirming @Kristen Pol. I am working on this and get back soon with resolution.

Please review the patch. Now the path url is getting from the views display handler path. Previously it was coming from views url. Please let me know if the approach not correct.

Thanks for the patch. Tests fail so changing to needs work.

I am working on the fix for the test case failure.

Thanks. I see that this is two lines but could be one. Is there some reason you split this?

+++ b/core/modules/views_ui/src/ViewUI.php
@@ -562,7 +563,8 @@ public function renderPreview($display_id, $args = []) {
+        $url = Url::fromUserInput('/' . $executable->display_handler->getOption('path'));
+        $path = $url;

Could change to:

$path = Url::fromUserInput('/' . $executable->display_handler->getOption('path'));

Please review the patch.
@kristen-pol Thanks for the suggestion we can do that in single line. For better readability i have done that in two lines but we can merge those 2 lines into 1.

Reviewing this for DrupalCamp Colorado contribution day.

Thanks for the update. Marking RTBC since:

1) Patch still applies cleanly to 9.1.x.

2) Tests pass and I'm making the assumption that additional tests aren't needed.

3) Changes are straight forward and address the issue in the issue summary.

4) Title and issue summary are clear.

5) Passes manual review. See screenshots.

6) Note: updated issue summary to use template and filled in some information.

$executable->getUrl() does a lot more than display the path, it has argument parsing etc.

Are we losing that functionality if we make this change?

Can we manually test previewing a view with contextual filters, and arguments entered into the preview form before and after this patch and ascertain if there is a change in behaviour.

And if so, that indicates we don't have tests, which also indicates we should at least be adding some here.

Tagging for both those tasks.

Thanks @larowlan for your review. Since no one has picked this for manual testing let me explain the patch.

Here i have not changed how $executable->getUrl() is working, i just changed the $path variable that picks the current views path and display that on Update Preview button click.

This feature is covered by the test case also.
Also i have made small GIF as per your comment using contextual filter. Please let me know if i am taking thing other way what you have suggested.

I have checked this issue and found that it is working fine for D 9.1.x after applying #19 patch & Please find the screen shot attached for the same below . Following are the steps done to test & verify the same :

Steps to Test

1. Create or edit a view with page display
2. Make sure auto preview is enabled
3. Apply the patch
4. Update the page path more than once (observe the path before update )
5. Notice the path changes are shown in the preview (observe the path after update )

I tested this manually, and I think it should be closed (works as designed) - here's my reasoning

• The preview outputs the path as a link
• When you edit the path, the view is not yet saved
• So Drupal's routes have not yet been updated to reflect the new view path
• So with the current patch, we immediately update the link in the preview
• But clicking this link yields a Page Not Found, because the view has not been saved, and therefore Drupal's routes have not yet been updated
• In HEAD, when you save the view, the link is updated. But not beforehand, because the link has nowhere to link to yet.

I will seek a second opinion from others in the Bug Smash Initiative

I initially agreed with #25 but noticed two things:

1. 'View Page' button in the top right updates before the view is saved, so this already links to a 404
2. 'Title' of the view in the preview section updates before the view is saved

So I guess that updating the path in the preview at least makes it consistent with the rest of the behaviour. The fact that we know it's going to be a broken link is kind of odd but there already is one in the button.

Tried to form an opinion about this one but ¯\_(ツ)_/¯

The fact that we know it's going to be a broken link is kind of odd but there already is one in the button.

So, I think if we update this to the current route, we would need to check if the Url is valid before outputting it as a link.

Or we can go a different route and check if $executable->getUrl() !== $executable->display_handler->getOption('path') and then output a different text in te preview, something like:
[link-to-not-updated-patg] (will be [new-path] after saving the View)

I don't know, the text could probably be better, but hopefully you get my point.

I tried to display both the URLs in list to see how it looks like.
I've updated the patch to review it.

This is how it looks in backend:

I think we can also ask someone from the usability team to get a better understanding.

Regarding #27, IMO I think outputting the path as plain text instead of a link is fine, since as noted in #25, the page doesn't exist yet because the view hasn't been saved. I feel like this is a bug or, at least, a UX issue. I would expect the path to show up properly in the preview, or not show at all.

Patch #29 seems to be a move in the right direction, however, as it is in my testing, the View Page still returns a page not found. AS #25 pointed out, Title updates before being saved, so I belive both should behave in the same way, both or none should update before saved, personally I beleive none should, as it feels more intuitive to apply changes through a save button. The new path after view is saved should be only a preview string, because as it is it also leads to a page not found.

Applied patch #29 and it worked fine.Adding screenshots below.

Before patch:

After patch:

RTBC +1

I like these new approach of showing both, but IMO I think the "New path after view is saved" path should not be linked because that page doesn't necessarily exist. So, for consistency, IMO I would argue that the link could be left out for *both* of these. Or like mentioned in #27 and #31, at minimum check if the path exists before linking it. IMO I think it's preferable just not linking them. The linked pages will usually be an old version of the view, so IMO having the link doesn't really add value and might even cause confusion.

- Update code to display plain url for the new URL so it is not clickable.
- Added small test case to ensure that URL is visible along with label.

Tested new patch, seems good to go!

The preview generates a plain preview, and the there is no way to access paths that doesn't exist. The preview button only updates when the changes are saved.
Will wait for the test to end to move to RTBC.

Test cases seem to be passing on local.
It might be the case that as test cases are running inside the separate directory, path might be incorrect.
I've refactored test cases to ensure that we assert label and URL seperately.
Let's see how it goes.

As I stated in the comment #35, now seems good to go.

+++ b/core/modules/views_ui/src/ViewUI.php
@@ -697,7 +698,22 @@ public function renderPreview($display_id, $args = []) {
             if (isset($path)) {
...
+              if ($current_path->getGeneratedLink() !== $updated_path) {
...
+              else {
...
             else {

we've got some nested if/else going on here.

I think this would be easier to maintain if we split this off into a new method, e.g getPreviewPath or similar, that way we could reduce a lot of the nesting with early returns.

1. +++ b/core/modules/rest/tests/modules/rest_test_views/test_views/views.view.test_serializer_display_field.yml
   @@ -81,7 +81,7 @@ display:
   -      path: test/serialize/field
   +      path: /test/serialize/field
   

   Why was this change needed? I can't see any mention of it in the issue.

2. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -562,7 +563,7 @@ public function renderPreview($display_id, $args = []) {
   -        $path = $executable->getUrl();
   +        $path = Url::fromUserInput('/' . $executable->display_handler->getOption('path'));
   

   Why did we need this change? See my comment above:

   $executable->getUrl() does a lot more than display the path, it has argument parsing etc.

   Are we losing that functionality if we make this change?

Patch #37 applied successfully in drupal 9.3.x-dev version and the view path getting updated immediately. The path did not previewed as a link. Adding screenshots for reference.

Moving back to needs work based on #39.

Also, from #41, it looks like there is no space after the ":" and before the "/" so that should be added when the patch is updated.

Fixing @larowlan's comments from #39
- I think, yml file change was unnecessary so I've removed it.
- Refactored the logic in a separate method, test cases are passing now.
- The $path variable is not overridden anymore. I think it is safer now.

Included @Kristen Pol's suggestion from #42 as well.

@mohit_aghera , updated view configuration as tests are failing without these configs.

i'm working on this.

I have tested the patch, and it works with Drupal 11.1.1. I’ve also updated the patch to align with the latest codebase, as the original was created 4 years ago.

Additionally, I have resolved all PHPCS and JavaScript linting issues. However, I’m encountering PHPStan errors related to missing return types.

Locally, all level 1 issues appear to be fixed, but the CI pipeline still fails due to return type errors. Despite updating all methods in the ViewUI file, the number of reported issues has increased.

I need guidance on resolving this discrepancy between local and pipeline results. Marking this as NR.

The view changes almost seem out of scope but the baseline would need to be regenerated

Fixed all the phpstan issues.
Resolved merge conflicts and removed changes related to views as this wasn't necessary.
There is one more failure in the CI, I'm going through that.

I've hidden the patches in favor of MR approach.