Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Form submit for "Show menu in language" should strip existing destination #6824

Open
indigoxela opened this issue Jan 14, 2025 · 8 comments · May be fixed by backdrop/backdrop#4997
Open

Form submit for "Show menu in language" should strip existing destination #6824

indigoxela opened this issue Jan 14, 2025 · 8 comments · May be fixed by backdrop/backdrop#4997

Comments

@indigoxela
Copy link
Member

indigoxela commented Jan 14, 2025

Description of the bug

Otherwise it won't be possible to switch the language when coming from some other page (with destination set).

Steps To Reproduce

  1. Set up a multilingual site 😁
  2. Menu items in different languages and all that should be available...
  3. Go to any frontend page and click the "Edit links" contextual link for a menu block
  4. You get to the menu you'd like to adapt, but filtered by English
  5. Then you try to filter by the other language via "Show menu in language" Filter button

Actual behavior

Instead of getting the filtered menu items, you get redirected back to the initial page.

Expected behavior

On the same page, the menu links overview, I'd expect to get the menu items per selected language.

Additional information

That happens, because the contextual link provides a destination back to the page you started from and submitting the Filter button gets you back there instead of filtering.

My workaround is to strip off the destination before I filter the items, but that's far from ideal.

@olafgrabienski
Copy link

Thanks for the report, @indigoxela! Following the steps, I was able to reproduce the issue.

Do you already have an approach in mind how to fix the issue without losing the link destination?

@argiepiano
Copy link

argiepiano commented Jan 20, 2025

PR submitted: backdrop/backdrop#4997

The issue was that, in menu_overview_form_language_filter_submit(), after getting the destination and creating the url query, the global $_GET['destination'] must be unset. This is commonly done in other forms. Otherwise, as explained in backdrop_goto docblock:

 * If a destination was specified in the current request's URI (i.e.,
 * $_GET['destination']) then it will override the $path and $options values
 * passed to this function.

@olafgrabienski
Copy link

Thanks for the PR, @argiepiano ! It works for me: When I use the language filter, I do get the filtered menu items.

As a side note, I'm not sure if the destination link does a reasonable job in the context of the "Edit links" form. If I didn't miss anything, with the PR there is no situation where it applies.

@argiepiano
Copy link

argiepiano commented Jan 20, 2025

The destination is preserved in the URL with this PR, as it was the intention of the original code, so when you save the form it will redirect you to where you came from. This is the expected behavior for contextual links. So, this PR actually preserves that.

Stripping the destination, as suggested in the title, is not a good idea, as it will create an anomalous behavior for contextual links.

@olafgrabienski
Copy link

@argiepiano Sorry, my comment was not very clear. I wasn't suggesting to strip the destination, I just was not sure if it applies in the context if this form. Now I saw, it applies in one situation: when you go to the form, change the link order, and save the form. So, everything fine, the destination makes sense. (In contrast, the destination doesn't apply when you go to the form, and edit a link.)

@indigoxela
Copy link
Member Author

Awesome, before I can even try to remember, what my plan was, there's already a PR. 😆

Works like a charm! 👍

I'm not sure if the destination link does a reasonable job in the context of the "Edit links" form.

@olafgrabienski its benefit is limited, but it's helpful for just quickly reordering items - in that case getting back to the page you came from makes sense.

@indigoxela
Copy link
Member Author

A simple fix - possibly omitting the "unset" was just an oversight, anyway. As @argiepiano already stated, it's done that way in several other forms. RTBC 👍

@argiepiano
Copy link

Cool! Could one of you set a milestone for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants