md-systems / redirect Goto Github PK

When the redirect check happens in Drupal\redirect\RedirectRepository redirects entered as redirect fields in nodes are not searched.

That makes since because loading field config and field instances on a request event would be terrible for performance. I would expect that field type redirects are discovered via state or cache but also could also be duplicated as redirect entities.

Am I missing something or is there more work to be done with redirect fields?

When using different negotiation methods for interface and content, there are redirect loops.

I tracked this down to redirectNormalizeAliases(), where we are checking if the used path is the same one than saved for the current system path. However, we call $this->languageManager->getCurrentLanguage() which defaults to interface language.

I think it should use $this->languageManager->getCurrentLanguage(LanguageInterface::TYPE_CONTENT) instead.

I'm wondering how the D7 function redirect_page_cache_clear() works for others. (It's in src/Form/RedirectSettingsForm.php, line 105):

public function submitForm(array &$form, FormStateInterface $form_state) {
  $config = $this->config('redirect.settings');
  foreach ($form_state->getValues() as $key => $value) {
    if (strpos($key, 'redirect_') !== FALSE) {
      $config->set(str_replace('redirect_', '', $key), $value);
    }
  }
  $config->save();
  redirect_page_cache_clear();
  drupal_set_message(t('Configuration was saved.'));
}

For me it breaks the settings form submit, should we remove it? Also, should we just remove it or replace it with an alternative?

I want to give my users the ability to CRUD redirects but I do not want them to change the global settings for them. Could you add new permission for the settings page?

I don't know whats the roadmap for this module, but the base is usable.
Would you mind to mirror the 8.x branch to Drupal.org and implement SemVer?

That would allow us to use drupal-pakagist for our composer setup properly.

Since this module existed as Path Redirect and migrate is in core with many migration templates, it should be fairly easy to add a migration template and source for this module.

This functionality is used in globalredirect module so we need to decide what to do.
#36

Both redirects and 404s have problems with an environment that involves multiple domains.

By default, Drupal ignores the domain mostly and just considers the routes. Except for per-language domains... Then we have aliases that are limited to a language only.
The log is now already language aware.

When releasing a site that previously had multiple domains, then redirects possibly want to be limited to a specific domain.

The idea is to add an optional domain field per redirect (needs consideration in the hash, much work...).
And 404s should be clearly identifyable, so a fix404 is properly prefilled with the domain.

A redirect might want to target to a different domain. This needs investigation.
There are many potential problems such as entities that change language on update and generate redirects. Also the combination with the recent recursive resolution of redirects needs investigation.

When you try to add a redirect in the form if you start filing the path, the when you try to fill the rest of fields after the validation of path the input placeholder is moved again to the path.
So it don't let you fill the rest of fields.

This is what happens when you attempt to generate redirects using Drush:

juampy@Juampy/var/www/drupal8(8.1.x)$ drush generate-redirects 5
PHP Fatal error:  Call to undefined function drush_generate_is_number() in /var/www/drupal8/modules/contrib/redirect/redirect.drush.inc on line 30
PHP Stack trace:
PHP   1. {main}() /usr/share/drush/drush.php:0
PHP   2. drush_main() /usr/share/drush/drush.php:12
PHP   3. Drush\Boot\BaseBoot->bootstrap_and_dispatch() /usr/share/drush/includes/preflight.inc:66
PHP   4. drush_dispatch() /usr/share/drush/lib/Drush/Boot/BaseBoot.php:67
PHP   5. call_user_func_array:{/usr/share/drush/includes/command.inc:185}() /usr/share/drush/includes/command.inc:185
PHP   6. drush_command() /usr/share/drush/includes/command.inc:185
PHP   7. _drush_invoke_hooks() /usr/share/drush/includes/command.inc:217
PHP   8. call_user_func_array:{/usr/share/drush/includes/command.inc:366}() /usr/share/drush/includes/command.inc:366
PHP   9. drush_redirect_generate_redirects() /usr/share/drush/includes/command.inc:366
Drush command terminated abnormally due to an unrecoverable error.                                                                                                   [error]
Error: Call to undefined function drush_generate_is_number() in /var/www/drupal8/modules/contrib/redirect/redirect.drush.inc, line 30

Hey, I installed the latest version of Redirect and am trying it out. So far, looks alright, but noticed the Fix 404 Pages page threw the following error:

