commit 3e8e6cc42a0ebf2e6160cdd470fc715903cc26b7 Author: Rajab Natshah Date: Wed Aug 31 15:36:10 2022 +0300 Issue #2879648: Redirects from aliased paths aren't triggered diff --git a/redirect.module b/redirect.module index 18e61cf..0b1ad61 100644 --- a/redirect.module +++ b/redirect.module @@ -99,9 +99,10 @@ function redirect_path_alias_update(PathAliasInterface $path_alias) { // Delete all redirects having the same source as this alias. redirect_delete_by_path($path_alias->getAlias(), $path_alias->language()->getId(), FALSE); - // Create redirect from the old path alias to the new one. if ($original_path_alias->getAlias() != $path_alias->getAlias()) { - if (!redirect_repository()->findMatchingRedirect($original_path_alias->getAlias(), [], $original_path_alias->language()->getId())) { + $existing_redirect = redirect_repository()->findMatchingRedirect($original_path_alias->getAlias(), [], $original_path_alias->language()->getId()); + if (!$existing_redirect) { + // Create redirect from the old path alias to the new one. $redirect = Redirect::create(); $redirect->setSource($original_path_alias->getAlias()); $redirect->setRedirect($path_alias->getPath()); @@ -109,6 +110,13 @@ function redirect_path_alias_update(PathAliasInterface $path_alias) { $redirect->setStatusCode($config->get('default_status_code')); $redirect->save(); } + else { + // Duplicate the existing redirect with: + // source = [new alias], dest = [existing redirect]. + $redirect = $existing_redirect->createDuplicate(); + $redirect->setSource($path_alias->getAlias()); + $redirect->save(); + } } } diff --git a/redirect.services.yml b/redirect.services.yml index 578acb0..7d9a036 100644 --- a/redirect.services.yml +++ b/redirect.services.yml @@ -11,7 +11,7 @@ services: arguments: ['@config.factory', '@state', '@access_manager', '@current_user', '@router.route_provider'] redirect.request_subscriber: class: Drupal\redirect\EventSubscriber\RedirectRequestSubscriber - arguments: ['@redirect.repository', '@language_manager', '@config.factory', '@path_alias.manager', '@module_handler', '@entity_type.manager', '@redirect.checker', '@router.request_context', '@path_processor_manager'] + arguments: ['@redirect.repository', '@language_manager', '@config.factory', '@path_alias.manager', '@module_handler', '@entity_type.manager', '@redirect.checker', '@router.request_context', '@redirect.path_processor_manager'] tags: - { name: event_subscriber } redirect.settings_cache_tag: @@ -28,3 +28,7 @@ services: class: Drupal\redirect\Routing\RouteSubscriber tags: - { name: event_subscriber } + redirect.path_processor_manager: + class: Drupal\redirect\PathProcessor\RedirectPathProcessorManager + tags: + - { name: service_collector, tag: path_processor_inbound, call: addInbound } diff --git a/src/EventSubscriber/RedirectRequestSubscriber.php b/src/EventSubscriber/RedirectRequestSubscriber.php index b01869d..c011788 100644 --- a/src/EventSubscriber/RedirectRequestSubscriber.php +++ b/src/EventSubscriber/RedirectRequestSubscriber.php @@ -7,11 +7,11 @@ use Drupal\Core\Config\ConfigFactoryInterface; use Drupal\Core\Entity\EntityTypeManagerInterface; use Drupal\Core\Extension\ModuleHandlerInterface; use Drupal\Core\Language\LanguageManagerInterface; -use Drupal\Core\PathProcessor\InboundPathProcessorInterface; use Drupal\Core\Routing\TrustedRedirectResponse; use Drupal\Core\Url; use Drupal\path_alias\AliasManagerInterface; use Drupal\redirect\Exception\RedirectLoopException; +use Drupal\redirect\PathProcessor\RedirectPathProcessorManagerInterface; use Drupal\redirect\RedirectChecker; use Drupal\redirect\RedirectRepository; use Symfony\Component\HttpFoundation\Response; @@ -64,11 +64,11 @@ class RedirectRequestSubscriber implements EventSubscriberInterface { protected $context; /** - * A path processor manager for resolving the system path. + * The redirect path processor manager. * - * @var \Drupal\Core\PathProcessor\InboundPathProcessorInterface + * @var \Drupal\redirect\PathProcessor\RedirectPathProcessorManagerInterface */ - protected $pathProcessor; + protected $redirectPathProcessor; /** * Constructs a \Drupal\redirect\EventSubscriber\RedirectRequestSubscriber object. @@ -89,8 +89,10 @@ class RedirectRequestSubscriber implements EventSubscriberInterface { * The redirect checker service. * @param \Symfony\Component\Routing\RequestContext * Request context. + * @param \Drupal\redirect\PathProcessor\RedirectPathProcessorManagerInterface $redirect_path_processor + * The redirect path processor manager. */ - public function __construct(RedirectRepository $redirect_repository, LanguageManagerInterface $language_manager, ConfigFactoryInterface $config, AliasManagerInterface $alias_manager, ModuleHandlerInterface $module_handler, EntityTypeManagerInterface $entity_type_manager, RedirectChecker $checker, RequestContext $context, InboundPathProcessorInterface $path_processor) { + public function __construct(RedirectRepository $redirect_repository, LanguageManagerInterface $language_manager, ConfigFactoryInterface $config, AliasManagerInterface $alias_manager, ModuleHandlerInterface $module_handler, EntityTypeManagerInterface $entity_type_manager, RedirectChecker $checker, RequestContext $context, RedirectPathProcessorManagerInterface $redirect_path_processor) { $this->redirectRepository = $redirect_repository; $this->languageManager = $language_manager; $this->config = $config->get('redirect.settings'); @@ -99,7 +101,7 @@ class RedirectRequestSubscriber implements EventSubscriberInterface { $this->entityTypeManager = $entity_type_manager; $this->checker = $checker; $this->context = $context; - $this->pathProcessor = $path_processor; + $this->redirectPathProcessor = $redirect_path_processor; } /** @@ -124,25 +126,14 @@ class RedirectRequestSubscriber implements EventSubscriberInterface { // Get URL info and process it to be used for hash generation. $request_query = $request->query->all(); - if (strpos($request->getPathInfo(), '/system/files/') === 0 && !$request->query->has('file')) { - // Private files paths are split by the inbound path processor and the - // relative file path is moved to the 'file' query string parameter. This - // is because the route system does not allow an arbitrary amount of - // parameters. We preserve the path as is returned by the request object. - // @see \Drupal\system\PathProcessor\PathProcessorFiles::processInbound() - $path = $request->getPathInfo(); - } - else { - // Do the inbound processing so that for example language prefixes are - // removed. - $path = $this->pathProcessor->processInbound($request->getPathInfo(), $request); - } - $path = trim($path, '/'); + // Get the possible paths for the current request. + $paths = $this->redirectPathProcessor->getRedirectRequestPaths($request); + // Store current request context. $this->context->fromRequest($request); try { - $redirect = $this->redirectRepository->findMatchingRedirect($path, $request_query, $this->languageManager->getCurrentLanguage()->getId()); + $redirect = $this->redirectRepository->findMatchingRedirectMultiple($paths, $request_query, $this->languageManager->getCurrentLanguage()->getId()); } catch (RedirectLoopException $e) { \Drupal::logger('redirect')->warning('Redirect loop identified at %path for redirect %rid', ['%path' => $e->getPath(), '%rid' => $e->getRedirectId()]); diff --git a/src/PathProcessor/RedirectPathProcessorManager.php b/src/PathProcessor/RedirectPathProcessorManager.php new file mode 100644 index 0000000..c430edd --- /dev/null +++ b/src/PathProcessor/RedirectPathProcessorManager.php @@ -0,0 +1,92 @@ +applies($processor)) { + parent::addInbound($processor, $priority); + } + } + + /** + * {@inheritdoc} + */ + public function getRedirectRequestPaths(Request $request) { + $paths = []; + + // Do the inbound processing to get the source path. + $path = $request->getPathInfo(); + $source_path = $this->processRedirectInbound($path, $request); + $paths[] = trim($source_path, '/'); + + // Add the aliased path. + $alias_path = $this->processRedirectInbound($path, $request, TRUE); + $paths[] = trim($alias_path, '/'); + + return array_filter(array_unique($paths)); + } + + /** + * Processes the inbound path. + * + * @param string $path + * The path to process, with a leading slash. + * @param \Symfony\Component\HttpFoundation\Request $request + * The request object. + * @param bool $skip_alias + * Whether to skip AliasPathProcessor::processInbound. + * + * @return string + * The processed path. + */ + protected function processRedirectInbound($path, Request $request, $skip_alias = FALSE) { + $processors = $this->getInbound(); + foreach ($processors as $processor) { + // Skip AliasPathProcessor::processInbound if specified. + if ($skip_alias === TRUE && $processor instanceof AliasPathProcessor) { + continue; + } + + $path = $processor->processInbound($path, $request); + } + return $path; + } + + /** + * Returns whether this processor should be added or not. + * + * @param \Drupal\Core\PathProcessor\InboundPathProcessorInterface $processor + * The processor object to add. + * + * @return bool + * True if the processor should be added, false otherwise. + */ + protected function applies(InboundPathProcessorInterface $processor) { + // Skip PathProcessorFiles::processInbound(). + // Private files paths are split by the inbound path processor and the + // relative file path is moved to the 'file' query string parameter. This + // is because the route system does not allow an arbitrary amount of + // parameters. + if ($processor instanceof PathProcessorFiles) { + return FALSE; + } + return TRUE; + } + +} \ No newline at end of file diff --git a/src/PathProcessor/RedirectPathProcessorManagerInterface.php b/src/PathProcessor/RedirectPathProcessorManagerInterface.php new file mode 100644 index 0000000..4da3877 --- /dev/null +++ b/src/PathProcessor/RedirectPathProcessorManagerInterface.php @@ -0,0 +1,23 @@ +getHashesByPath($source_path, $query, $language)); + } + + return $this->findRedirectByHashes($hashes, reset($source_paths), $language); + } + + /** + * Finds a redirect from the given path, query and language. * * @param string $source_path * The redirect source path. * @param array $query * The redirect source path query. - * @param $language + * @param string $language * The language for which is the redirect. * * @return \Drupal\redirect\Entity\Redirect @@ -63,6 +87,24 @@ class RedirectRepository { * @throws \Drupal\redirect\Exception\RedirectLoopException */ public function findMatchingRedirect($source_path, array $query = [], $language = Language::LANGCODE_NOT_SPECIFIED) { + $hashes = $this->getHashesByPath($source_path, $query, $language); + return $this->findRedirectByHashes($hashes, $source_path, $language); + } + + /** + * Returns redirect hashes for the given source path. + * + * @param string $source_path + * The redirect source path. + * @param array $query + * The redirect source path query. + * @param string $language + * The language for which is the redirect. + * + * @return array + * An array of redirect hashes. + */ + protected function getHashesByPath($source_path, array $query, $language) { $source_path = ltrim($source_path, '/'); $hashes = [Redirect::generateHash($source_path, $query, $language)]; if ($language != Language::LANGCODE_NOT_SPECIFIED) { @@ -77,6 +119,23 @@ class RedirectRepository { } } + return $hashes; + } + + /** + * Finds a redirect from the given hashes. + * + * @param array $hashes + * An array of redirect hashes. + * @param string $source_path + * The redirect source path. + * @param string $language + * The language for which is the redirect. + * + * @return \Drupal\redirect\Entity\Redirect|null + * The matched redirect entity or null if not found. + */ + protected function findRedirectByHashes(array $hashes, $source_path, $language) { // Load redirects by hash. A direct query is used to improve performance. $rid = $this->connection->query('SELECT rid FROM {redirect} WHERE hash IN (:hashes[]) ORDER BY LENGTH(redirect_source__query) DESC', [':hashes[]' => $hashes])->fetchField(); @@ -128,8 +187,8 @@ class RedirectRepository { * @param string $source_path * The redirect source path (without the query). * - * @return \Drupal\redirect\Entity\Redirect[] - * Array of redirect entities. + * @return \Drupal\redirect\Entity\Redirect|null + * The matched redirect entity or null if not found. */ public function findBySourcePath($source_path) { $ids = $this->manager->getStorage('redirect')->getQuery() diff --git a/tests/src/Functional/RedirectUILanguageTest.php b/tests/src/Functional/RedirectUILanguageTest.php index a7dfbe5..0526b48 100644 --- a/tests/src/Functional/RedirectUILanguageTest.php +++ b/tests/src/Functional/RedirectUILanguageTest.php @@ -2,6 +2,7 @@ namespace Drupal\Tests\redirect\Functional; +use Drupal\Core\Language\Language; use Drupal\language\Entity\ConfigurableLanguage; /** @@ -127,4 +128,36 @@ class RedirectUILanguageTest extends RedirectUITest { $this->assertRedirect('de/langpath', '/de/user', 301); } + /** + * Test language specific redirects with node aliases. + */ + public function testLanguageSpecificNodeAliasRedirects() { + $this->drupalLogin($this->adminUser); + + // Create a node with alias "test-alias". + $this->drupalCreateNode([ + 'type' => 'article', + 'title' => 'Test english node', + 'path' => ['alias' => '/test-alias'], + ]); + + // Create a redirect with en language. + $this->drupalPostForm('admin/config/search/redirect/add', [ + 'redirect_source[0][path]' => 'test-alias', + 'redirect_redirect[0][uri]' => '/node', + 'language[0][value]' => 'en', + ], t('Save')); + $this->assertRedirect('test-alias', '/node', 301); + $this->assertRedirect('de/test-alias', NULL, 404); + + // Create a redirect with de language. + $this->drupalPostForm('admin/config/search/redirect/add', [ + 'redirect_source[0][path]' => 'test-alias', + 'redirect_redirect[0][uri]' => '/node/add', + 'language[0][value]' => 'de', + ], t('Save')); + $this->assertRedirect('test-alias', '/node', 301); + $this->assertRedirect('de/test-alias', 'de/node/add', 301); + } + } diff --git a/tests/src/Functional/RedirectUITest.php b/tests/src/Functional/RedirectUITest.php index e1800e3..23796d1 100644 --- a/tests/src/Functional/RedirectUITest.php +++ b/tests/src/Functional/RedirectUITest.php @@ -275,4 +275,60 @@ class RedirectUITest extends BrowserTestBase { $this->drupalLogin($this->adminUser); } + /** + * Test redirects with node aliases. + */ + public function testNodeAliasRedirects() { + $this->drupalLogin($this->adminUser); + + // Create a node with alias "test-alias". + $node = $this->drupalCreateNode([ + 'type' => 'article', + 'title' => 'Test node redirect', + 'langcode' => Language::LANGCODE_NOT_SPECIFIED, + 'path' => ['alias' => '/test-alias'], + ]); + + // Create a redirect from "test-alias" to "/node" and assert redirect. + $this->drupalPostForm('admin/config/search/redirect/add', [ + 'redirect_source[0][path]' => 'test-alias', + 'redirect_redirect[0][uri]' => '/node', + ], t('Save')); + $this->assertRedirect('test-alias', '/node', 301); + + // Update the node alias to "test-alias-updated". + $this->drupalPostForm('node/' . $node->id() . '/edit', ['path[0][alias]' => '/test-alias-updated'], t('Save')); + // Assert the "test-alias" redirect is still present and was duplicated for + // the new alias. + $this->assertRedirect('test-alias', '/node', 301); + $this->assertRedirect('test-alias-updated', '/node', 301); + + // Delete the node and assert redirects are still present. + $node->delete(); + $this->assertRedirect('test-alias', '/node', 301); + $this->assertRedirect('test-alias-updated', '/node', 301); + } + + /** + * Test adding a node alias when a redirect already exists.. + */ + public function testNodeAliasOnExistingRedirect() { + $this->drupalLogin($this->adminUser); + + $this->drupalPostForm('admin/config/search/redirect/add', [ + 'redirect_source[0][path]' => 'some-url', + 'redirect_redirect[0][uri]' => '/node', + ], t('Save')); + + $this->assertRedirect('some-url', '/node', 301); + + $this->drupalCreateNode([ + 'type' => 'article', + 'title' => 'Test node redirect', + 'path' => ['alias' => '/some-url'], + ]); + + $this->assertRedirect('some-url', '/node', 301); + } + } diff --git a/tests/src/Unit/RedirectRequestSubscriberTest.php b/tests/src/Unit/RedirectRequestSubscriberTest.php index 18c179b..dec6520 100644 --- a/tests/src/Unit/RedirectRequestSubscriberTest.php +++ b/tests/src/Unit/RedirectRequestSubscriberTest.php @@ -135,11 +135,12 @@ class RedirectRequestSubscriberTest extends UnitTestCase { $context = $this->createMock('Symfony\Component\Routing\RequestContext'); - $inbound_path_processor = $this->createMock('Drupal\Core\PathProcessor\InboundPathProcessorInterface'); + $inbound_path_processor = $this->createMock('Drupal\redirect\PathProcessor\RedirectPathProcessorManagerInterface'); $inbound_path_processor->expects($this->any()) - ->method('processInbound') - ->with($request->getPathInfo(), $request) - ->willReturnCallback(function ($path, Request $request) { + ->method('getRedirectRequestPaths') + ->with($request) + ->willReturnCallback(function (Request $request) { + $path = $request->getPathInfo(); if (strpos($path, '/system/files/') === 0 && !$request->query->has('file')) { // Private files paths are split by the inbound path processor and the // relative file path is moved to the 'file' query string parameter. @@ -148,7 +149,7 @@ class RedirectRequestSubscriberTest extends UnitTestCase { // @see \Drupal\system\PathProcessor\PathProcessorFiles::processInbound() $path = '/system/files'; } - return $path; + return [trim($path, '/')]; }); $alias_manager = $this->createMock(AliasManagerInterface::class); @@ -156,7 +157,7 @@ class RedirectRequestSubscriberTest extends UnitTestCase { $entity_type_manager = $this->createMock(EntityTypeManagerInterface::class); $subscriber = new RedirectRequestSubscriber( - $this->getRedirectRepositoryStub('findMatchingRedirect', $redirect), + $this->getRedirectRepositoryStub('findMatchingRedirectMultiple', $redirect), $this->getLanguageManagerStub(), $this->getConfigFactoryStub(['redirect.settings' => ['passthrough_querystring' => $retain_query]]), $alias_manager,