From acf190c57ca23f7dd350ebbbc413e4be18819aa9 Mon Sep 17 00:00:00 2001 From: Nasir Uddin Nobin Date: Tue, 15 Feb 2022 02:05:48 +0600 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Matt Stauffer --- cli/Valet/PhpFpm.php | 47 ++++++++++++++++++++++---------------------- cli/Valet/Site.php | 22 ++++++++++----------- 2 files changed, 35 insertions(+), 34 deletions(-) diff --git a/cli/Valet/PhpFpm.php b/cli/Valet/PhpFpm.php index b28ae6c..9f8390a 100644 --- a/cli/Valet/PhpFpm.php +++ b/cli/Valet/PhpFpm.php @@ -33,7 +33,7 @@ public function __construct(Brew $brew, CommandLine $cli, Filesystem $files) /** * Install and configure PhpFpm. * - * @param null $phpVersion + * @param string|null $phpVersion * @return void */ public function install($phpVersion = null) @@ -64,7 +64,7 @@ public function uninstall() /** * Update the PHP FPM configuration. * - * @param null $phpVersion + * @param string|null $phpVersion * @return void */ public function updateConfiguration($phpVersion = null) @@ -181,7 +181,7 @@ public function stopRunning() * * @param $version * @param bool $force - * @param null $site + * @param string|null $site * @return string */ public function useVersion($version, $force = false, $site = null) @@ -202,21 +202,20 @@ public function useVersion($version, $force = false, $site = null) $this->brew->ensureInstalled($version, [], $this->taps); } - // we need to unlink and link only for global php version change + // Delete old Valet sock files, install the new version, and, if this is a global change, unlink and link PHP if ($site) { $this->cli->quietly('sudo rm '.VALET_HOME_PATH.'/'.$this->fpmSockName($version)); $this->install($version); } else { - // Unlink the current php if there is one + // Unlink the current global PHP if there is one installed if ($this->brew->hasLinkedPhp()) { $linkedPhp = $this->brew->linkedPhp(); - // updating old fpm to use a custom sock - // so exising lokced php versioned sites doesn't mess up + // Update the old FPM to keep running, using a custom sock file, so existing isolated sites aren't broken $this->updateConfiguration($linkedPhp); - // check all custom nginx config files - // look for the php version and update config files accordingly + // Update existing custom Nginx config files; if they're using the old or new PHP version, + // update them to the new correct sock file location $this->updateConfigurationForGlobalUpdate($version, $linkedPhp); $currentVersion = $this->brew->getLinkedPhpFormula(); @@ -285,7 +284,7 @@ public function validateRequestedVersion($version) } /** - * Get fpm sock file name from a given php version. + * Get FPM sock file name for a given PHP version. * * @param string|null $phpVersion * @return string @@ -298,16 +297,21 @@ public function fpmSockName($phpVersion = null) } /** - * Preseve exising isolated PHP versioned sites, when running a gloabl php version update. Look for the php version and will adjust that custom nginx config. + * Update all existing Nginx files when running a global PHP version update. + * If a given file is pointing to `valet.sock`, it's targeting the old global PHP version; + * update it to point to the new custom sock file for that version. + * If a given file is pointing the custom sock file for the new global version, that new + * version will now be hosted at `valet.sock`, so update the config file to point to that instead. * * @param $newPhpVersion * @param $oldPhpVersion + * @return void */ public function updateConfigurationForGlobalUpdate($newPhpVersion, $oldPhpVersion) { collect($this->files->scandir(VALET_HOME_PATH.'/Nginx')) - ->filter(function ($file) { - return ! starts_with($file, '.'); + ->reject(function ($file) { + return starts_with($file, '.'); }) ->each(function ($file) use ($newPhpVersion, $oldPhpVersion) { $content = $this->files->get(VALET_HOME_PATH.'/Nginx/'.$file); @@ -327,32 +331,29 @@ public function updateConfigurationForGlobalUpdate($newPhpVersion, $oldPhpVersio } /** - * Get the PHP versions to perform restart. + * Get a list of all PHP versions currently serving "isolated sites" (sites with custom Nginx + * configs pointing them to a specific PHP version). * * @return array */ public function getPhpVersionsToPerformRestart() { - // scan through custom nginx files - // look for config file, that is using custom .sock files (example: php74.sock) - // restart all those PHP FPM though valet - // to make sure all the custom php versioned sites keep running - $fpmSockFiles = $this->brew->supportedPhpVersions()->map(function ($version) { return $this->fpmSockName($this->normalizePhpVersion($version)); })->unique(); return collect($this->files->scandir(VALET_HOME_PATH.'/Nginx')) - ->filter(function ($file) { - return ! starts_with($file, '.'); + ->reject(function ($file) { + return starts_with($file, '.'); }) ->map(function ($file) use ($fpmSockFiles) { $content = $this->files->get(VALET_HOME_PATH.'/Nginx/'.$file); + // Get the normalized PHP version for this config file, if it's defined foreach ($fpmSockFiles as $sock) { if (strpos($content, $sock) !== false) { - // find the PHP version number from .sock path - // valet74.sock will outout php74 + // Extract the PHP version number from a custom .sock path; + // for example, "valet74.sock" will output "php74" $phpVersion = 'php'.str_replace(['valet', '.sock'], '', $sock); return $this->normalizePhpVersion($phpVersion); // example output php@7.4 diff --git a/cli/Valet/Site.php b/cli/Valet/Site.php index c9bd734..1ee192c 100644 --- a/cli/Valet/Site.php +++ b/cli/Valet/Site.php @@ -191,14 +191,14 @@ public function proxies() } /** - * Determine if the provided site is parked. + * Determine if the provided site is a valid site, whether parked or linked. * - * @param $valetSite + * @param string $valetSite * @return bool */ public function isValidSite($valetSite) { - // remove .tld to make the search a bit easier + // Remove .tld from sitename if it was provided $siteName = str_replace('.'.$this->config->read()['tld'], '', $valetSite); return $this->parked()->merge($this->links())->where('site', $siteName)->count() > 0; @@ -488,7 +488,8 @@ public function secured() */ public function secure($url, $siteConf = null, $certificateExpireInDays = 396, $caExpireInYears = 20) { - $phpVersion = $this->extractPhpVersion($url); // let's try to preserve the isolated php version here. Example output: 74 + // Extract in order to later preserve custom PHP version config when securing + $phpVersion = $this->extractPhpVersion($url); $this->unsecure($url); @@ -505,8 +506,7 @@ public function secure($url, $siteConf = null, $certificateExpireInDays = 396, $ $siteConf = $this->buildSecureNginxServer($url, $siteConf); - // if user had any isolated php version, let's swap the .sock file, - // so it still uses the old php version + // If they user had isolated the PHP version for this site, swap out .sock file if ($phpVersion) { $siteConf = $this->replaceSockFile($siteConf, "valet{$phpVersion}.sock", $phpVersion); } @@ -1020,7 +1020,7 @@ public function certificatesPath($url = null, $extension = null) } /** - * Replace Loopback. + * Replace Loopback configuration line in Valet site configuration file contents. * * @param string $siteConf * @return string @@ -1062,11 +1062,11 @@ public function extractPhpVersion($url) } /** - * Replace .sock file form a Nginx site conf. + * Replace .sock file in an Nginx site configuration file contents. * - * @param $siteConf - * @param $sockFile - * @param $phpVersion + * @param string $siteConf + * @param string $sockFile + * @param string $phpVersion * @return string */ public function replaceSockFile($siteConf, $sockFile, $phpVersion)