Drupal\Core\Database\DatabaseExceptionWrapper:
SQLSTATE[42703]: Undefined column: 7 ERROR: column "count" does not exist
LINE 1: SELECT count AS count, w.message AS message, w.variables AS ... ^:
SELECT count AS count, w.message AS message, w.variables AS variables, 
COUNT(wid) AS count, MAX(timestamp) AS timestamp FROM {watchdog} w 
LEFT OUTER JOIN {redirect} r ON w.message = r.redirect_source__path 
WHERE (r.rid IS NULL ) AND (w.type = :db_condition_placeholder_0) 
GROUP BY w.message, w.variables ORDER BY count DESC 
NULLS LAST LIMIT 25 OFFSET 0; 
Array ( [:db_condition_placeholder_0] => page not found ) 
in Drupal\redirect\Form\RedirectFix404Form->buildForm()
(line 96 of /path/to/site/root/modules/sandbox/redirect/src/Form/RedirectFix404Form.php).

Am guessing that first count AS count has crept in undetected and needs removing. Before I try and put together a PR to fix this, has anyone else spotted it yet?

Hi,

I'm testing the latest dev version of redirect module.

If I check in the settings the option "Check access to the redirected page", and then if you try to go to any page on url of type /node/x, then you've got a fatal error. See below.

The website encountered an unexpected error. Please try again later.
Recoverable fatal error: Argument 1 passed to Drupal\Core\Access\AccessManager::check() must implement interface Drupal\Core\Routing\RouteMatchInterface, instance of Symfony\Component\Routing\Route given, called in /srv/www/flocon2016/modules/redirect/src/RedirectChecker.php on line 72 and defined in Drupal\Core\Access\AccessManager->check() (line 123 of core/lib/Drupal/Core/Access/AccessManager.php).
Drupal\Core\Access\AccessManager->check(Object, Object, Object)
Drupal\redirect\RedirectChecker->canRedirect(Object, 'entity.node.canonical')
Drupal\redirect\EventSubscriber\RedirectRequestSubscriber->setResponse(Object, Object)
Drupal\redirect\EventSubscriber\RedirectRequestSubscriber->redirectNormalizeAliases(Object, 'kernel.request', Object)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.request', Object)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1)
Stack\StackedHttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)

I've been doing some profiling, and the request subscriber uses ~3.5% of the whole request time (which is 300-500ms).

2% of the total time is that method. And it's doing a loadByProperties(), which is doing an entity query, which needs to load the field storage definitions, and so on, and so on.

Redirect source widget ajax validation causes a warning and recoverable fatal error which prevents me from creating redirects:

Warning: array_merge_recursive(): Argument #1 is not an array in Drupal\Core\Render\BubbleableMetadata::mergeAttachments() (line 170 of core/lib/Drupal/Core/Render/BubbleableMetadata.php).

Recoverable fatal error: Argument 1 passed to Drupal\Core\Ajax\AjaxResponse::setAttachments() must be of the type array, null given, called in core/lib/Drupal/Core/Ajax/AjaxResponse.php on line 57 and defined in Drupal\Core\Ajax\AjaxResponse->setAttachments() (line 42 of core/lib/Drupal/Core/Render/AttachmentsTrait.php).

It is generally a best practice to disable dblog on production sites to improve performance. However, if you disable dblog, you cannot configure redirects. If you go to /admin/config/search/redirect, the following error appears in the Apache error log:

Uncaught PHP Exception Symfony\\Component\\Routing\\Exception\\RouteNotFoundException: "Route "redirect.fix_404" does not exist." at /docroot/core/lib/Drupal/Core/Routing/RouteProvider.php line 176

Instead of having the route for redirect.fix_404 depend on the optional dblog module, I suggest having the controller check whether dblog is enabled, and provide a helpful message if it is not.

I have a fresh Drupal 8 beta 15 install and am trying to add a redirect, however, when clicking the apply button nothing gets saved to the database and no error is generated.

The "Fix 404 pages" menu link appears twice.

https://github.com/md-systems/redirect/blob/8.x-1.x/redirect.links.menu.yml#L21

It's not working now with the GlobalRedirect merge and has to be re implemented.

Older versions of mysql disallow indexes on larger fields:

