django-cms icon indicating copy to clipboard operation
django-cms copied to clipboard

Language fallback without redirect doesn't work on homepage

Open sephii opened this issue 8 years ago • 11 comments

Summary

For some reason if you configure CMS_LANGUAGES the following way:

'default': {
    'fallbacks': ['en', 'fr'],
    'redirect_on_fallback': False,
    'public': True,
    'hide_untranslated': False
},

Where you'd expect a fallback without redirect for every language, you'll still get a redirect fallback for the homepage because of the following line https://github.com/divio/django-cms/blob/097fe820a05c9b9efd63c36d7f009bf77dff60bb/cms/views.py#L110. I don't see any reason why the homepage should behave differently from other pages. I think the or slug == "" condition should be removed, or, if there's a good reason for doing so, there should at least be a way to disable it.

Expected behaviour

All pages honour the redirect_on_fallback setting, even the homepage.

Actual behaviour

The homepage always redirects on fallback, no matter what the value of redirect_on_fallback is.

Environment

  • Python version: 3.4.2
  • Django version: 1.9.12
  • django CMS version: 3.4.1

sephii avatar Jan 06 '17 12:01 sephii

Thanks @sephii, Traced back to https://github.com/divio/django-cms/pull/2736

czpython avatar Jan 16 '17 15:01 czpython

Can I do a pull request for this or is there still a decision to be made?

alculquicondor avatar Oct 16 '17 16:10 alculquicondor

Personally I prefer the system to stay consistent, I wonder whether somebody could be relying on this behavior now though?

But I definitely like to idea of making that condition configurable & documenting it properly!

viktor-yunenko avatar Aug 06 '21 12:08 viktor-yunenko

hey, I would like to work on this issue. I'd appreciate some guidance as I'm new to open source.

zain93393 avatar Sep 26 '23 12:09 zain93393

@zain93393 Hi there! Great news. There's already some information on the issue and how to fix it. Since it is quite old, you might want to check if the corresponding line

https://github.com/django-cms/django-cms/blob/097fe820a05c9b9efd63c36d7f009bf77dff60bb/cms/views.py#L110

has changed since then.

To create a pull request (PR), you best fork the django-cms repo create a new branch fix/issue-5839 and make the changes. Once you have committed the changes to your new branch, you can create a PR from the github page of your clone.

Join the Slack channel #workgroup-pr-review to look for someone to review your PR.

It is good practice to also add a test that verifies the new behavior. But that might be a second step.

Please let me know if you have any questions!

fsbraun avatar Sep 26 '23 13:09 fsbraun

I can take a look at this and pair it with fellow djangonauts to work this out.

@fsbraun I can't seem to add a label to this, can you please update the label to djangonaut for this issue. So it gets added to the djangonaut project board. 😊

Pradhvan avatar Jan 17 '24 10:01 Pradhvan

Hello everyone! On line 177, a check renders a 404 if the page for which the request language doesn't exist and there are no fallbacks. https://github.com/django-cms/django-cms/blob/ed7e2064e023c29b169084dd53a3b8643ac66715/cms/views.py#L170-L186

Interestingly, there are no calls to _handle_no_page() after that. So, if redirect_on_fallback is False, homepage or not, we'll get a 200 rendering the main language's page with no content.

This issue traces back to #2736, and it was mentioned in the linked issue that the alternative to redirection is 404.

So, a solution is to add redirect_on_fallback to the condition on 177 (page.is_home on the following condition becomes redundant so it gets removed)

    if (language_is_unavailable and
            (not fallback_languages or not redirect_on_fallback)):
        # There is no page with the requested language
        # and either there are no configured fallbacks or
        # the user has explicitly requested not to
        # redirect on fallbacks.
        return _handle_no_page(request)
    elif language_is_unavailable and redirect_on_fallback:
        [...]

This will have the side-effect of rendering a 404 for any page if redirect_on_fallback is False (when the request language is unavailable of course).

Before I make the PR, I just want to ask if this is the way to go or not.

sakhawy avatar Jan 25 '24 23:01 sakhawy

I think you are going the right way. This is something I would expect. Does this also solve this issue?

fsbraun avatar Jan 25 '24 23:01 fsbraun

Yes! I just needed confirmation. PR is on the way. I was just worried that the default behavior for a False redirect_on_fallback wasn't 404.

sakhawy avatar Jan 25 '24 23:01 sakhawy

Ah, I understand. But I fear according to the docs the situation should be:

  • No public fallback language: 404 (independent of redirect_on_fallback
  • If public fallback:
    • redirect_on_fallback = True: Redirect to the fallback language (at its URL which might, e.g., be translated)
    • redirect_on_fallback = False: needs to display the fallback language under the original URL (i.e. display, no 404, and no redirect.)

That is the logic for any page, but not the home page apparently.

fsbraun avatar Jan 25 '24 23:01 fsbraun

Thanks for clearing that up! I'll make the appropriate changes.

sakhawy avatar Jan 26 '24 01:01 sakhawy