diff --git a/src/Redirect.php b/src/Redirect.php index 7e8c396..72b4b50 100644 --- a/src/Redirect.php +++ b/src/Redirect.php @@ -52,9 +52,11 @@ public function createRedirect($source, $path, LanguageInterface $language): voi $redirect->setSource($parsed_source); $redirect->setRedirect($parsed_path); $redirect->setStatusCode($this->configFactory->get('redirect.settings')->get('default_status_code')); - - // Check if the redirect doesn't already exist before saving. - $hash = $redirect->generateHash($parsed_path, [], $language->getId()); + // Redirect::preSave() builds the stored hash from the source path and the + // entity's own language. Read the language back off the entity so the + // check below asks for the hash the save will actually produce, rather + // than one built from a language the entity does not carry. + $hash = $redirect->generateHash(ltrim($parsed_source, '/'), [], $redirect->get('language')->value); $redirects = $storage->loadByProperties(['hash' => $hash]); if (empty($redirects)) { // Redirect does not exist yet, save as new one. diff --git a/tests/src/Kernel/RedirectTest.php b/tests/src/Kernel/RedirectTest.php index a5582f0..f34a1ec 100644 --- a/tests/src/Kernel/RedirectTest.php +++ b/tests/src/Kernel/RedirectTest.php @@ -76,31 +76,17 @@ public function testCreateRedirectForPublicScheme(): void { $this->assertStringEndsWith('new/destination.txt', $redirect->getRedirectUrl()->toUriString()); } - /** - * Whether the redirect dedup hash bug has been fixed. - * - * Flip to TRUE to re-enable the test. - */ - private static bool $issueRedirectDedupFixed = FALSE; - /** * Tests that duplicate redirects are silently skipped, not thrown. * - * Bug (undocumented, found while writing this test): the pre-check hash in - * Redirect::createRedirect() is generated from the destination path - * ($parsed_path), but \Drupal\redirect\Entity\Redirect::preSave() always - * recomputes the stored hash from the *source* path. The two hashes are - * never the same value for a real redirect, so the existence check almost - * never matches, and calling createRedirect() twice with an identical - * source/destination throws an uncaught database unique-constraint - * exception instead of being silently skipped. This is not yet filed as a - * Drupal.org issue; recorded here as a baseline for future investigation. + * Until #3045063 was fixed, the pre-check hashed the destination path while + * Redirect::preSave() hashed the source, so the two values never matched + * and a second identical call crashed on the redirect table's unique hash + * index instead of being skipped. + * + * @see https://www.drupal.org/i/3045063 */ public function testCreateRedirectTwiceWithSameArgumentsDoesNotThrow(): void { - if (!self::$issueRedirectDedupFixed) { - $this->markTestSkipped('Reproduces redirect dedup hash bug: duplicate createRedirect() throws instead of being skipped. Flip $issueRedirectDedupFixed once fixed.'); - } - /** @var \Drupal\filefield_paths\RedirectInterface $redirect_service */ $redirect_service = $this->container->get('filefield_paths.redirect'); $language = new Language(['id' => 'en']); @@ -115,6 +101,28 @@ public function testCreateRedirectTwiceWithSameArgumentsDoesNotThrow(): void { $this->assertCount(1, $redirects, 'Duplicate redirect should be skipped, not stored twice.'); } + /** + * The same skip happens when the file's language is not the site default. + * + * The stored hash is built in Redirect::preSave() from the entity's own + * language. Until #3045063 was fixed the pre-check used the language passed + * in by the caller, which is the file's and often not the one the entity + * carries, so the two hashes disagreed and the duplicate was missed. + * + * @see https://www.drupal.org/i/3045063 + */ + public function testCreateRedirectTwiceWithNonDefaultLanguageDoesNotThrow(): void { + /** @var \Drupal\filefield_paths\RedirectInterface $redirect_service */ + $redirect_service = $this->container->get('filefield_paths.redirect'); + $language = new Language(['id' => 'de']); + + $redirect_service->createRedirect('public://old/source.txt', 'public://new/destination.txt', $language); + $redirect_service->createRedirect('public://old/source.txt', 'public://new/destination.txt', $language); + + $redirects = $this->container->get('entity_type.manager')->getStorage('redirect')->loadMultiple(); + $this->assertCount(1, $redirects, 'Duplicate redirect should be skipped for a non-default language too.'); + } + /** * Tests that a private:// destination produces a web-accessible redirect. *