Next exception 'Drupal\Core\Database\DatabaseExceptionWrapper' with
message 'SQLSTATE[42000]: Syntax error or access violation: 1071
Specified key was too long; max key length is 767 bytes: CREATE TABLE
{redirect} (
`rid` INT NOT NULL auto_increment, 
`type` VARCHAR(255) NOT NULL, 
`uuid` VARCHAR(128) NOT NULL, 
`language` VARCHAR(12) NOT NULL, 
`hash` VARCHAR(255) NULL DEFAULT NULL, 
`uid` INT unsigned NULL DEFAULT NULL COMMENT 'The ID of the target
entity.', 
`redirect_source__path` VARCHAR(2048) NULL DEFAULT NULL COMMENT 'The
source path', 
`redirect_source__query` LONGBLOB NULL DEFAULT NULL COMMENT
'Serialized array of path queries', 
`redirect_redirect__uri` VARCHAR(2048) NULL DEFAULT NULL COMMENT 'The
URI of the link.', 
`redirect_redirect__title` VARCHAR(255) NULL DEFAULT NULL COMMENT
'The link text.', 
`redirect_redirect__options` LONGBLOB NULL DEFAULT NULL COMMENT
'Serialized array of options for the link.', 
`status_code` INT NULL DEFAULT NULL, 
PRIMARY KEY (`rid`), 
UNIQUE KEY `redirect_field__uuid__value` (`uuid`), 
UNIQUE KEY `hash` (`hash`), 
INDEX `redirect_field__uid__target_id` (`uid`), 
INDEX `redirect_field__redirect_source__path`
(`redirect_source__path`(50)), 
INDEX `redirect_field__redirect_redirect__uri`
(`redirect_redirect__uri`(30)), 
INDEX `source_language` (`redirect_source__path`, `language`)
) ENGINE = InnoDB DEFAULT CHARACTER SET utf8 COMMENT 'The base table
for redirect entities.';

I think the issue is on the combined source/language index introduced in #12.

Hi there,
I've tried this on a pristine install of Drupal 8.0.5 with the latest build of md-systems/redirect and I've found that I can't add redirects from the URL Redirects page (/admin/config/search/redirect/add)

I can add redirects from the Fix 401 Pages admin page but not from preemptively.

I'm interested in this for a project, How useable is the port at this time?

