deployphp / deployer

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

Breaking change in v7.3.2 & v7.3.3 for `cachetool` task #3741

Closed claytonrcarter closed 6 months ago

claytonrcarter commented 6 months ago

Hi, we recently updated from 7.3.1 to 7.3.3 and found that our very "default" cachetool usage stopped working. (The only cachetool config we have set is set('bin/cachetool', 'vendor/bin/cachetool') so use the composer installed version.) The issue was introduced by #3684 – where is caused a loud failure – and then tweaked by cb28eb82ff7549c888127ae29cb49024d2f2ca00 – where it now causes a silent failure.

The issue is that the cachetool setting is '' (empty string) by default (see https://github.com/deployphp/deployer/blob/master/contrib/cachetool.php#L50) but the new changes in cachetool_options (see diff of 7.3.1...master at https://github.com/deployphp/deployer/compare/v7.3.1...master#diff-3ff1b6689a573050ee941e9cf0474c9f96a4ff7697a386d9c1af6b320fc10ce6) mean that this default value leads to no invocations of cachetool:

  1. cachetool starts as ''
  2. it is then cast to an array, turning into ['']
  3. the empty element is skipped (by the foreach ... if)
  4. an empty array is returned from cachetool_options
  5. task cachetool:clear:opcache iterates over this array, doing nothing because it's empty

Based on the code, it seems that this will affect any of the cachetool:* tasks, as they all iterate over cachetool_options, but I have only experienced it with cachetool:clear:opcache

Workaround I have found that set('cachetool_args', ' '); is an effective workaround for the time being. Note that it needs the second arg to be a non-empty string of only whitespace.

Possible Fixes I'm sorry, but I don't have the time or wherewithal to test and submit a fix for this at this time. The options that jump out at me, though, are:

  1. don't cast (array)get('cachetool');, and do if ($options === '') { return [''] }, then cast (array) $options when it's given to the foreach
  2. leave the cast in place, but change the foreach to something like $return[] = empty($option) ? '' : "--fcgi={$option}"

Thank you!

Upvote & Fund

Fund with Polar

antonmedv commented 6 months ago

@infabo please, take a look.

infabo commented 6 months ago

I gonna submit a PR patch later today. I already was afraid of a side effect like this one.

infabo commented 6 months ago

@claytonrcarter Please have a look at https://github.com/deployphp/deployer/pull/3742 if this PR fixes the issue.

claytonrcarter commented 6 months ago

Thank you both for the fix!

@antonmedv Is there any chance we'll be getting a new point release soon to fix this publicly?

antonmedv commented 6 months ago

Yes, will release Deployer today.