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

Use current Login auth plugin instead of hard-coded 'Login' on the error page #8808

Closed
Joey3000 opened this Issue Sep 17, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@Joey3000
Contributor

Joey3000 commented Sep 17, 2015

Issue

When an exception page is shown, it has following links underneath the exception message:

Go Back | Go to Piwik | Login

The issue is that that Login link is hard-coded, as can be seen in https://github.com/piwik/piwik/blob/2.15.0-b7/core/testMinimumPhpVersion.php#L163 (using a released version here, just so that the line number does not change). That is a problem for the many third-party login plugins already in the Plugin Marketplace. Since if the user has a different login plugin installed and clicks on the above Login link, another exception page will be shown saying that the Login plugin is not active. (And so on in a circle, until the user clicks on Go to Piwik.) So, basically, that link is not usable with other login plugins and only leads to user confusion.

Solution

There are following two options:

a. Replace the hard-coded Login name with Piwik::getLoginPluginName(), which is currently used across the platform specifically to find out the name of the currently active login module.

The disadvantage of the above approach (a) is that it makes the exception page dependent on more things which could possibly fail. I haven't checked, but that could result in the exception page failure to appear, if something goes wrong on login plugin name acquisition, throwing another exception (cascading exceptions resulting in an uncaught error output?).

So, here is a simple and safe option:

b. Remove the Login link from the page.

What also speaks for this solution is the fact that the Login link is not really necessary. Because when a user clicks on Go to Piwik, they will:
(i) if logged in: return to their dashboard
(ii) if not logged in: be gently escorted to the login page

Either option is trivial to implement. I could submit a pull request, if it's decided which one to go with. BUT: I guess that that would cause a Travis "expected screenshot" test to fail due to the changed page appearance. And I won't be able to get a hold of the new image - see #8781 (comment).

Additional Point

Since there is also another Back to Piwik link at the very bottom of the exception page, which links exactly where the above Go to Piwik does, one of them could be removed. Or not. I'm not a designer. :)

@Joey3000

This comment has been minimized.

Show comment
Hide comment
@Joey3000

Joey3000 Sep 17, 2015

Contributor

Other cases of a hard-coded Login plugin name are in the following PR: #8807

Contributor

Joey3000 commented Sep 17, 2015

Other cases of a hard-coded Login plugin name are in the following PR: #8807

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Sep 20, 2015

Member

fixed in #8807

Member

mattab commented Sep 20, 2015

fixed in #8807

@mattab mattab closed this Sep 20, 2015

@mattab mattab added the Task label Sep 20, 2015

@mattab mattab added this to the 2.15.0 milestone Sep 20, 2015

@Joey3000

This comment has been minimized.

Show comment
Hide comment
@Joey3000

Joey3000 Sep 20, 2015

Contributor

fixed in #8807

Actually no. This here is a different case of a hard-coded login link. I haven't submitted a PR for this issue, because I don't know which solution (a or b) is preferred.

Contributor

Joey3000 commented Sep 20, 2015

fixed in #8807

Actually no. This here is a different case of a hard-coded login link. I haven't submitted a PR for this issue, because I don't know which solution (a or b) is preferred.

@mattab mattab reopened this Sep 20, 2015

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Sep 20, 2015

Member

b. Remove the Login link from the page.

Sounds good to me 👍

Member

mattab commented Sep 20, 2015

b. Remove the Login link from the page.

Sounds good to me 👍

Joey3000 added a commit to Joey3000/piwik that referenced this issue Sep 21, 2015

@Joey3000

This comment has been minimized.

Show comment
Hide comment
@Joey3000

Joey3000 Sep 21, 2015

Contributor

Fixed in #8831

Contributor

Joey3000 commented Sep 21, 2015

Fixed in #8831

@tsteur tsteur closed this in #8831 Sep 21, 2015

@mattab mattab changed the title from Hard-coded login link on the exception page to Use current Login auth plugin instead of hard-coded 'Login' on the error page Oct 13, 2015

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