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

Don't try to display a custom logo or favicon if they don't exist #10000

Merged
merged 4 commits into from Apr 11, 2016

Conversation

Projects
None yet
4 participants
@ksubileau
Copy link
Contributor

commented Apr 4, 2016

A possible fix for #9966

@quba

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2016

Gratz! Issue/pr nb #10000 :)

@@ -163,6 +163,16 @@ protected static function rewritePath($path)
return SettingsPiwik::rewriteMiscUserPathWithInstanceId($path);
}
public static function hasUserLogo()
{
return file_exists(Filesystem::getPathToPiwikRoot() . '/' . CustomLogo::getPathUserLogo());

This comment has been minimized.

Copy link
@tsteur

tsteur Apr 5, 2016

Member

BTW: Here you could use PIWIK_INCLUDE_PATH . CustomLogo::getPathUserLogo() and same below

This comment has been minimized.

Copy link
@ksubileau

ksubileau Apr 5, 2016

Author Contributor

I copied the code from the existing hasSvgLogo and getPathToLogo methods for consistency :). Is there any difference between the static method and the constant? Should I still use the constant?

This comment has been minimized.

Copy link
@tsteur

tsteur Apr 5, 2016

Member

good point, I didn't notice that. Consistency is always good. We usually always use PIWIK_INCLUDE_PATH but not sure why Filesystem::getPathToPiwikRoot() was used there is there's no difference and the constant should be better. I'm ok with Filesystem::getPathToPiwikRoot() in this case. To remove the duplicated code though maybe there could be a method like

private function getRootPath()
{
    return Filesystem::getPathToPiwikRoot() . '/';
}
// or 
private function getAbsolutePath($reference)
{
    return Filesystem::getPathToPiwikRoot() . '/' . $reference;
}

This comment has been minimized.

Copy link
@ksubileau

ksubileau Apr 6, 2016

Author Contributor

As you prefer, we can also replace all calls to this method in this file by the constant.
If we add a new method getAbsolutePath, I think it should be in the Filesystem class, isn't it ?

This comment has been minimized.

Copy link
@tsteur

tsteur Apr 7, 2016

Member

Somehow I would prefer not to have it in Filesystem but would be also ok there. Having a look at this again in the code there is often file_exists($pathToPiwikRoot . '/' . $customLogo) etc, finding several file_exists. The code seems quite confusing in there in general and needs to be refactored at some point. Maybe there could me instead a method

private function logoExists($reference)
{
    return file_exists(Filesystem::getPathToPiwikRoot() . '/' . $reference);
}

Of course this could be as well in Filesystem but might be as well fine in CustomLogo for now

This comment has been minimized.

Copy link
@ksubileau

ksubileau Apr 9, 2016

Author Contributor

Done in 637b839 ;)

else {
// Upload succeed, so we update the images availability
// according to what have been uploaded
if(isSubmittingLogo) {

This comment has been minimized.

Copy link
@tsteur

tsteur Apr 5, 2016

Member

just a minor thing, do you mind adding a whitespace before the opening brackets?

This comment has been minimized.

Copy link
@ksubileau

ksubileau Apr 5, 2016

Author Contributor

Done !

@tsteur

This comment has been minimized.

Copy link
Member

commented Apr 5, 2016

I tested it and works great 👍 Just made two really minor comments but after those changes we can merge. Thank you for that and for getting this done so quick 👍

@tsteur tsteur added this to the 2.16.2 milestone Apr 5, 2016

@tsteur tsteur added the Needs Review label Apr 5, 2016

@tsteur

This comment has been minimized.

Copy link
Member

commented Apr 11, 2016

Awesome 👍 👍 Thanks for this. Merging now

@tsteur tsteur merged commit 120a7f7 into matomo-org:master Apr 11, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@ksubileau ksubileau deleted the ksubileau:9966 branch Apr 16, 2016

@mattab mattab added the Bug label Aug 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.