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

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
Closed
Labels
Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Milestone

Comments

@Joey3000
Copy link
Contributor

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
Copy link
Contributor Author

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

@mattab
Copy link
Member

mattab commented Sep 20, 2015

fixed in #8807

@mattab mattab closed this as completed Sep 20, 2015
@mattab mattab added the Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. label Sep 20, 2015
@mattab mattab added this to the 2.15.0 milestone Sep 20, 2015
@Joey3000
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

Fixed in #8831

@mattab mattab changed the title Hard-coded login link on the exception page 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
Labels
Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Development

No branches or pull requests

2 participants