deployphp / deployer

The PHP deployment tool with support for popular frameworks out of the box
https://deployer.org
MIT License
10.56k stars 1.48k forks source link

Magento 2: use_redis_cache_id flag has no effect #3713

Closed mfickers closed 11 months ago

mfickers commented 11 months ago

The use_redis_cache_id is ignored in custom deploy scripts extending the Magento 2 recipe.

Environment

Steps to reproduce:

  1. Extend Magento 2 recipe
  2. Set flag "use_redis_cache_id" to true
    
    <?php
    namespace Deployer;

require 'recipe/magento2.php';

// Config set('use_redis_cache_id', true);

3. Generate tree for `deploy`

./vendor/bin/dep tree -f deploy.php deploy


## Expected behavior:
Steps `magento:set_cache_prefix` and `magento:cleanup_cache_prefix` are included:

The task-tree for deploy: └── deploy ├── deploy:prepare │ ├── deploy:info │ ├── deploy:setup │ ├── deploy:lock │ ├── deploy:release │ ├── deploy:update_code │ ├── deploy:shared │ ├── magento:set_cache_prefix // after deploy:shared │ └── deploy:writable ├── deploy:vendors ├── deploy:clear_paths ├── deploy:magento │ ├── magento:build │ │ ├── magento:compile │ │ ├── magento:sync:content_version // before magento:deploy:assets │ │ └── magento:deploy:assets │ ├── magento:maintenance:enable-if-needed │ ├── magento:config:import │ ├── magento:upgrade:db │ ├── magento:maintenance:disable │ └── magento:cache:flush ├── magento:cleanup_cache_prefix // after deploy:magento └── deploy:publish ├── deploy:symlink ├── deploy:unlock ├── deploy:cleanup └── deploy:success


## Actual behavior:
Steps `magento:set_cache_prefix` and `magento:cleanup_cache_prefix` are missing:

The task-tree for deploy: └── deploy ├── deploy:prepare │ ├── deploy:info │ ├── deploy:setup │ ├── deploy:lock │ ├── deploy:release │ ├── deploy:update_code │ ├── deploy:shared │ └── deploy:writable ├── deploy:vendors ├── deploy:clear_paths ├── deploy:magento │ ├── magento:build │ │ ├── magento:compile │ │ ├── magento:sync:content_version // before magento:deploy:assets │ │ └── magento:deploy:assets │ ├── magento:maintenance:enable-if-needed │ ├── magento:config:import │ ├── magento:upgrade:db │ ├── magento:maintenance:disable │ └── magento:cache:flush └── deploy:publish ├── deploy:symlink ├── deploy:unlock ├── deploy:cleanup └── deploy:success

mfickers commented 11 months ago

I suspect that this was broken by this commit: https://github.com/deployphp/deployer/pull/3453/commits/bf1d11d16286f3fe1451d0629f0fcdda31608890

This change moved the check to around the hooks. The problem is, the flag will always be false when this condition is executed, as this happens during parsing of the recipe and not at runtime.

According to this comment https://github.com/deployphp/deployer/pull/3453#issuecomment-1490310266, the change was made to prevent the steps from showing up if they are not executed. If anyone has an idea on how to fix this without breaking this behavior, I'll gladly create a PR.

As a workaround I've re-declared the missing steps in my script:

<?php
namespace Deployer;

require 'recipe/magento2.php';

// Config
set('use_redis_cache_id', true);

// Workaround
if (get('use_redis_cache_id')) {
    after('deploy:shared', 'magento:set_cache_prefix');
}
if (get('use_redis_cache_id')) {
    after('deploy:magento', 'magento:cleanup_cache_prefix');
}
mfickers commented 11 months ago

Looks like this is already fixed in master, as part of this PR: https://github.com/deployphp/deployer/pull/3567. The flag has been removed and the user should instead include the steps in their script. I'm closing this issue.