Fix mod_login not considering Itemid when redirecting

[Erftralle]

Steps to reproduce the issue

• Install a fresh Joomla! 3.4.1 with sample testing data
• Go to the module manager and modify mod_login (Login Form) options Login Redirection Page to mainmenu/Sample Sites and Logout Redirection Page to mainmenu/Home
• Ensure SEO Setting Search Engine Friendly URLs is switched On (like it is standard after a fresh installation)
• Login and logout and check if redirection is working correctly, especially whether the correct modules are shown corresponding to the menu item id

Expected result

Pages redirected to should show the modules belonging to the menu item

Actual result

Pages redirected to don't show the correct modules

System information (as much as possible)

Joomla! 3.4.1

Additional comments

Due to the changes in #6034 now the Itemid is missing when calculating the redirection URL.
The patch should correct that.

[zero-24]

@test success Thanks @Erftralle

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6542.}

[Hackwar]

@test success

[zero-24]

RTC Thanks!

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6542.}

[Ruud68]

Hi, just tested this as possible solution for my issue #6549 but there is still a 'glitch':
The url is stil wrong:
e.g. it redirects to: https://mydomain/blog-overzicht?view=categories
where it should redirect to https://mydomain/blog-overzicht

[Hackwar]

@Ruud68 Are you sure that this is the fault of Joomla and not some third party code that messes around in the routing code? Can you post the URL of your site?

[Ruud68]

Hi @Hackwar , when reverting to a restore (Joomla 3.4) it works, when applying 3.4.1 it stops working (on multiple sites). The conclusion that I have is then that it is because of the patch :)
here is a testsite without this patch: https://office.x-ceed.nl/ab_test/ (logout should go to menu 'blog overzicht')
here is a testsite with this patch applied: https://office.x-ceed.nl/ll_test/ (logout should go to menu 'blog overzicht')

If you need anything please let me know :)

[Hackwar]

@Ruud68 the question isn't wether 3.4.1 is "broken" in that regard or not. The question is, if the fix that is provided in this PR fixes the issue. So far you've only said that 3.4.1 is broken, not that this PR does not fix your issues.

[Ruud68]

Hi @Hackwar problem was introduced with 3.4.1. reverting to 3.4 fixes it. Implementing your patch on 3.4.1 does alter the outcome of the resulting url (different), but problem is still there: please look at two provided testsites > you can create an account on both of the to see what happens.

