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

Fix: Cannot install Prestashop 8.1.3 on Windows #35052

Open
wants to merge 3 commits into
base: 8.1.x
Choose a base branch
from

Conversation

Codencode
Copy link
Contributor

Questions Answers
Branch? 8.1.x
Description? When trying to install a new copy of Prestashop 8.1.3 on Windows, the system throws an error and crashes.
Type? bug fix
Category? IN
BC breaks? no
Deprecations? no
Fixed issue or discussion? Fixes #35044

@Codencode Codencode requested a review from a team as a code owner January 16, 2024 11:27
@prestonBot
Copy link
Collaborator

Hi, thanks for this contribution!

I found some issues with the Pull Request description:

  • Your pull request does not seem to fix any issue, consider creating one (see note below) and linking it by writing Fixes #1234.

Would you mind having a look at it? This will help us understand how interesting your contribution is, thank you very much!

About linked issues

Please consider opening an issue before submitting a Pull Request:

  • If it's a bug fix, it helps maintainers verify that the bug is effectively due to a defect in the code, and that it hasn't been fixed already.
  • It can help trigger a discussion about the best implementation path before a single line of code is written.
  • It may lead the Core Product team to mark that issue as a priority, further attracting the maintainers' attention.

(Note: this is an automated message, but answering it will reach a real human)

@prestonBot prestonBot added 8.1.x Branch Bug fix Type: Bug fix labels Jan 16, 2024
@Hlavtox
Copy link
Contributor

Hlavtox commented Jan 16, 2024

Maybe I am wrong, but does it make sense to first warmup the cache and then clean it?

@PrestaShop/committers Guys could you please check it out? Installer doesn't work on Windows at all, it would be good if it was fixed. Maybe there are some other issues also.

@Codencode
Copy link
Contributor Author

Codencode commented Jan 16, 2024

Maybe I am wrong, but does it make sense to first warmup the cache and then clean it?

@PrestaShop/committers Guys could you please check it out? Installer doesn't work on Windows at all, it would be good if it was fixed. Maybe there are some other issues also.

In fact it doesn't make much sense, but if you first clear the cache with the "cache:clear" command, when the system tries to execute the "cache:warmup" command it generates an error because it is not found, I think precisely because it is the cache has been emptied and therefore also all connected services.

@Codencode
Copy link
Contributor Author

Original

Original

Original.mp4

With modification

With modification

With.modification.mp4

Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

I understand that changing the code this way seems to fix the situation but I think this patch is wrong. Because SymfonyCacheClearer is called from many places, for example when in the backoffice you upload a new module, and performing the warmup before the clearing is indeed not correct.

What I understand is: on Windows, when the warmup attempts to run, it fails because something is missing. No idea what, and as I am not running Windows, I can't verify.

My suggestion: I believe we should maybe change the logic inside installer().

Can you check @Codencode when the function SymfonyCacheClear::clear() is called, by monitoring the stack trace? My guess is that it is called when installing modules.

In PR #29695 was introduced a little setting ModuleManager::$systemClearCache that, when turned off, prevents the cache from being cleared.

Here is my suggestion to try:

  1. Set ModuleManager::$systemClearCache to false -> this will avoid calling the cache clearing step when modules are installed
  2. After all modules are installed, perform a clean cache clear then warmup

@Codencode
Copy link
Contributor Author

Codencode commented Jan 17, 2024

Hi @matks,
You're right, in fact it doesn't make much sense to reverse the instructions.
The error that is generated is the following:

ERROR:
"Failed opening required 'D:\cms\Prestashop\Site\prestashop813\var\cache\prod\ContainerA8Cu5F9\getConsole_Command_CacheWarmupService.php' (include_path='D:\cms\Prestashop\Site\prestashop813\vendor/pear/pear_exception;D :\cms\Prestashop\Site\prestashop813\vendor/pear/console_getopt;D:\cms\Prestashop\Site\prestashop813\vendor/pear/pear-core-minimal/src;D:\cms\Prestashop\Site\prestashop813\ vendor/pear/archive_tar;C:\xampp\8.1\php\PEAR')"

CALL STACK:

  • Symfony\Component\Console\Application->doRun (d:\cms\Prestashop\Site\prestashop813\vendor\symfony\symfony\src\Symfony\Component\Console\Application.php:240)
  • Symfony\Bundle\FrameworkBundle\Console\Application->doRun (d:\cms\Prestashop\Site\prestashop813\vendor\symfony\symfony\src\Symfony\Bundle\FrameworkBundle\Console\Application.php:83)
  • PrestaShop\PrestaShop\Adapter\Cache\Clearer\SymfonyCacheClearer->PrestaShop\PrestaShop\Adapter\Cache\Clearer{closure:D:\cms\Prestashop\Site\prestashop813\src\Adapter\Cache\Clearer\SymfonyCacheClearer.php: 63-109} (d:\cms\Prestashop\Site\prestashop813\src\Adapter\Cache\Clearer\SymfonyCacheClearer.php:103)

I also attach a debug video

2024-01-17-11-03-40_eFZQEEFR.mp4

Also the same problem occurs when deleting cache from admin.

Regarding the suggestion regarding ModuleManager::$systemClearCache, I couldn't find this attribute, in fact if you check here https://github.com/PrestaShop/PrestaShop/blob/8.1.x/src/Core/Module/ModuleManager.php you will see that it is not there, so I was not able to do the test you suggested.