In case anybody else is looking, I used this code to update the stored hash schemas and actually change the db field:

  $entity_manager = \Drupal::entityManager();
  $storage = $entity_manager->getStorage('redirect');
  // Hash field has been shortened to 64 characters.
  if ($storage instanceof SqlEntityStorageInterface) {
    $original_definitions = $entity_manager->getLastInstalledFieldStorageDefinitions('redirect');

    // Hash to 64 max length.
    $current_field = \Drupal::keyValue('entity.storage_schema.sql')->get('redirect.field_schema_data.hash', []);
    $current_field['redirect']['fields']['hash']['length'] = 64;
    db_change_field('redirect', 'hash', 'hash', $current_field['redirect']['fields']['hash']);

    // Update stored schemas.
    \Drupal::keyValue('entity.storage_schema.sql')->set('redirect.field_schema_data.hash', $current_field);
    // Update stored field.
    $original_field = $original_definitions['hash'];
    // @todo there must be a better way?!
    $reflection = new ReflectionObject($original_field);
    $schema_property = $reflection->getProperty('schema');
    $schema_property->setAccessible(TRUE);
    $original_schema = $schema_property->getValue($original_field);
    $original_schema['columns']['value']['length'] = 64;

    $schema_property->setValue($original_field, $original_schema);
    $schema_property->setAccessible(FALSE);
    \Drupal::keyValue('entity.definitions.installed')->set('redirect.field_storage_definitions', $original_definitions);

See https://www.drupal.org/node/1796596

I committed an earlier version of the patch a while back: 3f11f06

We should update that according to the approach that was implented in 7.x-1.x (status and disable instead of delete I think, never really looked at the issue).

We should also check if other patches were committed and port what applies to the 8.x-1.x branch too.

Problem
At the moment the redirect module is not working as intended for a variety of multilingual scenarios.
For example when we create a redirect with a redirect language in the source path, it is not working at all, it doesn’t redirect. This is only working with the default language.

Another problem is with the target path. When we create a redirect to an any path it always ends up on a url with the language prefix that is set in the config, That leads to a possible limitation. A node that have previously been saved with the wrong language ending up with an alias (people link to it) and then we fix the language... The alias changes and there should be a redirect from the old language version to the new one. I fear this either leads to broken redirect target or no redirect at all. Simply said: Cross language redirects are impossible.

Proposal
Now a proposed solution to fix the second problem is adding an optional target language selector. With this the redirect will always end up on the selected target path even if the language is set to another.

See screenshot proposal for adding another selector with the target language. We need some feedback to decide what is better to solved this critical problem.

The target language “Not specified” should be altered to “- Source language -”.
The source language “- Not specified -” and other special items should be clarified such as “- All languages -” or so. Users don’t understand the implication of these values otherwise.

To be discussed
Selecting a language will trigger ajax update and will update the domain and language prefix with the path input. Still i’m not too happy about this uncommon pattern to output text just before the form text field. We should discuss this with an UX expert.

Also with the target, we should similarly make the user aware how a target path is interpreted (and output domain and language context for relative targets).

I'm using the Drupal core git repo at revision 1d1fe19702f5dc6894acc45431b1d0c85ab306d7 with the latest commit here, f15c76f. I was able to add a redirect, and the redirect actually works if I visit the URL, but the config page is no longer accessible after adding the first entry. This issue is repeatable.

I receive the following error:

Fatal error: Declaration of Drupal\redirect\Plugin\Field\FieldFormatter\RedirectSourceFormatter::viewElements() must be compatible with Drupal\Core\Field\FormatterInterface::viewElements(Drupal\Core\Field\FieldItemListInterface $items) in /var/www/sitename/code/modules/contrib/redirect/src/Plugin/Field/FieldFormatter/RedirectSourceFormatter.php on line 25

When working through fix404 page, a link should disappear once a redirect is added.

I think it was like this before..?

Should not be possible to call entity_view() on redirect entities. Please help review the following core patch: https://www.drupal.org/node/2567919

I have a fresh Drupal 8 beta 15 install with one node on it with a path of localhost/test-article which can be accessed at both localhost/test-article and localhost/ however, once the redirect module is enabled localhost/test-article gets redirected to localhost/. Disabling the module makes both paths available. This appears to only be happening to the node set to the home page.

If I click on it, it redirects me to the same page.

I'm not sure yet where this information should be stored, but doing it through entity->save() is bad for multiple reasons:

• A save invalidates the storage cache
• It also invalidates the redirect:$id and redirect_list cache tags, which aren't actually used, which results in two additional sql queries (updating the checksum table), and possibly notification of external caching system which also means that a cache redirect would be invalidated again immediately after caching it, every time.

Two ways to fix this:

1. add a new method on the storage handler specifically for this, which would at least avoid the cache tags, but we still have to invalidate the storage cache to get updated results in e.g. views (query would work, though)
2. Store it somewhere else, but it still needs to be possible to query it.

Hello all, is there a particular architectural reason for the field not extending EntityReference? I'm currently working on adding some logic to the provided field (https://github.com/janstoeckler/redirect/tree/field-enhancement) and was thinking about possibly using Entity Reference instead of extending FieltItemBase.

Are there any pros or cons I might be missing?

See title, currently causes pretty serious performance issues if you have a lot of redirects.

Needs a custom schema class.

After creating a redirect, I get a WSOD returning to the main redirect admin UI at /admin/config/search/redirect It seems the redirect is saved and works, as I am redirected as expected with the test entries made. Hitting that admin page, I get the following error on my system php error log:

PHP Fatal error: Declaration of Drupal\redirect\Plugin\Field\FieldFormatter\RedirectSourceFormatter::viewElements() must be compatible with Drupal\Core\Field\FormatterInterface::viewElements(Drupal\Core\Field\FieldItemListInterface $items) in modules/redirect/src/Plugin/Field/FieldFormatter/RedirectSourceFormatter.php on line 25

See #15

Page caching obviously breaks redirect loop protection. For now, we just disabled it for the test. I can see a few options:

a) Drop the feature, let browsers do it.
b) Replace the implementation, use a active check before redirecting, to at least still protect against redirect loops between redirect.module managed redirects.
c) Just keep it like it is, it will only work when page cache is disabled. Which means that for example authenticated users will notice it and can fix it.
d) Add an option to disallow caching of redirects.

I am running nginx + Perusio config.

I installed the latest 8.x release from this repo. I created a test redirect (node/123 > a valid article). When I visit http://site.dev/node/123 with query string retaining disabled, I get a 404. When retaining is enabled, I get a proper redirect, but I also get ?q=node/123 appending to my URL.

Neither are ideal, but the later at least redirects.

I assume this is a bug, not intended functionality?

The redirect API test needs to first test the multilingual cases. Most importantly cases where the same source URI has multiple records with different languages. Make sure then matchers return the right one.

Then the UI test should check all undfined / language specific creation and edit of links with the correct result of the persisted redirects.

Extend the RedirectSubscriberTest with a method that tests language variations to make sure the language contexts are properly resolved.

Finally, we should discuss as a followup, if we need a separate test that actually triggers redirects by accessing the URLs in a simpletest and check if the response is redirecting. Currently this is only tested implicitly through the RedirectRequestSubscriberTest.

Once #4 is merged, the remaining tests that aren't passing are due to redirects storing alias information instead of internal path (for instance, one failing test is looking for a redirect to node/1). Since the Url::getInternalPath() method is deprecated, this module may need to store router info when available.

Drupal 8.2.4
Redirect 8.x-1.0-alpha1

A quick fix is to turn off normalize_aliases.
I think the redirect module should only normalize GET requests, is that true?