To be short: patch does NOT fix problem :(

[infograf768]

Although it solves some of the issues here, the url obtained by the redirect are not the same as the url obtained by using directly the menu item targetted.
Example:
I redirect to
http:/mysite.com/submit-an-article.html

and I get when redirected:
http:/mysite.com/submit-an-article.html?view=form

So, same issue as stated above by @Ruud68 and no specific extension in the way.

[infograf768]

Let me add that reverting #6034 solves the issue here.

[joomdonation]

@Hackwar Compare the code in old version (3.4.0) I can see that something wrong:

• In case we are in SEF Mode and there is a menu item selected for Login / Logout redirect, the return link can simply be $url = 'index.php?Itemid=' . $item->id; Seems we don't need to append any vars to the url
• In case there is no menu item selected, the process is more complicated than what was implemented in 1.4.1. Need to check if there is Itemid variable.....

My quick merge code in both versions return this:

public static function getReturnURL($params, $type)
{
    $app    = JFactory::getApplication();
    $router = $app::getRouter();
    $menu   = $app->getMenu();
    $item   = $menu->getItem($params->get($type));
    if ($item && $item->link)
    {
        if ($router->getMode() == JROUTER_MODE_SEF)
        {
            $url = 'index.php?Itemid=' . $item->id;
        }
        else
        {
            $url = $item->link . '&Itemid=' . $item->id;
        }
    }
    else
    {
        // Stay on the same page
        $vars = $router->getVars();
        if ($router->getMode() == JROUTER_MODE_SEF)
        {
            if (isset($vars['Itemid']))
            {
                $itemid = $vars['Itemid'];
                $item   = $menu->getItem($itemid);
                unset($vars['Itemid']);

                if (isset($item) && $vars == $item->query)
                {
                    $url = 'index.php?Itemid=' . $itemid;
                }
                else
                {
                    $url = 'index.php?' . JUri::buildQuery($vars) . '&Itemid=' . $itemid;
                }
            }
            else
            {
                $url = 'index.php?' . JUri::buildQuery($vars);
            }
        }
        else
        {
            $url = 'index.php?' . JUri::buildQuery($vars);
        }
    }

    return base64_encode($url);
}

This is just based on my code review. Maybe it could help.

[joomdonation]

Hmm. I wonder why in case "Stay on the same page", we cannot get the return url use:

$url = JUri::getInstance()->toString();

Then the code of the method getReturnURL can be:

public static function getReturnURL($params, $type)
{
    $app    = JFactory::getApplication();
    $menu   = $app->getMenu();
    $item   = $menu->getItem($params->get($type));
    if ($item && $item->link)
    {
        if ($app::getRouter() == JROUTER_MODE_SEF)
        {
            $url = 'index.php?Itemid=' . $item->id;
        }
        else
        {
            $url = $item->link . '&Itemid=' . $item->id;
        }
    }
    else
    {
        // Stay on the same page
        $url = JUri::getInstance()->toString();
    }

    return base64_encode($url);
}

It seems work well (with my test).

[Hackwar]

public static function getReturnURL($params, $type)
{
    $app    = JFactory::getApplication();
    $menu   = $app->getMenu();
    $item   = $menu->getItem($params->get($type));
    if ($item && $item->link)
    {
        $url = 'index.php?Itemid=' . $item->id;
    }
    else
    {
        // Stay on the same page
        $url = JUri::getInstance()->toString();
    }

    return base64_encode($url);
}

is the same

[joomdonation]

@Hackwar Yes, it is the same. Maybe @Erftralle can update your PR with the new code?

[infograf768]

#6542 (comment)

does work OK here. No more &view= etc.

[Erftralle]

Maybe @Erftralle can update your PR with the new code?

Done, with some minor modifications. Thanks.

[infograf768]

works but it will not if redirecting to an alias menu item as we would get a 404.
Maybe change to
if ($item && $item->type != 'alias')

What do you think?

[Erftralle]

Thanks for testing!

What do you think?

That would be acceptable for me.
It would be possible to retrieve the real Itemid from the aliasoptions of the menu parameters but this could be an alias as well so we would have to implement some kind of recursive algorithm.

[Hackwar]

No, it is not possible (without modifying the data directly in the DB) to select an alias as the target for an alias.

[Erftralle]

No, it is not possible (without modifying the data directly in the DB) to select an alias as the target for an alias.

Can't confirm that. I've just tested it (without modifying the data directly in the DB) with the current staging branch. Maybe it would be a good idea to prevent that.

[infograf768]

The other types of menu item which would give a 404 are
url, heading and separator

maybe the solution would be to prevent these types of menu from being proposed in the "menuitem" dropdown?

[Ruud68]

Hi, really want to help by testing. But I am unsure what changes to implement looking at all the snippets of code above. there is the risk that I will be testing the wrong code :(

So if we have something that I can test, can somebody summarize the changes that need to be implemented to ensure the testing of the correct code?

[joomdonation]

@infograf768 If that's the case, then Yes, we should do that. However, to simply the work, I think you can simply disable the menu items so that users cannot choose it. Right now, only separator menu item type is disabled(See https://github.com/joomla/joomla-cms/blob/staging/modules/mod_login/mod_login.xml#L45), you can modify it to disable="separator,alias,link,heading" .

Same for logout redirect url as https://github.com/joomla/joomla-cms/blob/staging/modules/mod_login/mod_login.xml#L54

[infograf768]

I propose to use this PR as it is for the helper + changing mod_login.xml to prevent proposing menu types which should not be used to redirect, i.e.

                <field
                    name="login"
                    type="menuitem"
                    disable="separator,alias,heading,url"
                    label="MOD_LOGIN_FIELD_LOGIN_REDIRECTURL_LABEL"
                    description="MOD_LOGIN_FIELD_LOGIN_REDIRECTURL_DESC" >
                    <option
                        value="">JDEFAULT</option>
                </field>
                <field
                    name="logout"
                    type="menuitem"
                    disable="separator,alias,heading,url"
                    label="MOD_LOGIN_FIELD_LOGOUT_REDIRECTURL_LABEL"
                    description="MOD_LOGIN_FIELD_LOGOUT_REDIRECTURL_DESC" >
                    <option
                        value="">JDEFAULT</option>
                </field>

This will grey-out the unwanted types:

[infograf768]

heh, looks like we had the same idea...

[joomdonation]

@infograf768 You are so fast :D.

[joomdonation]

@Ruud68 If you know how to edit code on your site, then you can follow the instruction below to test it:

• Open the file modules/mod_login/helper.php
• Replace the function method getReturnURL in that file with the code Hannes posted here Fix mod_login not considering Itemid when redirecting #6542 (comment)

I believe it will solve your issue. Then you can update the modified files to your live site while waiting for 3.4.2 released.

[Erftralle]

@infograf768 @joomdonation : Thanks for your help, modified PR according to your proposals.

[Ruud68]

Hi, just to confirm that proposed changes to helper.php and mod_login.xml worked on multiple sites.
Thanks all for fixing this :)

[infograf768]

I guess this can be merged now. :)

[zero-24]

RTC based on tests also here: #6549

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6542.}

[Ruud68]

Hi, just encountered a 'small' issue :( it is possible to select an unpublished menu entry as the redirect page resulting in unexpected behaviour. Probably adjust the mod_login.xml a little more?

[Erftralle]

Probably adjust the mod_login.xml a little more?

Do you mean to grey out unpublished menu items as well? That would make not much sense as it would always be possible to unpublish a menu item after selecting a redirect page in the module options.

One could check in the helper if the menu item is published and force to stay on the same page if not but unfortunately we would have to change the SQL query in JMenuSite::load() to make the published property available.

Have to correct myself.
As only published items are loaded in JMenuSite::load() the 'stay on the same' page behaviour is already working.

I personally would expect the site administrator to select a published redirect page and would expect him to check if the redirect is working as expected, but what do others think about that?

[joomdonation]

I personally would expect the site administrator to select a published redirect page and would expect him to check if the redirect is working as expected, but what do others think about that?

I agree. However, I think only show published menu items will help admin of the site find and select the menu item he wants easier.

I made small PR to your branch, just merge it and I think it would be OK.

[Erftralle]

However, I think only show published menu items will help admin of the site find and select the menu item he wants easier.

As I already stated above this makes not much sense for me because it is always possible to unpublish a menu item after a redirect has already been selected in the module options.
Don't showing up the unpublished items (instead of greying out them) makes it even worst because a once selected redirect, which has been unpublished afterwards, will not show up correctly in the module's redirect selection box (will display as 'Default', which is not the real setting).
This isn't ok for me!

[westiefan]

@test success