From 09a0668f7681036bbb3f8a1b4ace0656a5bbf071 Mon Sep 17 00:00:00 2001 From: Nate Haug Date: Wed, 20 Apr 2016 17:32:21 -0700 Subject: [PATCH] Issue #905: Cleaning up validation on Redirect forms. Consistency fixes between aliases and redirects. --- core/modules/path/path.admin.inc | 6 + .../redirect/config/redirect.settings.json | 7 +- core/modules/redirect/redirect.admin.inc | 140 ++++++++---------- core/modules/redirect/redirect.install | 14 +- core/modules/redirect/redirect.module | 46 ++---- core/modules/redirect/tests/redirect.test | 24 +-- core/modules/system/system.install | 6 +- 7 files changed, 95 insertions(+), 148 deletions(-) diff --git a/core/modules/path/path.admin.inc b/core/modules/path/path.admin.inc index 68c07bd7709..451e59bf7f7 100755 --- a/core/modules/path/path.admin.inc +++ b/core/modules/path/path.admin.inc @@ -180,8 +180,14 @@ function path_admin_form($form, &$form_state, $path = array('source' => '', 'ali '#type' => 'submit', '#value' => t('Delete'), '#submit' => array('path_admin_form_delete_submit'), + '#limit_validation_errors' => array(array('actions'), array('pid')), ); } + $form['actions']['cancel'] = array( + '#type' => 'link', + '#title' => t('Cancel'), + '#href' => isset($_GET['destination']) ? $_GET['destination'] : 'admin/config/search/path', + ); return $form; } diff --git a/core/modules/redirect/config/redirect.settings.json b/core/modules/redirect/config/redirect.settings.json index 7a8c19445f0..3007a0be411 100644 --- a/core/modules/redirect/config/redirect.settings.json +++ b/core/modules/redirect/config/redirect.settings.json @@ -5,10 +5,5 @@ "passthrough_querystring": true, "warning": false, "additional_statuses": false, - "purge_inactive": false, - "global_home": true, - "global_clean": true, - "global_canonical": true, - "global_deslash": false, - "global_admin_paths": false + "purge_inactive": false } diff --git a/core/modules/redirect/redirect.admin.inc b/core/modules/redirect/redirect.admin.inc index 07ac41bc4bc..3b7af6a688a 100644 --- a/core/modules/redirect/redirect.admin.inc +++ b/core/modules/redirect/redirect.admin.inc @@ -374,7 +374,7 @@ function redirect_edit_form($form, &$form_state, $redirect = NULL) { '#maxlength' => 560, '#default_value' => $redirect->rid || $redirect->redirect ? redirect_url($redirect->redirect, $redirect->redirect_options, TRUE) : '', '#required' => TRUE, - '#description' => t('Enter an internal Backdrop path, path alias, or complete external URL (like http://example.com/) to redirect to. Use %front to redirect to the front page.', array('%front' => '')), + '#description' => t('Enter an internal Backdrop path, path alias, or complete external URL (like http://example.com/) to which the user will be sent. Use %front to redirect to the front page.', array('%front' => '')), '#element_validate' => array('redirect_element_validate_redirect'), ); $form['redirect_options'] = array( @@ -403,19 +403,30 @@ function redirect_edit_form($form, &$form_state, $redirect = NULL) { ); $form['override'] = array( - '#type' => 'checkbox', - '#title' => t('I understand the following warnings and would like to proceed with saving this URL redirect'), - '#default_value' => FALSE, - '#access' => FALSE, - '#required' => FALSE, - '#weight' => -100, '#prefix' => '
', '#suffix' => '
', + '#access' => FALSE, + '#weight' => -100, + ); + $form['override']['confirm'] = array( + '#type' => 'checkbox', + '#title' => t('I understand the above warnings and would like to proceed with saving this URL redirect'), + '#default_value' => FALSE, + '#parents' => array('override'), ); - if (!empty($form_state['storage']['override_messages'])) { + + if (!empty($form_state['storage']['override_messages']) && !count(form_get_errors())) { $form['override']['#access'] = TRUE; - //$form['override']['#required'] = TRUE; - $form['override']['#description'] = theme('item_list', array('items' => $form_state['storage']['override_messages'])); + if (count($form_state['storage']['override_messages']) > 1) { + $message_output = theme('item_list', array('items' => $form_state['storage']['override_messages'])); + } + else { + $message_output = implode('', $form_state['storage']['override_messages']); + } + $form['override']['messages'] = array( + '#markup' => $message_output, + '#weight' => -1, + ); // Reset the messages. $form_state['storage']['override_messages'] = array(); } @@ -423,7 +434,14 @@ function redirect_edit_form($form, &$form_state, $redirect = NULL) { $form['actions'] = array('#type' => 'actions'); $form['actions']['submit'] = array( '#type' => 'submit', - '#value' => t('Save'), + '#value' => t('Save redirect'), + ); + $form['actions']['delete'] = array( + '#type' => 'submit', + '#value' => t('Delete'), + '#submit' => array('redirect_edit_form_delete_submit'), + '#limit_validation_errors' => array(array('actions')), + '#access' => !$redirect->isNew(), ); $form['actions']['cancel'] = array( '#type' => 'link', @@ -440,26 +458,33 @@ function redirect_edit_form($form, &$form_state, $redirect = NULL) { * @see redirect_edit_form() */ function redirect_element_validate_source($element, &$form_state) { - $value = &$element['#value']; - // Check that the source contains no URL fragment. - if (strpos($value, '#') !== FALSE) { + if (strpos($element['#value'], '#') !== FALSE) { form_error($element, t('The source path cannot contain an URL fragment anchor.')); } - _redirect_extract_url_options($element, $form_state); + $parsed_value = _redirect_extract_url_options($element, $form_state); // Disallow redirections from the frontpage. - if ($value === '') { + if ($parsed_value['path'] === '') { form_error($element, t('The source path cannot be the front page.')); } // Cannot create redirects for valid paths. if (empty($form_state['values']['override'])) { - $menu_item = menu_get_item($value); - if ($menu_item && $menu_item['page_callback'] != 'redirect_redirect' && $value == $menu_item['path']) { - $form_state['storage']['override_messages']['valid-path'] = t('The source path %path is likely a valid path. It is preferred to create URL aliases for existing paths rather than redirects.', array('%path' => $value, '@url-alias' => url('admin/config/search/path/add'))); + // Check if this is an alias. + $alias = path_load(array('alias' => $parsed_value['path'])); + if ($alias) { + $form_state['storage']['override_messages']['existing-alias'] = t('The source path !path is an existing alias. You may prefer to edit this URL alias rather than using a redirect.', array('!path' => '' . l($parsed_value['path'], $parsed_value['path']) . '', '@url-alias' => url('admin/config/search/path/edit/' . $alias['pid']))); $form_state['rebuild'] = TRUE; + return $element; + } + $path = backdrop_get_normal_path($parsed_value['path'], $form_state['values']['langcode']); + $menu_item = menu_get_item($path); + if ($menu_item && $menu_item['page_callback'] != 'redirect_redirect') { + $form_state['storage']['override_messages']['valid-path'] = t('The source path !path is an existing path. Creating a redirect may make it inaccessible. You may prefer to create a URL alias for this path rather than a redirect.', array('!path' => '' . l($parsed_value['path'], $parsed_value['path']) . '', '@url-alias' => url('admin/config/search/path/add'))); + $form_state['rebuild'] = TRUE; + return $element; } } @@ -472,13 +497,14 @@ function redirect_element_validate_source($element, &$form_state) { * @see redirect_edit_form() */ function redirect_element_validate_redirect($element, &$form_state) { - $value = &$element['#value']; - _redirect_extract_url_options($element, $form_state); - $value = &$form_state['values']['redirect']; - - - // Normalize the path. - $value = backdrop_get_normal_path($value, $form_state['values']['langcode']); + $parsed = _redirect_extract_url_options($element, $form_state); + if (!url_is_external($parsed['url'])) { + $value = backdrop_get_normal_path($parsed['url'], $form_state['values']['langcode']); + } + else { + $value = $parsed['url']; + } + form_set_value($element, $value, $form_state); if (!valid_url($value) && !valid_url($value, TRUE) && $value != '' && $value != '' && !file_exists($value)) { form_error($element, t('The redirect path %value is not valid.', array('%value' => $value))); @@ -518,11 +544,6 @@ function _redirect_extract_url_options(&$element, &$form_state) { unset($options['https']); } - if (!url_is_external($parsed['url'])) { - $parsed['url'] = backdrop_get_normal_path($parsed['url'], $form_state['values']['langcode']); - } - - form_set_value($element, $parsed['url'], $form_state); return $parsed; } @@ -543,17 +564,24 @@ function redirect_edit_form_validate($form, &$form_state) { $form_state['rebuild'] = TRUE; } } - - if ($form['override']['#access']) { - backdrop_set_message('Did you read the warnings and click the checkbox?', 'error'); - $form_state['rebuild'] = TRUE; - } } redirect_validate($redirect, $form, $form_state); $form_state['redirect'] = $redirect; } +/** + * Form submit handler; handle the 'Delete' button on redirect_edit_form(). + */ +function redirect_edit_form_delete_submit($form, &$form_state) { + $destination = array(); + if (isset($_GET['destination'])) { + $destination = backdrop_get_destination(); + unset($_GET['destination']); + } + $form_state['redirect'] = array('admin/config/search/redirect/delete/' . $form_state['redirect']->rid, array('query' => $destination)); +} + /** * Form submit handler; insert or update an URL redirect. * @@ -639,41 +667,6 @@ function redirect_settings_form($form, &$form_state) { '#description' => t('Only redirects managed by the redirect module itself will be deleted. Redirects managed by other modules will be left alone.'), ); - $form['globals'] = array( - '#type' => 'fieldset', - '#title' => t('Always enabled redirections'), - '#description' => t('(formerly Global Redirect features)'), - '#access' => FALSE, - ); - $form['globals']['global_home'] = array( - '#type' => 'checkbox', - '#title' => t('Redirect from paths like index.php and /node to the root directory.'), - '#default_value' => $config->get('global_home'), - '#access' => FALSE, - ); - $form['globals']['global_clean'] = array( - '#type' => 'checkbox', - '#title' => t('Redirect from non-clean URLs to clean URLs.'), - '#default_value' => $config->get('global_clean'), - '#disabled' => !$config->get('clean_url'), - '#access' => FALSE, - ); - $form['globals']['global_canonical'] = array( - '#type' => 'checkbox', - '#title' => t('Redirect from non-canonical URLs to the canonical URLs.'), - '#default_value' => $config->get('global_canonical'), - ); - $form['globals']['global_deslash'] = array( - '#type' => 'checkbox', - '#title' => t('Remove trailing slashes from paths.'), - '#default_value' => $config->get('global_deslash'), - '#access' => FALSE, - ); - $form['globals']['global_admin_paths'] = array( - '#type' => 'checkbox', - '#title' => t('Allow redirections on admin paths.'), - '#default_value' => $config->get('global_admin_paths'), - ); $form['actions']['#type'] = 'actions'; $form['actions']['submit'] = array( '#type' => 'submit', @@ -695,11 +688,6 @@ function redirect_settings_form_submit($form, &$form_state) { $config->set('passthrough_querystring', (bool) $values['passthrough_querystring']); $config->set('additional_statuses', (bool) $values['additional_statuses']); $config->set('purge_inactive', (bool) $values['purge_inactive']); - $config->set('global_home', (bool) $values['global_home']); - $config->set('global_clean', (bool) $values['global_clean']); - $config->set('global_canonical', (bool) $values['global_canonical']); - $config->set('global_deslash', (bool) $values['global_deslash']); - $config->set('global_admin_paths', (bool) $values['global_admin_paths']); $config->save(); backdrop_set_message(t('The configuration options have been saved.')); } diff --git a/core/modules/redirect/redirect.install b/core/modules/redirect/redirect.install index f1345ed368d..6b7ba7a6671 100644 --- a/core/modules/redirect/redirect.install +++ b/core/modules/redirect/redirect.install @@ -40,7 +40,7 @@ function redirect_schema() { 'type' => 'varchar', 'length' => 255, 'not null' => TRUE, - 'description' => 'The source path to redirect from.', + 'description' => 'The source path from which to redirect.', ), 'source_options' => array( 'type' => 'text', @@ -52,7 +52,7 @@ function redirect_schema() { 'type' => 'varchar', 'length' => 255, 'not null' => TRUE, - 'description' => 'The destination path to redirect to.', + 'description' => 'The destination path to which the user will be redirected.', ), 'redirect_options' => array( 'type' => 'text', @@ -118,11 +118,6 @@ function redirect_update_1000() { $config->set('passthrough_querystring', update_variable_get('redirect_passthrough_querystring', TRUE)); $config->set('default_status_code', update_variable_get('redirect_default_status_code', '301')); $config->set('purge_inactive', update_variable_get('redirect_purge_inactive', FALSE)); - $config->set('global_home', update_variable_get('redirect_global_home', TRUE)); - $config->set('global_clean', update_variable_get('redirect_global_clean', TRUE)); - $config->set('global_canonical', update_variable_get('redirect_global_canonical', TRUE)); - $config->set('global_deslash', update_variable_get('redirect_global_deslash', FALSE)); - $config->set('global_admin_paths', update_variable_get('redirect_global_admin_paths', FALSE)); $config->save(); // Remove old variables. @@ -132,11 +127,6 @@ function redirect_update_1000() { update_variable_del('redirect_default_status_code'); update_variable_del('redirect_page_cache'); update_variable_del('redirect_purge_inactive'); - update_variable_del('redirect_global_home'); - update_variable_del('redirect_global_clean'); - update_variable_del('redirect_global_canonical'); - update_variable_del('redirect_global_deslash'); - update_variable_del('redirect_global_admin_paths'); } /** diff --git a/core/modules/redirect/redirect.module b/core/modules/redirect/redirect.module index 3f1ea495610..cd88095a9ab 100644 --- a/core/modules/redirect/redirect.module +++ b/core/modules/redirect/redirect.module @@ -131,7 +131,7 @@ function redirect_menu() { ); // Add an action link to the 404 page. - $site_404 = config_get('system.site_404'); + $site_404 = config_get('system.c'); if (empty($path)) { $site_404 = 'system/404'; } @@ -255,36 +255,20 @@ function redirect_init() { if ($redirect = redirect_get_current_redirect()) { redirect_redirect($redirect); } - - $redirect_global = FALSE; - $request_uri = $original_uri = ltrim(request_uri(), '/'); - - // Redirect from non-clean URLs to clean URLs. - if (config_get('redirect.settings', 'global_clean') && config_get('system.core', 'clean_url') && strpos($request_uri, '?q=') !== FALSE) { - //$redirect_global = TRUE; - //$request_uri = str_replace('?q=', '', $request_uri); - } - - if (strpos($request_uri, 'index.php') !== FALSE) { - //$redirect_global = TRUE; - //$request_uri = str_replace('index.php', '', $request_uri); - } - - //$request_uri = ltrim($request_uri, '/'); - //$parsed = parse_url($request_uri); - // TODO: gff figure out what this is trying to do, if it does it and if Not - // determine if should/can be fixed or removed. - if ($redirect_global && $request_uri != $original_uri) { - redirect_redirect(array(/*'redirect' => $request_uri,*/ 'type' => 'global')); - } } /** * Implements hook_cron(). */ function redirect_cron() { - // Purge inactive self-managed redirects from the database. - redirect_purge_inactive_redirects(); + // Throttle to only once every 6 hours. + $last_run = state_get('redirect_cron_last', 0); + $interval = 21600; + if ($last_run + $interval < REQUEST_TIME) { + // Purge inactive self-managed redirects from the database. + redirect_purge_inactive_redirects(); + state_set('redirect_cron_last', REQUEST_TIME); + } } /** @@ -576,7 +560,9 @@ function redirect_load_multiple(array $rids = array(), array $conditions = array $redirects = array(); if ($rids) { foreach ($rids as $rid) { - $redirects[$rid] = $cached_redirects[$rid]; + if (isset($cached_redirects[$rid])) { + $redirects[$rid] = $cached_redirects[$rid]; + } } } elseif ($conditions) { @@ -1033,7 +1019,7 @@ function redirect_page_cache_clear(Redirect $redirect) { * - If the PHP entry point is the root index.php file. * - If PHP is not running as CLI. * - If the site is not offline or in install/update mode. - * - If the curerent page is not an admin page (check can be disabled). + * - If the current page is not an admin page (check can be disabled). * - If the current request does not have any POST data since a redirect * may interrupt form submission. * @@ -1042,9 +1028,7 @@ function redirect_page_cache_clear(Redirect $redirect) { */ function redirect_can_redirect() { $can_redirect = &backdrop_static(__FUNCTION__); - $config = config('redirect.settings'); if (!isset($can_redirect)) { - $path = current_path(); $can_redirect = TRUE; if ($_SERVER['SCRIPT_NAME'] != $GLOBALS['base_path'] . 'index.php') { @@ -1063,10 +1047,6 @@ function redirect_can_redirect() { // Do not redirect in offline or maintenance mode. $can_redirect = FALSE; } - elseif (!$config->get('global_admin_paths') && path_is_admin($path)) { - // Do not redirect on admin paths. - $can_redirect = FALSE; - } } return $can_redirect; diff --git a/core/modules/redirect/tests/redirect.test b/core/modules/redirect/tests/redirect.test index 0cee646b5b2..f5c71458807 100644 --- a/core/modules/redirect/tests/redirect.test +++ b/core/modules/redirect/tests/redirect.test @@ -25,6 +25,7 @@ class RedirectTestHelper extends BackdropWebTestCase { if (!empty($redirect->rid)) { return redirect_load($redirect->rid, TRUE); } + return FALSE; } protected function assertNoRedirect($redirect) { @@ -34,12 +35,15 @@ class RedirectTestHelper extends BackdropWebTestCase { } /** - * Add an URL redirection + * Add an URL redirection. * * @param $source * A source path. * @param $redirect * A redirect path. + * + * @return Redirect + * A redirect object. */ protected function addRedirect($source_path, $redirect_path, array $redirect = array()) { $source_parsed = redirect_parse_url($source_path); @@ -96,7 +100,7 @@ class RedirectUnitTest extends BackdropUnitTestCase { $haystack = array('a' => 'aa', 'b' => 'bb', 'c' => array('c1' => 'cc1', 'c2' => 'cc2')); $cases = array( array('query' => array('a' => 'aa', 'b' => 'invalid'), 'result' => FALSE), - array('query' => array('b' => 'bb', 'b' => 'bb'), 'result' => TRUE), + array('query' => array('b' => 'bb'), 'result' => TRUE), array('query' => array('b' => 'bb', 'c' => 'invalid'), 'result' => FALSE), array('query' => array('b' => 'bb', 'c' => array()), 'result' => TRUE), array('query' => array('b' => 'bb', 'c' => array('invalid')), 'result' => FALSE), @@ -125,22 +129,6 @@ class RedirectUnitTest extends BackdropUnitTestCase { $this->assertIdentical($output, $test_case['expected']); } } - - /** - * Test redirect_parse_url(). - */ - function testParseURL() { - //$test_cases = array( - // array( - // 'input' => array('b' => 'aa', 'c' => array('c2' => 'aa', 'c1' => 'aa'), 'a' => 'aa'), - // 'expected' => array('a' => 'aa', 'b' => 'aa', 'c' => array('c1' => 'aa', 'c2' => 'aa')), - // ), - //); - //foreach ($test_cases as $index => $test_case) { - // $output = redirect_parse_url($test_case['input']); - // $this->assertIdentical($output, $test_case['expected']); - //} - } } class RedirectFunctionalTest extends RedirectTestHelper { diff --git a/core/modules/system/system.install b/core/modules/system/system.install index 497258a0266..c0af2c1dac6 100755 --- a/core/modules/system/system.install +++ b/core/modules/system/system.install @@ -532,10 +532,10 @@ function system_requirements($phase) { $path = backdrop_get_path('module', 'redirect'); if ($path && strpos($path, 'core/modules/redirect') === FALSE) { $requirements['redirect_replaced'] = array( - 'title' => $t('Link'), + 'title' => $t('Redirect'), 'value' => $t('Duplicate module detected'), 'severity' => REQUIREMENT_WARNING, - 'description' => $t('Backdrop core now provides a bundled Redirect module. A different copy of Redirect module has been located at %path. Remove this module from your installation to use the new Redirect module. For more information see the Redirect change notice.', array('%path' => BACKDROP_ROOT . '/' . $path)), + 'description' => $t('Backdrop core now provides a bundled Redirect module. A different copy of Redirect module has been located at %path. Remove this module from your installation to use the new Redirect module. For more information see the Redirect change notice.', array('%path' => BACKDROP_ROOT . '/' . $path)), ); } } @@ -2738,7 +2738,7 @@ function system_update_1050() { function system_update_1051() { $path = backdrop_get_path('module', 'redirect'); if ($path && strpos($path, 'core/modules/redirect') === FALSE) { - backdrop_set_message(t('Backdrop core now provides a bundled Redirect module. A different copy of Redirect module has been located at %path. To use the new Redirect module, disable and then uninstall the current module, remove it from your installation, and then enable the new Redirect module from the modules page. For more information see the Redirect change notice.', array('%path' => BACKDROP_ROOT . '/' . $path)), 'warning'); + backdrop_set_message(t('Backdrop core now provides a bundled Redirect module. A different copy of Redirect module has been located at %path. Remove this module from your installation to use the new Redirect module. For more information see the Redirect change notice.', array('%path' => BACKDROP_ROOT . '/' . $path)), 'warning'); } }