From a5952429359dab9dbfbc4ee8250f7a37574f74e7 Mon Sep 17 00:00:00 2001 From: lucashedding <lucashedding@1463982.no-reply.drupal.org> Date: Mon, 11 Nov 2019 16:42:05 -0600 Subject: [PATCH] Issue #3093700 by heddn, ressa, rfay, rkoller: Improve requirements failures for composer workflow --- automatic_updates.install | 12 +++++ automatic_updates.services.yml | 7 ++- src/ReadinessChecker/DiskSpace.php | 26 +--------- src/ReadinessChecker/FileOwnership.php | 15 +----- src/ReadinessChecker/Filesystem.php | 15 +++++- src/ReadinessChecker/ReadOnlyFilesystem.php | 12 +++-- src/ReadinessChecker/Vendor.php | 20 +++++++ .../Kernel/ReadinessChecker/DiskSpaceTest.php | 6 +-- .../Kernel/ReadinessChecker/ReadOnlyTest.php | 24 +++++---- .../Kernel/ReadinessChecker/VendorTest.php | 52 +++++++++++++++++++ 10 files changed, 129 insertions(+), 60 deletions(-) create mode 100644 src/ReadinessChecker/Vendor.php create mode 100644 tests/src/Kernel/ReadinessChecker/VendorTest.php diff --git a/automatic_updates.install b/automatic_updates.install index 0b5505b357..cfc49c1a8c 100644 --- a/automatic_updates.install +++ b/automatic_updates.install @@ -6,6 +6,7 @@ */ use Drupal\automatic_updates\ReadinessChecker\ReadinessCheckerManagerInterface; +use Drupal\automatic_updates\ReadinessChecker\Vendor; use Drupal\Core\StringTranslation\PluralTranslatableMarkup; use Drupal\Core\Url; @@ -13,6 +14,17 @@ use Drupal\Core\Url; * Implements hook_requirements(). */ function automatic_updates_requirements($phase) { + $vendor_checker = new Vendor(\Drupal::getContainer()->get('app.root')); + if (!empty($vendor_checker->run())) { + return [ + 'not installable' => [ + 'title' => t('Automatic Updates'), + 'severity' => REQUIREMENT_ERROR, + 'value' => '1.x', + 'description' => t('This module does not currently support relocated vendor folder and composer-based workflows.'), + ], + ]; + } if ($phase !== 'runtime') { return; } diff --git a/automatic_updates.services.yml b/automatic_updates.services.yml index a9381067cc..2361032411 100644 --- a/automatic_updates.services.yml +++ b/automatic_updates.services.yml @@ -64,7 +64,6 @@ services: automatic_updates.disk_space_checker: class: Drupal\automatic_updates\ReadinessChecker\DiskSpace arguments: - - '@logger.channel.automatic_updates' - '@app.root' tags: - { name: readiness_checker, category: error} @@ -120,3 +119,9 @@ services: - '@module_handler' tags: - { name: readiness_checker, category: warning} + automatic_updates.vendor: + class: Drupal\automatic_updates\ReadinessChecker\Vendor + arguments: + - '@app.root' + tags: + - { name: readiness_checker, category: error} diff --git a/src/ReadinessChecker/DiskSpace.php b/src/ReadinessChecker/DiskSpace.php index ec08d03c42..7ce40c13f8 100644 --- a/src/ReadinessChecker/DiskSpace.php +++ b/src/ReadinessChecker/DiskSpace.php @@ -2,14 +2,10 @@ namespace Drupal\automatic_updates\ReadinessChecker; -use Drupal\Core\StringTranslation\StringTranslationTrait; -use Psr\Log\LoggerInterface; - /** * Disk space checker. */ class DiskSpace extends Filesystem { - use StringTranslationTrait; /** * Minimum disk space (in bytes) is 10mb. @@ -21,26 +17,6 @@ class DiskSpace extends Filesystem { */ const MEGABYTE_DIVISOR = 1000000; - /** - * The logger. - * - * @var \Psr\Log\LoggerInterface - */ - protected $logger; - - /** - * ReadOnlyFilesystem constructor. - * - * @param \Psr\Log\LoggerInterface $logger - * The logger. - * @param string $app_root - * The app root. - */ - public function __construct(LoggerInterface $logger, $app_root) { - $this->logger = $logger; - $this->rootPath = (string) $app_root; - } - /** * {@inheritdoc} */ @@ -63,7 +39,7 @@ class DiskSpace extends Filesystem { '@space' => static::MINIMUM_DISK_SPACE / static::MEGABYTE_DIVISOR, ]); } - if (disk_free_space($this->getVendorPath()) < static::MINIMUM_DISK_SPACE) { + if (is_dir($this->getVendorPath()) && disk_free_space($this->getVendorPath()) < static::MINIMUM_DISK_SPACE) { $messages[] = $this->t('Vendor filesystem "@vendor" has insufficient space. There must be at least @space megabytes free.', [ '@vendor' => $this->getVendorPath(), '@space' => static::MINIMUM_DISK_SPACE / static::MEGABYTE_DIVISOR, diff --git a/src/ReadinessChecker/FileOwnership.php b/src/ReadinessChecker/FileOwnership.php index 6e6d02b992..d628f74109 100644 --- a/src/ReadinessChecker/FileOwnership.php +++ b/src/ReadinessChecker/FileOwnership.php @@ -2,29 +2,16 @@ namespace Drupal\automatic_updates\ReadinessChecker; -use Drupal\Core\StringTranslation\StringTranslationTrait; - /** * File ownership checker. */ class FileOwnership extends Filesystem { - use StringTranslationTrait; - - /** - * FileOwnership constructor. - * - * @param string $app_root - * The app root. - */ - public function __construct($app_root) { - $this->rootPath = (string) $app_root; - } /** * {@inheritdoc} */ protected function doCheck() { - $file_path = $this->getRootPath() . '/core/core.api.php'; + $file_path = $this->getRootPath() . DIRECTORY_SEPARATOR . implode(DIRECTORY_SEPARATOR, ['core', 'core.api.php']); return $this->ownerIsScriptUser($file_path); } diff --git a/src/ReadinessChecker/Filesystem.php b/src/ReadinessChecker/Filesystem.php index 8adb6c4505..2edc02f181 100644 --- a/src/ReadinessChecker/Filesystem.php +++ b/src/ReadinessChecker/Filesystem.php @@ -2,10 +2,13 @@ namespace Drupal\automatic_updates\ReadinessChecker; +use Drupal\Core\StringTranslation\StringTranslationTrait; + /** * Base class for filesystem checkers. */ abstract class Filesystem implements ReadinessCheckerInterface { + use StringTranslationTrait; /** * The root file path. @@ -21,11 +24,21 @@ abstract class Filesystem implements ReadinessCheckerInterface { */ protected $vendorPath; + /** + * Filesystem constructor. + * + * @param string $app_root + * The app root. + */ + public function __construct($app_root) { + $this->rootPath = (string) $app_root; + } + /** * {@inheritdoc} */ public function run() { - if (!$this->exists($this->getRootPath() . '/core/core.api.php')) { + if (!$this->exists($this->getRootPath() . DIRECTORY_SEPARATOR . implode(DIRECTORY_SEPARATOR, ['core', 'core.api.php']))) { return [$this->t('The web root could not be located.')]; } diff --git a/src/ReadinessChecker/ReadOnlyFilesystem.php b/src/ReadinessChecker/ReadOnlyFilesystem.php index c05233d421..c82cc8d1e3 100644 --- a/src/ReadinessChecker/ReadOnlyFilesystem.php +++ b/src/ReadinessChecker/ReadOnlyFilesystem.php @@ -5,14 +5,12 @@ namespace Drupal\automatic_updates\ReadinessChecker; use Drupal\Component\Render\MarkupInterface; use Drupal\Core\File\Exception\FileException; use Drupal\Core\File\FileSystemInterface; -use Drupal\Core\StringTranslation\StringTranslationTrait; use Psr\Log\LoggerInterface; /** * Read only filesystem checker. */ class ReadOnlyFilesystem extends Filesystem { - use StringTranslationTrait; /** * The logger. @@ -65,7 +63,7 @@ class ReadOnlyFilesystem extends Filesystem { } else { $error = $this->t('Drupal core filesystem at "@path" is read only. Updates to Drupal core cannot be applied against a read only file system.', ['@path' => $this->rootPath . '/core']); - $this->doReadOnlyCheck($this->getRootPath(), 'core.api.php', $messages, $error); + $this->doReadOnlyCheck($this->getRootPath(), implode(DIRECTORY_SEPARATOR, ['core', 'core.api.php']), $messages, $error); $error = $this->t('Vendor filesystem at "@path" is read only. Updates to the site\'s code base cannot be applied against a read only file system.', ['@path' => $this->vendorPath]); $this->doReadOnlyCheck($this->getVendorPath(), 'composer/LICENSE', $messages, $error); } @@ -85,12 +83,16 @@ class ReadOnlyFilesystem extends Filesystem { * The error message to add if the file is read only. */ protected function doReadOnlyCheck($file_path, $file, array &$messages, MarkupInterface $message) { + // Ignore check if the path doesn't exit. + if (!is_file($file_path . DIRECTORY_SEPARATOR . $file)) { + return; + } try { // If we can copy and delete a file, then we don't have a read only // file system. - if ($this->fileSystem->copy("$file_path/$file", "$file_path/$file.automatic_updates", FileSystemInterface::EXISTS_REPLACE)) { + if ($this->fileSystem->copy($file_path . DIRECTORY_SEPARATOR . $file, $file_path . DIRECTORY_SEPARATOR . "$file.automatic_updates", FileSystemInterface::EXISTS_REPLACE)) { // Delete it after copying. - $this->fileSystem->delete("$file_path/$file.automatic_updates"); + $this->fileSystem->delete($file_path . DIRECTORY_SEPARATOR . "$file.automatic_updates"); } else { $this->logger->error($message); diff --git a/src/ReadinessChecker/Vendor.php b/src/ReadinessChecker/Vendor.php new file mode 100644 index 0000000000..08aaa736e7 --- /dev/null +++ b/src/ReadinessChecker/Vendor.php @@ -0,0 +1,20 @@ +<?php + +namespace Drupal\automatic_updates\ReadinessChecker; + +/** + * Error if site is managed via composer instead of via tarballs. + */ +class Vendor extends Filesystem { + + /** + * {@inheritdoc} + */ + protected function doCheck() { + if (!$this->exists($this->getVendorPath() . DIRECTORY_SEPARATOR . 'autoload.php')) { + return [$this->t('The vendor folder could not be located.')]; + } + return []; + } + +} diff --git a/tests/src/Kernel/ReadinessChecker/DiskSpaceTest.php b/tests/src/Kernel/ReadinessChecker/DiskSpaceTest.php index 8c4580e880..4e573f55d9 100644 --- a/tests/src/Kernel/ReadinessChecker/DiskSpaceTest.php +++ b/tests/src/Kernel/ReadinessChecker/DiskSpaceTest.php @@ -26,17 +26,17 @@ class DiskSpaceTest extends KernelTestBase { */ public function testDiskSpace() { // No disk space issues. - $disk_space = new DiskSpace($this->container->get('logger.channel.automatic_updates'), $this->container->get('app.root')); + $disk_space = new DiskSpace($this->container->get('app.root')); $messages = $disk_space->run(); $this->assertEmpty($messages); // Out of space. - $disk_space = new TestDiskSpace($this->container->get('logger.channel.automatic_updates'), $this->container->get('app.root')); + $disk_space = new TestDiskSpace($this->container->get('app.root')); $messages = $disk_space->run(); $this->assertCount(1, $messages); // Out of space not the same logical disk. - $disk_space = new TestDiskSpaceNonSameDisk($this->container->get('logger.channel.automatic_updates'), $this->container->get('app.root')); + $disk_space = new TestDiskSpaceNonSameDisk($this->container->get('app.root')); $messages = $disk_space->run(); $this->assertCount(2, $messages); } diff --git a/tests/src/Kernel/ReadinessChecker/ReadOnlyTest.php b/tests/src/Kernel/ReadinessChecker/ReadOnlyTest.php index 4d3507ac2b..1c62a96c61 100644 --- a/tests/src/Kernel/ReadinessChecker/ReadOnlyTest.php +++ b/tests/src/Kernel/ReadinessChecker/ReadOnlyTest.php @@ -34,7 +34,7 @@ class ReadOnlyTest extends KernelTestBase { $this->assertEmpty($messages); $filesystem = $this->createMock(FileSystemInterface::class); - $filesystem->expects($this->any()) + $filesystem ->method('copy') ->withAnyParameters() ->will($this->onConsecutiveCalls( @@ -45,7 +45,7 @@ class ReadOnlyTest extends KernelTestBase { 'full/file/path' ) ); - $filesystem->expects($this->any()) + $filesystem ->method('delete') ->withAnyParameters() ->will($this->onConsecutiveCalls( @@ -54,10 +54,11 @@ class ReadOnlyTest extends KernelTestBase { ) ); + $app_root = $this->container->get('app.root'); $readonly = $this->getMockBuilder(ReadOnlyFilesystem::class) ->setConstructorArgs([ $this->createMock(LoggerInterface::class), - '/var/www/html', + $app_root, $filesystem, ]) ->setMethods([ @@ -65,7 +66,7 @@ class ReadOnlyTest extends KernelTestBase { 'exists', ]) ->getMock(); - $readonly->expects($this->any()) + $readonly ->method('areSameLogicalDisk') ->withAnyParameters() ->will($this->onConsecutiveCalls( @@ -74,7 +75,7 @@ class ReadOnlyTest extends KernelTestBase { FALSE ) ); - $readonly->expects($this->any()) + $readonly ->method('exists') ->withAnyParameters() ->will($this->onConsecutiveCalls( @@ -91,21 +92,22 @@ class ReadOnlyTest extends KernelTestBase { // Test same logical disk. $expected_messages = []; - $expected_messages[] = $this->t('Logical disk at "/var/www/html" is read only. Updates to Drupal cannot be applied against a read only file system.'); + $expected_messages[] = $this->t('Logical disk at "@app_root" is read only. Updates to Drupal cannot be applied against a read only file system.', ['@app_root' => $app_root]); $messages = $readonly->run(); $this->assertEquals($expected_messages, $messages); // Test read-only. $expected_messages = []; - $expected_messages[] = $this->t('Drupal core filesystem at "/var/www/html/core" is read only. Updates to Drupal core cannot be applied against a read only file system.'); - $expected_messages[] = $this->t('Vendor filesystem at "/var/www/html/vendor" is read only. Updates to the site\'s code base cannot be applied against a read only file system.'); + $expected_messages[] = $this->t('Drupal core filesystem at "@core" is read only. Updates to Drupal core cannot be applied against a read only file system.', [ + '@core' => $app_root . DIRECTORY_SEPARATOR . 'core', + ]); + $expected_messages[] = $this->t('Vendor filesystem at "@vendor" is read only. Updates to the site\'s code base cannot be applied against a read only file system.', [ + '@vendor' => $app_root . DIRECTORY_SEPARATOR . 'vendor', + ]); $messages = $readonly->run(); $this->assertEquals($expected_messages, $messages); // Test delete fails. - $expected_messages = []; - $expected_messages[] = $this->t('Drupal core filesystem at "/var/www/html/core" is read only. Updates to Drupal core cannot be applied against a read only file system.'); - $expected_messages[] = $this->t('Vendor filesystem at "/var/www/html/vendor" is read only. Updates to the site\'s code base cannot be applied against a read only file system.'); $messages = $readonly->run(); $this->assertEquals($expected_messages, $messages); } diff --git a/tests/src/Kernel/ReadinessChecker/VendorTest.php b/tests/src/Kernel/ReadinessChecker/VendorTest.php new file mode 100644 index 0000000000..d93dd6fcd5 --- /dev/null +++ b/tests/src/Kernel/ReadinessChecker/VendorTest.php @@ -0,0 +1,52 @@ +<?php + +namespace Drupal\Tests\automatic_updates\Kernel\ReadinessChecker; + +use Drupal\automatic_updates\ReadinessChecker\Vendor; +use Drupal\Core\StringTranslation\StringTranslationTrait; +use Drupal\KernelTests\KernelTestBase; + +/** + * Tests locating vendor folder. + * + * @group automatic_updates + */ +class VendorTest extends KernelTestBase { + use StringTranslationTrait; + + /** + * {@inheritdoc} + */ + public static $modules = [ + 'automatic_updates', + ]; + + /** + * Tests vendor folder existing. + */ + public function testVendor() { + $vendor = $this->container->get('automatic_updates.vendor'); + $this->assertEmpty($vendor->run()); + + $missing_vendor = $this->getMockBuilder(Vendor::class) + ->setConstructorArgs([ + $this->container->get('app.root'), + ]) + ->setMethods([ + 'exists', + ]) + ->getMock(); + $missing_vendor + ->method('exists') + ->withAnyParameters() + ->will($this->onConsecutiveCalls( + TRUE, + FALSE + ) + ); + $expected_messages = []; + $expected_messages[] = $this->t('The vendor folder could not be located.'); + $this->assertEquals($expected_messages, $missing_vendor->run()); + } + +} -- GitLab