diff --git a/CHANGELOG.md b/CHANGELOG.md index 5af5c6c..4812962 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,12 @@ release is dated by its Drupal.org release node. was the wrapper's absolute URL encoded into one path segment, a row that could never match. A file on a private field that moved after a title change with active updating on left one behind. +- [#3494240](https://www.drupal.org/i/3494240): Stopped creating a redirect + on an entity's first save, and when a file leaves the staging location on a + later save. Nothing could link to either path, and with "Create Redirect" + on every new upload left a redirect behind. A bare scheme root such as + `public://` is not treated as a staging location, because it would match + every file on the scheme. ## 8.x-1.0-rc2 - 2026-09-07 diff --git a/src/Hook/FileFieldPathsProcessFileLegacy.php b/src/Hook/FileFieldPathsProcessFileLegacy.php index cc02333..2fe0d84 100644 --- a/src/Hook/FileFieldPathsProcessFileLegacy.php +++ b/src/Hook/FileFieldPathsProcessFileLegacy.php @@ -4,6 +4,7 @@ namespace Drupal\filefield_paths\Hook; +use Drupal\Core\Config\ConfigFactoryInterface; use Drupal\Component\Utility\DeprecationHelper; use Drupal\Core\Entity\ContentEntityInterface; use Drupal\Core\Extension\ModuleHandlerInterface; @@ -30,6 +31,7 @@ final readonly class FileFieldPathsProcessFileLegacy { public function __construct( + private ConfigFactoryInterface $configFactory, private FileSystemInterface $fileSystem, private FileRepositoryInterface $fileRepository, private StreamWrapperManagerInterface $streamWrapperManager, @@ -165,10 +167,14 @@ public function fileFieldPathsProcessFile(ContentEntityInterface $entity, FileFi } $this->processOutcome->recordUpdated($file->id()); - // Create redirect from old location. + // Create redirect from old location, unless the entity is new or the + // file is only now leaving the staging area. Nothing can link to a + // path that existed only between the upload and the save. if ( !empty($settings['redirect']) && $settings['active_updating'] && - $this->moduleHandler->moduleExists('redirect') + $original_entity instanceof ContentEntityInterface && + $this->moduleHandler->moduleExists('redirect') && + !$this->isStagedUpload($file->getFileUri(), $settings) ) { $redirect = $this->getRedirect(); $redirect->createRedirect($file->getFileUri(), $new_file->getFileUri(), $file->language()); @@ -238,4 +244,34 @@ private function sanitizeDestination(string $destination, string $source_uri, st return implode('/', $segments) . '/' . $event->getFilename(); } + /** + * Checks whether a file is still at the upload staging location. + * + * @param string $uri + * The file URI before the move. + * @param array $settings + * The File (Field) Paths settings for the field. + * + * @return bool + * TRUE if the file has not left the staging location yet. + */ + private function isStagedUpload(string $uri, array $settings): bool { + $temp_location = $settings['temp_location'] ?? NULL; + if (empty($temp_location)) { + $temp_location = $this->configFactory + ->get('filefield_paths.settings') + ->get('temp_location'); + } + if (!is_string($temp_location) || $temp_location === '') { + return FALSE; + } + // A bare scheme root such as "public://" is not a staging directory, and + // the settings form accepts one. Used as a prefix it would match every + // file on that scheme and stop redirects being created at all. + if ((string) $this->streamWrapperManager::getTarget($temp_location) === '') { + return FALSE; + } + return str_starts_with($uri, rtrim($temp_location, '/') . '/'); + } + } diff --git a/tests/src/Kernel/FileFieldPathsProcessFileLegacyTest.php b/tests/src/Kernel/FileFieldPathsProcessFileLegacyTest.php index 99e5de3..3552cd8 100644 --- a/tests/src/Kernel/FileFieldPathsProcessFileLegacyTest.php +++ b/tests/src/Kernel/FileFieldPathsProcessFileLegacyTest.php @@ -158,6 +158,7 @@ protected function getService(): FileFieldPathsProcessFileLegacy { */ protected function constructService(?FileSystemInterface $fileSystem = NULL, ?FileRepositoryInterface $fileRepository = NULL): FileFieldPathsProcessFileLegacy { return new FileFieldPathsProcessFileLegacy( + $this->container->get('config.factory'), $fileSystem ?? $this->container->get('file_system'), $fileRepository ?? $this->container->get('file.repository'), $this->container->get('stream_wrapper_manager'), diff --git a/tests/src/Kernel/StagingRedirectTest.php b/tests/src/Kernel/StagingRedirectTest.php new file mode 100644 index 0000000..427da1a --- /dev/null +++ b/tests/src/Kernel/StagingRedirectTest.php @@ -0,0 +1,219 @@ + + */ + protected static $modules = [ + 'system', + 'user', + 'field', + 'file', + 'path_alias', + 'redirect', + 'link', + 'entity_test', + 'filefield_paths', + ]; + + /** + * {@inheritdoc} + */ + protected function setUp(): void { + parent::setUp(); + $this->installEntitySchema('user'); + $this->installEntitySchema('file'); + $this->installEntitySchema('entity_test'); + $this->installEntitySchema('redirect'); + $this->installEntitySchema('path_alias'); + $this->installSchema('file', ['file_usage']); + $this->installConfig(['filefield_paths']); + // Pin the staging location. The install default moved to + // temporary://filefield_paths in 8.x-1.0-rc2, and the files these tests + // attach must sit inside the staging location whatever the default is. + $this->config('filefield_paths.settings')->set('temp_location', 'public://filefield_paths')->save(); + $this->config('redirect.settings')->set('default_status_code', 301)->save(); + + FieldStorageConfig::create([ + 'field_name' => 'field_file', + 'entity_type' => 'entity_test', + 'type' => 'file', + 'cardinality' => FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED, + 'settings' => ['uri_scheme' => 'public'], + ])->save(); + + $options = ['slashes' => FALSE, 'pathauto' => FALSE, 'transliterate' => FALSE]; + $field = FieldConfig::create([ + 'entity_type' => 'entity_test', + 'field_name' => 'field_file', + 'bundle' => 'entity_test', + ]); + $field->setThirdPartySetting('filefield_paths', 'enabled', TRUE); + $field->setThirdPartySetting('filefield_paths', 'file_path', [ + 'value' => 'sorted', + 'options' => $options, + ]); + $field->setThirdPartySetting('filefield_paths', 'file_name', [ + 'value' => '', + 'options' => $options, + ]); + $field->setThirdPartySetting('filefield_paths', 'active_updating', TRUE); + $field->setThirdPartySetting('filefield_paths', 'redirect', TRUE); + $field->setThirdPartySetting('filefield_paths', 'retroactive_update', FALSE); + $field->save(); + } + + /** + * Saves a new entity with a file at the given URI. + * + * @param string $uri + * Where the file sits before the entity is saved. + */ + private function createEntityWithFileAt(string $uri): void { + EntityTest::create([ + 'name' => 'test', + 'field_file' => [['target_id' => $this->createFileAt($uri)->id()]], + ])->save(); + } + + /** + * Saves an entity without a file, then saves it again with one. + * + * @param string $uri + * Where the file sits before the second save. + */ + private function updateEntityWithFileAt(string $uri): void { + $entity = EntityTest::create(['name' => 'test']); + $entity->save(); + $entity = EntityTest::load($entity->id()); + $entity->set('field_file', [['target_id' => $this->createFileAt($uri)->id()]]); + $entity->save(); + } + + /** + * Writes a file to disk and saves a permanent file entity for it. + */ + private function createFileAt(string $uri): File { + $file_system = $this->container->get('file_system'); + $directory = $file_system->dirname($uri); + $file_system->prepareDirectory($directory, $file_system::CREATE_DIRECTORY); + file_put_contents($uri, 'contents'); + $file = File::create(['uri' => $uri]); + $file->setPermanent(); + $file->save(); + return $file; + } + + /** + * Counts the redirects that exist. + */ + private function redirectCount(): int { + return count($this->container->get('entity_type.manager') + ->getStorage('redirect') + ->loadMultiple()); + } + + /** + * A new entity earns no redirect, wherever its file came from. + * + * Nothing can link to a file's path before the entity's first save. + * + * @see https://www.drupal.org/i/3494240 + */ + public function testNoRedirectWhenTheEntityIsNew(): void { + $this->createEntityWithFileAt('public://published/example.txt'); + + $this->assertFileExists('public://sorted/example.txt'); + $this->assertSame(0, $this->redirectCount(), 'A new entity should not leave a redirect behind.'); + } + + /** + * A file leaving the staging area earns no redirect. + * + * The staging path only ever existed between the upload and the save, so + * nothing can be linking to it, even when the entity already existed. + * + * @see https://www.drupal.org/i/3494240 + */ + public function testNoRedirectWhenTheFileComesFromStaging(): void { + $this->updateEntityWithFileAt('public://filefield_paths/example.txt'); + + $this->assertFileExists('public://sorted/example.txt'); + $this->assertSame(0, $this->redirectCount(), 'A staged upload should not leave a redirect behind.'); + } + + /** + * A file that was already published still earns a redirect. + * + * @see https://www.drupal.org/i/3494240 + */ + public function testRedirectWhenThePublishedFileMovesOnUpdate(): void { + $this->updateEntityWithFileAt('public://published/example.txt'); + + $this->assertFileExists('public://sorted/example.txt'); + $this->assertSame(1, $this->redirectCount(), 'A published file that moves should leave a redirect.'); + } + + /** + * A bare scheme root is not a staging location. + * + * The settings form accepts "public://" with no directory. Used as a + * staging prefix it would match every file on the scheme and quietly stop + * redirects being created at all. + * + * @see https://www.drupal.org/i/3494240 + */ + public function testBareSchemeRootIsNotTreatedAsStaging(): void { + $this->config('filefield_paths.settings') + ->set('temp_location', 'public://') + ->save(); + + $this->updateEntityWithFileAt('public://published/example.txt'); + + $this->assertFileExists('public://sorted/example.txt'); + $this->assertSame(1, $this->redirectCount(), 'A bare scheme root must not suppress redirects.'); + } + + /** + * The field's own staging location wins over the global one. + * + * @see https://www.drupal.org/i/3494240 + */ + public function testFieldStagingLocationTakesPrecedence(): void { + $field = FieldConfig::loadByName('entity_test', 'entity_test', 'field_file'); + \assert($field instanceof FieldConfig); + $field->setThirdPartySetting('filefield_paths', 'temp_location', 'public://custom_stage'); + $field->save(); + $this->container->get('entity_field.manager')->clearCachedFieldDefinitions(); + + $this->updateEntityWithFileAt('public://custom_stage/example.txt'); + + $this->assertFileExists('public://sorted/example.txt'); + $this->assertSame(0, $this->redirectCount(), 'The field level staging location should suppress the redirect.'); + } + +}