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

Improve BO speed by removing menu images #8780

Merged
merged 2 commits into from Mar 16, 2018

Conversation

@alegout
Copy link
Contributor

commented Feb 20, 2018

Questions Answers
Branch? 1.7.3.x
Description? Improve speed by removing menu images, apparently not used by us. Maybe contributors? -72% on blackfire and /product/catalog page
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? no
How to test? profile BO pages.

Important guidelines


This change is Reviewable

@alegout

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2018

capture d ecran 2018-02-20 a 20 27 49

@alegout

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2018

Another file_exists call we can remove after for the image.

@alegout

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2018

Now:

capture d ecran 2018-02-21 a 10 06 47

$path_img = _PS_MODULE_DIR_.$tab['module'].'/'.$tab['class_name'].'.png';
// Relative link will always work, whatever the base uri set in the admin
$img = '../modules/'.$tab['module'].'/'.$tab['class_name'].'.png';
}

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 Feb 21, 2018

Member

By seeing these lines removed, I'd say you remove a feature.
Module developers can add their AdminModuleController tab icon in the root folder of their module.

This comment has been minimized.

Copy link
@alegout

alegout Feb 21, 2018

Author Contributor

Yes, but is this really used and needed?

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 Feb 21, 2018

Member

(Forwarded an email about that)

@kpodemski

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2018

Images are not used from version 1.5 of PrestaShop, of course you could add them but it's a common to use font-icons now instead, for me it's a good change 👍

@alegout

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2018

Ok cool, thanks for you answer @kpodemski.

And in fact, more you have links in the menu (installed module, etc...) more you gain speed.

@kpodemski

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2018

Yeah, totally, also - if someone still want to use an image, there's still an option to do that via CSS, so you shouldn't worry about acceptation of this PR @Quetzacoalt91 ;)

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

commented Feb 21, 2018

But you know we have to respect the semver rules, we cannot remove features like this in a patch version (1.7.3.0 won't have this PR, even if we approve it immediately).

I know the consequences may be awesome, but the module related code is the only one I'd keep, because we can't breaks things for our contributors without prior notice.

@kpodemski

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2018

Ok, but this feature is not available anymore since... i don't remember :D You can try to add this image and you will not see it in back-office anyway... so yeah, we scan for the images but those images are not displayed...

@eternoendless

This comment has been minimized.

Copy link
Member

commented Feb 21, 2018

I think that if we ever decide to keep this feature it should be done via config instead of via filesystem probing, because as blackfire shows, it's extremely slow :)

@alegout

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2018

Even if a module has an image, we don't use it in the template file.
So i think it's safe... For me, module image feature is broken since we removed it in the template?

@eternoendless eternoendless added this to the 1.7.3.1 milestone Feb 21, 2018

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

commented Feb 21, 2018

Both SF and legacy?

@kpodemski

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2018

@alegout exactly, it's not used anymore for years...

@Quetzacoalt91 yes, it's not used anymore, even in PrestaShop 16...

@Quetzacoalt91
Copy link
Member

left a comment

Go on then, I'll trust what you guys say.

@alegout

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2018

yes @Quetzacoalt91 checked in legacy and SF themes.

@kpodemski

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2018

@mickaelandrieu
Copy link
Member

left a comment

As it's part if AdminController, this have a positive performant interest for every page of back office :-)

👍 for me too, maybe explain what need to be tested can help for QA validation ;)

@eternoendless

This comment has been minimized.

Copy link
Member

commented Mar 16, 2018

Thank you @alegout

@eternoendless eternoendless merged commit b3f886b into PrestaShop:1.7.3.x Mar 16, 2018

2 of 3 checks passed

code-review/reviewable 1 file left (colinegin)
Details
Codacy/PR Quality Review Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.