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.

Sign up for GitHub

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

Jump to bottom

Fixed Apache Optimization #9244

Merged
merged 1 commit into from
Aug 6, 2018
Merged

Fixed Apache Optimization #9244

merged 1 commit into from
Aug 6, 2018

Conversation

Pedrock
Copy link
Contributor

@Pedrock Pedrock commented Jul 1, 2018 •
edited
Loading

Questions Answers
Branch? develop
Description? When Apache optimization is enabled in the CCC settings, the .htaccess is written as if the cache control was disabled
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? - Setup a shop
- Go to Advanced Parameters -> Performance and enable Apache optimization
- The browser should now be caching all asset files, including JS and CSS (the main .htaccess should include "ExpiresActive On" and a "ExpiresByType" for each file type)

This change is Reviewable

@prestonBot
Copy link
Collaborator

Hello @Pedrock!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@prestonBot prestonBot added the 1.7.4.x Branch label Jul 1, 2018
@mickaelandrieu mickaelandrieu changed the title CO: Fix Apache Optimization Fixed Apache Optimization Jul 3, 2018
@mickaelandrieu mickaelandrieu added the Bug Type: Bug label Jul 3, 2018
eternoendless
eternoendless requested changes Jul 12, 2018
View reviewed changes
src/Adapter/Cache/CombineCompressCacheConfiguration.php Outdated
@@ -194,7 +194,7 @@ private function manageApacheOptimization($enabled)

// feature activation
if (false === $isCurrentlyEnabled && true === $enabled) {
if ($this->tools->generateHtaccess()) {
if ($this->tools->generateHtaccess(null, null, 1)) {
$this->configuration->set('PS_HTACCESS_CACHE_CONTROL', true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the configuration should be set before calling generateHtaccess()? That way we wouldn't need to pass any parameters.

Copy link
Contributor Author

@Pedrock Pedrock Jul 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would simplify the change but the user won't have any kind of feedback if the operation fails. You can tell me how you prefer it I can change it accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eternoendless

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would that be? You just need to tweak the code a little like this:

$this->configuration->set('PS_HTACCESS_CACHE_CONTROL', true);
if (!$this->tools->generateHtaccess()) {
    $errors = array(
       // ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eternoendless Thank you for the feedback. I have updated the code

eternoendless
eternoendless previously approved these changes Jul 24, 2018
View reviewed changes
@eternoendless
Copy link
Member

Thanks for the changes! Would you mind rebasing onto develop?

@Pedrock Pedrock force-pushed the fix-apache-optimization branch 2 times, most recently from f5e0a1d to 3cd294d Compare July 24, 2018 13:12
@Pedrock
Copy link
Contributor Author

Pedrock commented Jul 24, 2018

@eternoendless Do you want me to rebase onto develop and change the target branch of the PR to develop or just to rebase onto 1.7.4.x since this is the current target branch of the PR?

@eternoendless
Copy link
Member

Rebase into develop and change the target branch please.

@Pedrock Pedrock changed the base branch from 1.7.4.x to develop July 24, 2018 16:29
@prestonBot prestonBot added 1.7.4.x Branch Bug Type: Bug develop Branch labels Jul 24, 2018
@Pedrock
Copy link
Contributor Author

Pedrock commented Jul 24, 2018

@eternoendless done!

@PierreRambaud PierreRambaud removed the 1.7.4.x Branch label Jul 24, 2018
@eternoendless eternoendless added this to the 1.7.5.0 milestone Jul 27, 2018
@PierreRambaud PierreRambaud added the Waiting for QA Status: action required, waiting for test feedback label Jul 27, 2018
@ntiepresta ntiepresta self-assigned this Jul 30, 2018
@ntiepresta ntiepresta added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Jul 30, 2018
@ntiepresta ntiepresta removed their assignment Jul 30, 2018
@Quetzacoalt91
Copy link
Member

Travis ran properly, but sha1 don't match: https://travis-ci.org/PrestaShop/PrestaShop/builds/407597070

Merging.

@Quetzacoalt91 Quetzacoalt91 merged commit 986b628 into PrestaShop:develop Aug 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eternoendless eternoendless eternoendless approved these changes

Assignees
No one assigned
Labels
Bug Type: Bug develop Branch QA ✔️ Status: check done, code approved
Projects
None yet
Milestone
1.7.5.0
Development

Successfully merging this pull request may close these issues.
7 participants
@Pedrock @prestonBot @eternoendless @Quetzacoalt91 @mickaelandrieu @PierreRambaud @ntiepresta