Thank you.

@Hlavtox
Copy link
Contributor

Hlavtox commented Jan 17, 2024

Yeah it's buggy when deleting it from admin also. Always some cache stub errors, lock files not existing etc.

@Codencode
Copy link
Contributor Author

A different solution from the one I proposed is the one proposed by @ChillCode here #35044 (comment) which consists in modifying the AppKernel::getContainerClearCacheLockPath() method like this:

original method

    protected function getContainerClearCacheLockPath(): string
    {
        $class = $this->getContainerClass();
        $cacheDir = $this->getCacheDir();

        return sprintf('%s/%s.php.cache_clear.lock', $cacheDir, $class);
    }

updated method

    protected function getContainerClearCacheLockPath(): string
    {
        $class = $this->getContainerClass();;
        $cacheDir = sys_get_temp_dir();

        return sprintf('%s/%s.php.cache_clear.lock', $cacheDir, $class);
    }

I have tested it and it works both in case of installation and in case of deletion of the cache by the admin.

@Hlavtox @matks what do you think?

@Codencode
Copy link
Contributor Author

I removed the previous change as it was incorrect and made the change proposed by @ChillCode.

@Codencode
Copy link
Contributor Author

A different solution from the one I proposed is the one proposed by @ChillCode here #35044 (comment) which consists in modifying the AppKernel::getContainerClearCacheLockPath() method like this:

original method

    protected function getContainerClearCacheLockPath(): string
    {
        $class = $this->getContainerClass();
        $cacheDir = $this->getCacheDir();

        return sprintf('%s/%s.php.cache_clear.lock', $cacheDir, $class);
    }

updated method

    protected function getContainerClearCacheLockPath(): string
    {
        $class = $this->getContainerClass();;
        $cacheDir = sys_get_temp_dir();

        return sprintf('%s/%s.php.cache_clear.lock', $cacheDir, $class);
    }

I have tested it and it works both in case of installation and in case of deletion of the cache by the admin.

@Hlavtox @matks what do you think?

@Hlavtox @matks have you had a chance to check this out?

@SharakPL
Copy link
Contributor

In PR #29695 was introduced a little setting ModuleManager::$systemClearCache that, when turned off, prevents the cache from being cleared.

Here is my suggestion to try:

  1. Set ModuleManager::$systemClearCache to false -> this will avoid calling the cache clearing step when modules are installed
  2. After all modules are installed, perform a clean cache clear then warmup

@matks trying this on PS 8.1.5, but can't find systemClearCache anywhere in the files

Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

You can't do this, the cache dir of Symfony is not the tmp directory, it depends on the environment and the debug mode This modification ould certainly have huge negative impacts in all environments (including windows)

@ps-jarvis ps-jarvis added the Waiting for author Status: action required, waiting for author feedback label Apr 11, 2024
@Codencode
Copy link
Contributor Author

You can't do this, the cache dir of Symfony is not the tmp directory, it depends on the environment and the debug mode This modification ould certainly have huge negative impacts in all environments (including windows)

Ok, so during the installation phase we could prevent the execution of the cache:warmup command which is what generates the problem.

We can edit the src/Adapter/Cache/Clearer/SymfonyCacheClearer.php file like this:

if (!defined('PS_INSTALLATION_IN_PROGRESS')) { // The warmup should not be run during installation
    // Warmup prod environment only (not needed for dev since many things are dynamic)
    $application = new Application($kernel);
    $application->setAutoExit(false);
    $input = new ArrayInput([
        'command' => 'cache:warmup',
        '--no-optional-warmers' => true,
        '--env' => 'prod',
        '--no-debug' => true,
    ]);

    $output = new NullOutput();
    $application->doRun($input, $output);
}

@prestashop-issue-bot prestashop-issue-bot bot removed the Waiting for author Status: action required, waiting for author feedback label Apr 11, 2024
@ChillCode
Copy link

ChillCode commented Apr 11, 2024

Now installation fails because it tries to copy the only locked file (appAppKernelProdContainer.php.cache_clear.lock) that is used precisely to block the current process, so not performing any action just to this locked file is the other workaround to handle this for every system.

Don't know if using Symfony locking is plausible, at the end the issue will only occur when locked files are to be copied in this case.

Also when you have issues with WIN, get rid of the issue....but is an unpopular opinion, mine is that WSL or a cheap remote VM is a next step for any dev/maintainer and your computer will be safe.

@ChillCode
Copy link

ChillCode commented Apr 11, 2024

Hi @jolelievre

You can't do this, the cache dir of Symfony is not the tmp directory, it depends on the environment and the debug mode This modification ould certainly have huge negative impacts in all environments (including windows)

That change doesn't affect Symfony cachedir I think. only where appAppKernelProdContainer.php.cache_clear.lock will be stored during the current process.

My workaround only changes were that file resides even works when I apply on a clean PS 8.1.5 installation on a Debian VM, because my /tmp works and that file is not accessed during the clear process and deleted at the end.

But as said before, some systems fake sys_get_temp_dir() or /tmp is just not set and can cause other issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.1.x Branch Bug fix Type: Bug fix
Projects
Status: Waiting for author
Development

Successfully merging this pull request may close these issues.

None yet

8 participants