diff --git a/src/Entity/Redirect.php b/src/Entity/Redirect.php index c1796c1f..35f572d2 100644 --- a/src/Entity/Redirect.php +++ b/src/Entity/Redirect.php @@ -8,6 +8,9 @@ use Drupal\Core\Entity\EntityStorageInterface; use Drupal\Core\Entity\EntityTypeInterface; use Drupal\Core\Field\BaseFieldDefinition; +use Drupal\Core\Language\Language; +use Drupal\Core\Language\LanguageDefault; +use Drupal\Core\Language\LanguageInterface; use Drupal\link\LinkItemInterface; /** @@ -85,9 +88,25 @@ public static function preCreate(EntityStorageInterface $storage_controller, arr * {@inheritdoc} */ public function preSave(EntityStorageInterface $storage_controller) { - // Get the language code directly from the field as language() might not - // be up to date if the language was just changed. - $this->set('hash', Redirect::generateHash($this->redirect_source->path, (array) $this->redirect_source->query, $this->get('language')->value)); + $path = $this->redirect_source->path; + + // Get the language code directly from the field as language() might not be + // up to date if the language was just changed. + $redirect_language = $this->get('language')->value; + + // Pass the path through the alias manager to convert it to the the alias's + // source. If it is not an alias the original value will be returned. Check + // for the Redirect's language, then language not specified and finally the + // default language for path aliases. We have to try multiple languages as + // there is no way to determine what the "correct" alias language might be + // without checking all options. + /** @var \Drupal\Core\Path\AliasManagerInterface $path_alias_manager */ + $path_alias_manager = \Drupal::service('path.alias_manager'); + if (!\Drupal::service('module_handler')->moduleExists('language')) { + $redirect_language = \Drupal::languageManager()->getCurrentLanguage()->getId(); + } + $path = ltrim($path_alias_manager->getPathByAlias('/' . $path, $redirect_language), '/'); + $this->set('hash', Redirect::generateHash($path, (array) $this->redirect_source->query, $redirect_language)); } /** diff --git a/src/EventSubscriber/RedirectRequestSubscriber.php b/src/EventSubscriber/RedirectRequestSubscriber.php index 48fafc2d..e1672b4e 100644 --- a/src/EventSubscriber/RedirectRequestSubscriber.php +++ b/src/EventSubscriber/RedirectRequestSubscriber.php @@ -163,10 +163,21 @@ public function onKernelRequestCheckRedirect(GetResponseEvent $event) { if ($this->config->get('passthrough_querystring')) { $url->setOption('query', (array) $url->getOption('query') + $request_query); } + + // Redirects to path aliases are stored and searched for using the alias's + // source (/node/123 vs /foo-bar-path). This can cause redirect loops + // because the path processor above may have converted a path alias to a + // path source (/foo-bar-path => /node/123) which will trigger a redirect + // on the path's valid alias. + $redirect_url = $url->setAbsolute()->toString(); + if ($redirect_url === ($request->getSchemeAndHttpHost() . $request->getRequestUri())) { + return; + } + $headers = [ 'X-Redirect-ID' => $redirect->id(), ]; - $response = new TrustedRedirectResponse($url->setAbsolute()->toString(), $redirect->getStatusCode(), $headers); + $response = new TrustedRedirectResponse($redirect_url, $redirect->getStatusCode(), $headers); $response->addCacheableDependency($redirect); $event->setResponse($response); } diff --git a/src/Form/RedirectForm.php b/src/Form/RedirectForm.php index 1b4f4534..3dd7d032 100644 --- a/src/Form/RedirectForm.php +++ b/src/Form/RedirectForm.php @@ -54,7 +54,7 @@ protected function prepareEntity() { } } - $redirect->setLanguage($this->getRequest()->get('language') ? $this->getRequest()->get('language') : Language::LANGCODE_NOT_SPECIFIED); + $redirect->setLanguage($this->getRequest()->get('language') ? $this->getRequest()->get('language') : \Drupal::languageManager()->getCurrentLanguage()->getId()); } } @@ -124,18 +124,14 @@ public function validateForm(array &$form, FormStateInterface $form_state) { // Do nothing, we want to only compare the resulting URLs. } - $parsed_url = UrlHelper::parse(trim($source['path'])); - $path = isset($parsed_url['path']) ? $parsed_url['path'] : NULL; - $query = isset($parsed_url['query']) ? $parsed_url['query'] : NULL; - $hash = Redirect::generateHash($path, $query, $form_state->getValue('language')[0]['value']); - // Search for duplicate. - $redirects = \Drupal::entityTypeManager() - ->getStorage('redirect') - ->loadByProperties(['hash' => $hash]); + $parsed_url = UrlHelper::parse(trim($source['path'])); + $path = isset($parsed_url['path']) ? $parsed_url['path'] : ''; + $query = isset($parsed_url['query']) ? $parsed_url['query'] : []; + $language = $form_state->getValue('language')[0]['value']; + $redirect = \Drupal::service('redirect.repository')->findMatchingRedirect($path, $query, $language); - if (!empty($redirects)) { - $redirect = array_shift($redirects); + if (!empty($redirect)) { if ($this->entity->isNew() || $redirect->id() != $this->entity->id()) { $form_state->setErrorByName('redirect_source', $this->t('The source path %source is already being redirected. Do you want to edit the existing redirect?', [ diff --git a/src/RedirectRepository.php b/src/RedirectRepository.php index ff1ccf8b..53ee4981 100644 --- a/src/RedirectRepository.php +++ b/src/RedirectRepository.php @@ -6,6 +6,7 @@ use Drupal\Core\Database\Connection; use Drupal\Core\Entity\EntityTypeManagerInterface; use Drupal\Core\Language\Language; +use Drupal\Core\Language\LanguageInterface; use Drupal\redirect\Entity\Redirect; use Drupal\redirect\Exception\RedirectLoopException; @@ -69,6 +70,15 @@ public function findMatchingRedirect($source_path, array $query = [], $language $hashes[] = Redirect::generateHash($source_path, $query, Language::LANGCODE_NOT_SPECIFIED); } + // The source path might be an alias, so add the source for the alias if it + // is. The alias manager will return the source path as is if it's not an + // alias, which will lead to a duplicate it in the hashes array, deal with + // that too. + $alias_language = ($language == Language::LANGCODE_NOT_SPECIFIED) ? NULL : $language; + $alias_source = ltrim(\Drupal::service('path.alias_manager')->getPathByAlias('/' . $source_path, $alias_language), '/'); + $hashes[] = Redirect::generateHash($alias_source, $query, Language::LANGCODE_NOT_SPECIFIED); + $hashes = array_unique($hashes); + // Add a hash without the query string if using passthrough querystrings. if (!empty($query) && $this->config->get('passthrough_querystring')) { $hashes[] = Redirect::generateHash($source_path, [], $language); diff --git a/tests/src/Functional/RedirectUITest.php b/tests/src/Functional/RedirectUITest.php index 34ab60c7..cdfc7a55 100644 --- a/tests/src/Functional/RedirectUITest.php +++ b/tests/src/Functional/RedirectUITest.php @@ -91,6 +91,7 @@ public function testAutomaticRedirects() { // Test that changing the path back deletes the first redirect, creates // a new one and does not result in a loop. $this->drupalPostForm('node/' . $node->id() . '/edit', ['path[0][alias]' => '/node_test_alias'], t('Save')); + \Drupal::service('path.alias_manager')->cacheClear(); $redirect = $this->repository->findMatchingRedirect('node_test_alias', [], Language::LANGCODE_NOT_SPECIFIED); $this->assertTrue(empty($redirect)); diff --git a/tests/src/Kernel/RedirectAPITest.php b/tests/src/Kernel/RedirectAPITest.php index 972d0c71..303fffd0 100644 --- a/tests/src/Kernel/RedirectAPITest.php +++ b/tests/src/Kernel/RedirectAPITest.php @@ -160,6 +160,18 @@ public function testRedirectEntity() { else { $this->fail('findMatchingRedirect is case sensitive.'); } + + // Redirects to a path alias should be findable by the alias's source as the + // hash is based on the alias's source. + \Drupal::service('path.alias_storage')->save('/node', '/node-alias'); + $redirect = $this->controller->create(); + $redirect->setSource('node-alias'); + $redirect->setRedirect('/some-different-url'); + $redirect->save(); + $found = $repository->findMatchingRedirect('/node'); + $message = 'Failed to find a redirect to a path alias by searching for the path source.'; + $this->assertInstanceOf(Redirect::class, $found, $message); + $this->assertEqual($redirect->getHash(), $found->getHash(), $message); } /**