diff --git a/core/core.api.php b/core/core.api.php index c2750cadf172..63fd607ba7f1 100644 --- a/core/core.api.php +++ b/core/core.api.php @@ -2054,12 +2054,6 @@ function hook_validation_constraint_alter(array &$definitions) { * an array. Here are the details of its elements, all of which are optional: * - callback: The callback to invoke to handle the server side of the * Ajax event. More information on callbacks is below in @ref sub_callback. - * - path: The URL path to use for the request. If omitted, defaults to - * 'system/ajax', which invokes the default Drupal Ajax processing (this will - * call the callback supplied in the 'callback' element). If you supply a - * path, you must set up a routing entry to handle the request yourself and - * return output described in @ref sub_callback below. See the - * @link menu Routing topic @endlink for more information on routing. * - wrapper: The HTML 'id' attribute of the area where the content returned by * the callback should be placed. Note that callbacks have a choice of * returning content or JavaScript commands; 'wrapper' is used for content @@ -2085,6 +2079,13 @@ function hook_validation_constraint_alter(array &$definitions) { * - message: Translated message to display. * - url: For a bar progress indicator, URL path for determining progress. * - interval: For a bar progress indicator, how often to update it. + * - url: A \Drupal\Core\Url to which to submit the Ajax request. If omitted, + * defaults to either the same URL as the form or link destination is for + * someone with JavaScript disabled, or a slightly modified version (e.g., + * with a query parameter added, removed, or changed) of that URL if + * necessary to support Drupal's content negotiation. It is recommended to + * omit this key and use Drupal's content negotiation rather than using + * substantially different URLs between Ajax and non-Ajax. * * @subsection sub_callback Setting up a callback to process Ajax * Once you have set up your form to trigger an Ajax response (see @ref sub_form diff --git a/core/lib/Drupal/Core/Form/FormBuilder.php b/core/lib/Drupal/Core/Form/FormBuilder.php index d9668014ec95..64e01e61276e 100644 --- a/core/lib/Drupal/Core/Form/FormBuilder.php +++ b/core/lib/Drupal/Core/Form/FormBuilder.php @@ -185,9 +185,22 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS // Ensure the form ID is prepared. $form_id = $this->getFormId($form_id, $form_state); + $request = $this->requestStack->getCurrentRequest(); + + // Inform $form_state about the request method that's building it, so that + // it can prevent persisting state changes during HTTP methods for which + // that is disallowed by HTTP: GET and HEAD. + $form_state->setRequestMethod($request->getMethod()); + + // Initialize the form's user input. The user input should include only the + // input meant to be treated as part of what is submitted to the form, so + // we base it on the form's method rather than the request's method. For + // example, when someone does a GET request for + // /node/add/article?destination=foo, which is a form that expects its + // submission method to be POST, the user input during the GET request + // should be initialized to empty rather than to ['destination' => 'foo']. $input = $form_state->getUserInput(); if (!isset($input)) { - $request = $this->requestStack->getCurrentRequest(); $input = $form_state->isMethodType('get') ? $request->query->all() : $request->request->all(); $form_state->setUserInput($input); } @@ -313,8 +326,22 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS */ public function rebuildForm($form_id, FormStateInterface &$form_state, $old_form = NULL) { $form = $this->retrieveForm($form_id, $form_state); - // All rebuilt forms will be cached. - $form_state->setCached(); + + // Only GET and POST are valid form methods. If the form receives its input + // via POST, then $form_state must be persisted when it is rebuilt between + // submissions. If the form receives its input via GET, then persisting + // state is forbidden by $form_state->setCached(), and the form must use + // the URL itself to transfer its state across steps. Although $form_state + // throws an exception based on the request method rather than the form's + // method, we base the decision to cache on the form method, because: + // - It's the form method that defines what the form needs to do to manage + // its state. + // - rebuildForm() should only be called after successful input processing, + // which means the request method matches the form method, and if not, + // there's some other error, so it's ok if an exception is thrown. + if ($form_state->isMethodType('POST')) { + $form_state->setCached(); + } // If only parts of the form will be returned to the browser (e.g., Ajax or // RIA clients), or if the form already had a new build ID regenerated when diff --git a/core/lib/Drupal/Core/Form/FormBuilderInterface.php b/core/lib/Drupal/Core/Form/FormBuilderInterface.php index 60ad704832f7..bbfed36b985a 100644 --- a/core/lib/Drupal/Core/Form/FormBuilderInterface.php +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php @@ -108,9 +108,7 @@ interface FormBuilderInterface { * form workflow, to be returned for rendering. * * Ajax form submissions are almost always multi-step workflows, so that is - * one common use-case during which form rebuilding occurs. See - * Drupal\system\FormAjaxController::content() for more information about - * creating Ajax-enabled forms. + * one common use-case during which form rebuilding occurs. * * @param string $form_id * The unique string identifying the desired form. If a function with that @@ -130,7 +128,6 @@ interface FormBuilderInterface { * The newly built form. * * @see self::processForm() - * @see \Drupal\system\FormAjaxController::content() */ public function rebuildForm($form_id, FormStateInterface &$form_state, $old_form = NULL); diff --git a/core/lib/Drupal/Core/Form/FormState.php b/core/lib/Drupal/Core/Form/FormState.php index 4a17d6b22588..9cb0b0da2605 100644 --- a/core/lib/Drupal/Core/Form/FormState.php +++ b/core/lib/Drupal/Core/Form/FormState.php @@ -141,16 +141,29 @@ class FormState implements FormStateInterface { /** * The HTTP form method to use for finding the input for this form. * - * May be 'post' or 'get'. Defaults to 'post'. Note that 'get' method forms do + * May be 'POST' or 'GET'. Defaults to 'POST'. Note that 'GET' method forms do * not use form ids so are always considered to be submitted, which can have - * unexpected effects. The 'get' method should only be used on forms that do - * not change data, as that is exclusively the domain of 'post.' + * unexpected effects. The 'GET' method should only be used on forms that do + * not change data, as that is exclusively the domain of 'POST.' * * This property is uncacheable. * * @var string */ - protected $method = 'post'; + protected $method = 'POST'; + + /** + * The HTTP method used by the request building or processing this form. + * + * May be any valid HTTP method. Defaults to 'GET', because even though + * $method is 'POST' for most forms, the form's initial build is usually + * performed as part of a GET request. + * + * This property is uncacheable. + * + * @var string + */ + protected $requestMethod = 'GET'; /** * If set to TRUE the original, unprocessed form structure will be cached, @@ -475,6 +488,12 @@ class FormState implements FormStateInterface { * {@inheritdoc} */ public function setCached($cache = TRUE) { + // Persisting $form_state is a side-effect disallowed during a "safe" HTTP + // method. + if ($cache && $this->isRequestMethodSafe()) { + throw new \LogicException(sprintf('Form state caching on %s requests is not allowed.', $this->requestMethod)); + } + $this->cache = (bool) $cache; return $this; } @@ -569,6 +588,29 @@ class FormState implements FormStateInterface { return $this->method === strtoupper($method_type); } + /** + * {@inheritdoc} + */ + public function setRequestMethod($method) { + $this->requestMethod = strtoupper($method); + return $this; + } + + /** + * Checks whether the request method is a "safe" HTTP method. + * + * http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.1.1 defines + * GET and HEAD as "safe" methods, meaning they SHOULD NOT have side-effects, + * such as persisting $form_state changes. + * + * @return bool + * + * @see \Symfony\Component\HttpFoundation\Request::isMethodSafe() + */ + protected function isRequestMethodSafe() { + return in_array($this->requestMethod, array('GET', 'HEAD')); + } + /** * {@inheritdoc} */ diff --git a/core/lib/Drupal/Core/Form/FormStateInterface.php b/core/lib/Drupal/Core/Form/FormStateInterface.php index e0ca1c6a7aa2..fd46f70fed3c 100644 --- a/core/lib/Drupal/Core/Form/FormStateInterface.php +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php @@ -640,6 +640,10 @@ interface FormStateInterface { * TRUE if the form should be cached, FALSE otherwise. * * @return $this + * + * @throws \LogicException + * If the current request is using an HTTP method that must not change + * state (e.g., GET). */ public function setCached($cache = TRUE); @@ -731,17 +735,36 @@ interface FormStateInterface { public function getLimitValidationErrors(); /** - * Sets the HTTP form method. + * Sets the HTTP method to use for the form's submission. + * + * This is what the form's "method" attribute should be, not necessarily what + * the current request's HTTP method is. For example, a form can have a + * method attribute of POST, but the request that initially builds it uses + * GET. * * @param string $method - * The HTTP form method. + * Either "GET" or "POST". Other HTTP methods are not valid form submission + * methods. * * @see \Drupal\Core\Form\FormState::$method + * @see \Drupal\Core\Form\FormStateInterface::setRequestMethod() * * @return $this */ public function setMethod($method); + /** + * Sets the HTTP method used by the request that is building the form. + * + * @param string $method + * Can be any valid HTTP method, such as GET, POST, HEAD, etc. + * + * @return $this + * + * @see \Drupal\Core\Form\FormStateInterface::setMethod() + */ + public function setRequestMethod($method); + /** * Returns the HTTP form method. * diff --git a/core/lib/Drupal/Core/Render/Element/RenderElement.php b/core/lib/Drupal/Core/Render/Element/RenderElement.php index a3b362cbdcc6..5e7568b7a8a7 100644 --- a/core/lib/Drupal/Core/Render/Element/RenderElement.php +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php @@ -128,14 +128,7 @@ abstract class RenderElement extends PluginBase implements ElementInterface { * @see self::preRenderAjaxForm() */ public static function processAjaxForm(&$element, FormStateInterface $form_state, &$complete_form) { - $element = static::preRenderAjaxForm($element); - - // If the element was processed as an #ajax element, and a custom URL was - // provided, set the form to be cached. - if (!empty($element['#ajax_processed']) && !empty($element['#ajax']['url'])) { - $form_state->setCached(); - } - return $element; + return static::preRenderAjaxForm($element); } /** @@ -238,10 +231,6 @@ abstract class RenderElement extends PluginBase implements ElementInterface { // content negotiation takes care of formatting the response appropriately. // However, 'url' and 'options' may be set when wanting server processing // to be substantially different for a JavaScript triggered submission. - // One such substantial difference is form elements that use - // #ajax['callback'] for determining which part of the form needs - // re-rendering. For that, we have a special 'system.ajax' route which - // must be manually set. $settings += [ 'url' => NULL, 'options' => ['query' => []], diff --git a/core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php b/core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php index b15ce93bef7b..9d904ee2d00e 100644 --- a/core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php +++ b/core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php @@ -15,12 +15,12 @@ use Symfony\Component\HttpFoundation\RequestStack; /** * Defines a theme negotiator that deals with the active theme on ajax requests. * - * Many different pages can invoke an Ajax request to system/ajax or another - * generic Ajax path. It is almost always desired for an Ajax response to be - * rendered using the same theme as the base page, because most themes are built - * with the assumption that they control the entire page, so if the CSS for two - * themes are both loaded for a given page, they may conflict with each other. - * For example, Bartik is Drupal's default theme, and Seven is Drupal's default + * Many different pages can invoke an Ajax request to a generic Ajax path. It is + * almost always desired for an Ajax response to be rendered using the same + * theme as the base page, because most themes are built with the assumption + * that they control the entire page, so if the CSS for two themes are both + * loaded for a given page, they may conflict with each other. For example, + * Bartik is Drupal's default theme, and Seven is Drupal's default * administration theme. Depending on whether the "Use the administration theme * when editing or creating content" checkbox is checked, the node edit form may * be displayed in either theme, but the Ajax response to the Field module's diff --git a/core/modules/dblog/src/Tests/DbLogFormInjectionTest.php b/core/modules/dblog/src/Tests/DbLogFormInjectionTest.php index 6957f517dc74..62fa23455b94 100644 --- a/core/modules/dblog/src/Tests/DbLogFormInjectionTest.php +++ b/core/modules/dblog/src/Tests/DbLogFormInjectionTest.php @@ -99,6 +99,9 @@ class DbLogFormInjectionTest extends KernelTestBase implements FormInterface { */ public function testLoggerSerialization() { $form_state = new FormState(); + + // Forms are only serialized during POST requests. + $form_state->setRequestMethod('POST'); $form_state->setCached(); $form_builder = $this->container->get('form_builder'); $form_id = $form_builder->getFormId($this, $form_state); diff --git a/core/modules/file/src/Controller/FileWidgetAjaxController.php b/core/modules/file/src/Controller/FileWidgetAjaxController.php index 1c1b7e3a7857..d05a4dd7bb6d 100644 --- a/core/modules/file/src/Controller/FileWidgetAjaxController.php +++ b/core/modules/file/src/Controller/FileWidgetAjaxController.php @@ -7,13 +7,12 @@ namespace Drupal\file\Controller; -use Drupal\system\Controller\FormAjaxController; use Symfony\Component\HttpFoundation\JsonResponse; /** * Defines a controller to respond to file widget AJAX requests. */ -class FileWidgetAjaxController extends FormAjaxController { +class FileWidgetAjaxController { /** * Returns the progress status for a file upload process. diff --git a/core/modules/simpletest/src/WebTestBase.php b/core/modules/simpletest/src/WebTestBase.php index c43888bcc6c3..0ea88ba4465e 100644 --- a/core/modules/simpletest/src/WebTestBase.php +++ b/core/modules/simpletest/src/WebTestBase.php @@ -1591,17 +1591,14 @@ abstract class WebTestBase extends TestBase { * * This function can also be called to emulate an Ajax submission. In this * case, this value needs to be an array with the following keys: - * - path: A path to submit the form values to for Ajax-specific processing, - * which is likely different than the $path parameter used for retrieving - * the initial form. Defaults to 'system/ajax'. - * - triggering_element: If the value for the 'path' key is 'system/ajax' or - * another generic Ajax processing path, this needs to be set to the name - * of the element. If the name doesn't identify the element uniquely, then - * this should instead be an array with a single key/value pair, - * corresponding to the element name and value. The callback for the - * generic Ajax processing path uses this to find the #ajax information - * for the element, including which specific callback to use for - * processing the request. + * - path: A path to submit the form values to for Ajax-specific processing. + * - triggering_element: If the value for the 'path' key is a generic Ajax + * processing path, this needs to be set to the name of the element. If + * the name doesn't identify the element uniquely, then this should + * instead be an array with a single key/value pair, corresponding to the + * element name and value. The \Drupal\Core\Form\FormAjaxResponseBuilder + * uses this to find the #ajax information for the element, including + * which specific callback to use for processing the request. * * This can also be set to NULL in order to emulate an Internet Explorer * submission of a form with a single text field, and pressing ENTER in that @@ -1649,7 +1646,10 @@ abstract class WebTestBase extends TestBase { $submit_matches = $this->handleForm($post, $edit, $upload, $ajax ? NULL : $submit, $form); $action = isset($form['action']) ? $this->getAbsoluteUrl((string) $form['action']) : $this->getUrl(); if ($ajax) { - $action = $this->getAbsoluteUrl(!empty($submit['path']) ? $submit['path'] : 'system/ajax'); + if (empty($submit['path'])) { + throw new \Exception('No #ajax path specified.'); + } + $action = $this->getAbsoluteUrl($submit['path']); // Ajax callbacks verify the triggering element if necessary, so while // we may eventually want extra code that verifies it in the // handleForm() function, it's not currently a requirement. @@ -1735,8 +1735,7 @@ abstract class WebTestBase extends TestBase { * and the value is the button label. i.e.) array('op' => t('Refresh')). * @param $ajax_path * (optional) Override the path set by the Ajax settings of the triggering - * element. In the absence of both the triggering element's Ajax path and - * $ajax_path 'system/ajax' will be used. + * element. * @param $options * (optional) Options to be forwarded to the url generator. * @param $headers @@ -1807,7 +1806,7 @@ abstract class WebTestBase extends TestBase { $extra_post = '&' . $this->serializePostValues($extra_post); // Unless a particular path is specified, use the one specified by the - // Ajax settings, or else 'system/ajax'. + // Ajax settings. if (!isset($ajax_path)) { if (isset($ajax_settings['url'])) { // In order to allow to set for example the wrapper envelope query @@ -1824,10 +1823,12 @@ abstract class WebTestBase extends TestBase { $parsed_url['path'] ); } - else { - $ajax_path = 'system/ajax'; - } } + + if (empty($ajax_path)) { + throw new \Exception('No #ajax path specified.'); + } + $ajax_path = $this->container->get('unrouted_url_assembler')->assemble('base://' . $ajax_path, $options); // Submit the POST request. diff --git a/core/modules/system/src/Controller/FormAjaxController.php b/core/modules/system/src/Controller/FormAjaxController.php deleted file mode 100644 index 084847a81487..000000000000 --- a/core/modules/system/src/Controller/FormAjaxController.php +++ /dev/null @@ -1,186 +0,0 @@ -logger = $logger; - $this->formBuilder = $form_builder; - $this->renderer = $renderer; - $this->ajaxRenderer = $ajax_renderer; - $this->routeMatch = $route_match; - $this->formAjaxResponseBuilder = $form_ajax_response_builder; - } - - /** - * {@inheritdoc} - */ - public static function create(ContainerInterface $container) { - return new static( - $container->get('logger.factory')->get('ajax'), - $container->get('form_builder'), - $container->get('renderer'), - $container->get('main_content_renderer.ajax'), - $container->get('current_route_match'), - $container->get('form_ajax_response_builder') - ); - } - - /** - * Processes an Ajax form submission. - * - * @param \Symfony\Component\HttpFoundation\Request $request - * The current request object. - * - * @return mixed - * Whatever is returned by the triggering element's #ajax['callback'] - * function. One of: - * - A render array containing the new or updated content to return to the - * browser. This is commonly an element within the rebuilt form. - * - A \Drupal\Core\Ajax\AjaxResponse object containing commands for the - * browser to process. - * - * @throws \Symfony\Component\HttpKernel\Exception\HttpExceptionInterface - */ - public function content(Request $request) { - $ajax_form = $this->getForm($request); - $form = $ajax_form->getForm(); - $form_state = $ajax_form->getFormState(); - $commands = $ajax_form->getCommands(); - - $this->formBuilder->processForm($form['#form_id'], $form, $form_state); - - return $this->formAjaxResponseBuilder->buildResponse($request, $form, $form_state, $commands); - } - - /** - * Gets a form submitted via #ajax during an Ajax callback. - * - * This will load a form from the form cache used during Ajax operations. It - * pulls the form info from the request body. - * - * @param \Symfony\Component\HttpFoundation\Request $request - * The current request object. - * - * @return \Drupal\system\FileAjaxForm - * A wrapper object containing the $form, $form_state, $form_id, - * $form_build_id and an initial list of Ajax $commands. - * - * @throws \Symfony\Component\HttpKernel\Exception\HttpExceptionInterface - */ - protected function getForm(Request $request) { - $form_state = new FormState(); - $form_build_id = $request->request->get('form_build_id'); - - // Get the form from the cache. - $form = $this->formBuilder->getCache($form_build_id, $form_state); - if (!$form) { - // If $form cannot be loaded from the cache, the form_build_id must be - // invalid, which means that someone performed a POST request onto - // system/ajax without actually viewing the concerned form in the browser. - // This is likely a hacking attempt as it never happens under normal - // circumstances. - $this->logger->warning('Invalid form POST data.'); - throw new BadRequestHttpException(); - } - - // Since some of the submit handlers are run, redirects need to be disabled. - $form_state->disableRedirect(); - - // When a form is rebuilt after Ajax processing, its #build_id and #action - // should not change. - // @see \Drupal\Core\Form\FormBuilderInterface::rebuildForm() - $form_state->addRebuildInfo('copy', [ - '#build_id' => TRUE, - '#action' => TRUE, - ]); - - // The form needs to be processed; prepare for that by setting a few - // internal variables. - $form_state->setUserInput($request->request->all()); - $form_id = $form['#form_id']; - - return new FileAjaxForm($form, $form_state, $form_id, $form['#build_id'], []); - } - -} diff --git a/core/modules/system/src/Tests/Ajax/AjaxFormCacheTest.php b/core/modules/system/src/Tests/Ajax/AjaxFormCacheTest.php index f151c33a7e7e..4ebeed66105a 100644 --- a/core/modules/system/src/Tests/Ajax/AjaxFormCacheTest.php +++ b/core/modules/system/src/Tests/Ajax/AjaxFormCacheTest.php @@ -37,15 +37,6 @@ class AjaxFormCacheTest extends AjaxTestBase { // The number of cache entries should not have changed. $this->assertEqual(0, count($key_value_expirable->getAll())); - - // Visit a form that is explicitly cached, 3 times. - $cached_form_url = Url::fromRoute('ajax_forms_test.cached_form'); - $this->drupalGet($cached_form_url); - $this->drupalGet($cached_form_url); - $this->drupalGet($cached_form_url); - - // The number of cache entries should be exactly 3. - $this->assertEqual(3, count($key_value_expirable->getAll())); } /** diff --git a/core/modules/system/src/Tests/Ajax/AjaxFormPageCacheTest.php b/core/modules/system/src/Tests/Ajax/AjaxFormPageCacheTest.php index 03727b699a39..41132638b0ea 100644 --- a/core/modules/system/src/Tests/Ajax/AjaxFormPageCacheTest.php +++ b/core/modules/system/src/Tests/Ajax/AjaxFormPageCacheTest.php @@ -35,7 +35,7 @@ class AjaxFormPageCacheTest extends AjaxTestBase { } /** - * Create a simple form, then POST to system/ajax to change to it. + * Create a simple form, then submit the form via AJAX to change to it. */ public function testSimpleAJAXFormValue() { $this->drupalGet('ajax_forms_test_get_form'); diff --git a/core/modules/system/src/Tests/Form/FormStoragePageCacheTest.php b/core/modules/system/src/Tests/Form/FormStoragePageCacheTest.php index 5fa8c34bec1d..5cad67fd0255 100644 --- a/core/modules/system/src/Tests/Form/FormStoragePageCacheTest.php +++ b/core/modules/system/src/Tests/Form/FormStoragePageCacheTest.php @@ -53,7 +53,7 @@ class FormStoragePageCacheTest extends WebTestBase { // Trigger validation error by submitting an empty title. $edit = ['title' => '']; $this->drupalPostForm(NULL, $edit, 'Save'); - $this->assertText($build_id_initial, 'Old build id on the page'); + $this->assertText('No old build id', 'No old build id on the page'); $build_id_first_validation = $this->getFormBuildId(); $this->assertNotEqual($build_id_initial, $build_id_first_validation, 'Build id changes when form validation fails'); @@ -74,7 +74,7 @@ class FormStoragePageCacheTest extends WebTestBase { // Trigger validation error by submitting an empty title. $edit = ['title' => '']; $this->drupalPostForm(NULL, $edit, 'Save'); - $this->assertText($build_id_initial, 'Old build id is initial build id'); + $this->assertText('No old build id', 'No old build id on the page'); $build_id_from_cache_first_validation = $this->getFormBuildId(); $this->assertNotEqual($build_id_initial, $build_id_from_cache_first_validation, 'Build id changes when form validation fails'); $this->assertNotEqual($build_id_first_validation, $build_id_from_cache_first_validation, 'Build id from first user is not reused'); @@ -96,10 +96,15 @@ class FormStoragePageCacheTest extends WebTestBase { $this->assertText('No old build id', 'No old build id on the page'); $build_id_initial = $this->getFormBuildId(); - // Trigger rebuild, should regenerate build id. + // Trigger rebuild, should regenerate build id. When a submit handler + // triggers a rebuild, the form is built twice in the same POST request, + // and during the second build, there is an old build ID, but because the + // form is not cached during the initial GET request, it is different from + // that initial build ID. $edit = ['title' => 'something']; $this->drupalPostForm(NULL, $edit, 'Rebuild'); - $this->assertText($build_id_initial, 'Initial build id as old build id on the page'); + $this->assertNoText('No old build id', 'There is no old build id on the page.'); + $this->assertNoText($build_id_initial, 'The old build id is not the initial build id.'); $build_id_first_rebuild = $this->getFormBuildId(); $this->assertNotEqual($build_id_initial, $build_id_first_rebuild, 'Build id changes on first rebuild.'); diff --git a/core/modules/system/src/Tests/Form/StorageTest.php b/core/modules/system/src/Tests/Form/StorageTest.php index 8f3873a7f980..831ba25577da 100644 --- a/core/modules/system/src/Tests/Form/StorageTest.php +++ b/core/modules/system/src/Tests/Form/StorageTest.php @@ -73,16 +73,19 @@ class StorageTest extends WebTestBase { // Use form rebuilding triggered by a submit button. $this->drupalPostForm(NULL, $edit, 'Continue submit'); + // The first one is for the building of the form. $this->assertText('Form constructions: 2'); + // The second one is for the rebuilding of the form. + $this->assertText('Form constructions: 3'); // Reset the form to the values of the storage, using a form rebuild // triggered by button of type button. $this->drupalPostForm(NULL, array('title' => 'changed'), 'Reset'); $this->assertFieldByName('title', 'new', 'Values have been reset.'); - $this->assertText('Form constructions: 3'); + $this->assertText('Form constructions: 4'); $this->drupalPostForm(NULL, $edit, 'Save'); - $this->assertText('Form constructions: 3'); + $this->assertText('Form constructions: 4'); $this->assertText('Title: new', 'The form storage has stored the values.'); } @@ -129,55 +132,6 @@ class StorageTest extends WebTestBase { $this->assertText("The thing has been changed.", 'The altered form storage value was updated in cache and taken over.'); } - /** - * Tests a form using form state without using 'storage' to pass data from the - * constructor to a submit handler. The data has to persist even when caching - * gets activated, what may happen when a modules alter the form and adds - * #ajax properties. - */ - function testFormStatePersist() { - // Test the form one time with caching activated and one time without. - $run_options = array( - array(), - array('query' => array('cache' => 1)), - ); - foreach ($run_options as $options) { - $this->drupalPostForm('form-test/state-persist', array(), t('Submit'), $options); - // The submit handler outputs the value in $form_state, assert it's there. - $this->assertText('State persisted.'); - - // Test it again, but first trigger a validation error, then test. - $this->drupalPostForm('form-test/state-persist', array('title' => ''), t('Submit'), $options); - $this->assertText(t('!name field is required.', array('!name' => 'title'))); - // Submit the form again triggering no validation error. - $this->drupalPostForm(NULL, array('title' => 'foo'), t('Submit'), $options); - $this->assertText('State persisted.'); - - // Now post to the rebuilt form and verify it's still there afterwards. - $this->drupalPostForm(NULL, array('title' => 'bar'), t('Submit'), $options); - $this->assertText('State persisted.'); - } - } - - /** - * Verify that the form build-id remains the same when validation errors - * occur on a mutable form. - */ - public function testMutableForm() { - // Request the form with 'cache' query parameter to enable form caching. - $this->drupalGet('form_test/form-storage', ['query' => ['cache' => 1]]); - $buildIdFields = $this->xpath('//input[@name="form_build_id"]'); - $this->assertEqual(count($buildIdFields), 1, 'One form build id field on the page'); - $buildId = (string) $buildIdFields[0]['value']; - - // Trigger validation error by submitting an empty title. - $edit = ['title' => '']; - $this->drupalPostForm(NULL, $edit, 'Continue submit'); - - // Verify that the build-id did not change. - $this->assertFieldByName('form_build_id', $buildId, 'Build id remains the same when form validation fails'); - } - /** * Verifies that form build-id is regenerated when loading an immutable form * from the cache. diff --git a/core/modules/system/src/Tests/Queue/QueueSerializationTest.php b/core/modules/system/src/Tests/Queue/QueueSerializationTest.php index 9108ac228e41..638b3f58c70d 100644 --- a/core/modules/system/src/Tests/Queue/QueueSerializationTest.php +++ b/core/modules/system/src/Tests/Queue/QueueSerializationTest.php @@ -97,6 +97,7 @@ class QueueSerializationTest extends KernelTestBase implements FormInterface { */ public function testQueueSerialization() { $form_state = new FormState(); + $form_state->setRequestMethod('POST'); $form_state->setCached(); $form_builder = $this->container->get('form_builder'); $form_id = $form_builder->getFormId($this, $form_state); diff --git a/core/modules/system/system.routing.yml b/core/modules/system/system.routing.yml index 12e7d9accdc4..a386196c34cd 100644 --- a/core/modules/system/system.routing.yml +++ b/core/modules/system/system.routing.yml @@ -1,12 +1,3 @@ -system.ajax: - path: '/system/ajax' - defaults: - _controller: '\Drupal\system\Controller\FormAjaxController::content' - options: - _theme: ajax_base_page - requirements: - _access: 'TRUE' - system.401: path: '/system/401' defaults: diff --git a/core/modules/system/tests/modules/ajax_forms_test/ajax_forms_test.routing.yml b/core/modules/system/tests/modules/ajax_forms_test/ajax_forms_test.routing.yml index 3caf5bae2552..ccca279b6930 100644 --- a/core/modules/system/tests/modules/ajax_forms_test/ajax_forms_test.routing.yml +++ b/core/modules/system/tests/modules/ajax_forms_test/ajax_forms_test.routing.yml @@ -30,10 +30,3 @@ ajax_forms_test.lazy_load_form: requirements: _access: 'TRUE' -ajax_forms_test.cached_form: - path: '/ajax_forms_test_cached_form' - defaults: - _title: 'AJAX forms cached form test' - _form: '\Drupal\ajax_forms_test\Form\AjaxFormsTestCachedForm' - requirements: - _access: 'TRUE' diff --git a/core/modules/system/tests/modules/ajax_forms_test/src/Form/AjaxFormsTestCachedForm.php b/core/modules/system/tests/modules/ajax_forms_test/src/Form/AjaxFormsTestCachedForm.php deleted file mode 100644 index 3b227832bfc5..000000000000 --- a/core/modules/system/tests/modules/ajax_forms_test/src/Form/AjaxFormsTestCachedForm.php +++ /dev/null @@ -1,50 +0,0 @@ - 'select', - '#title' => $this->t('Test 1'), - '#options' => [ - 'option1' => $this->t('Option 1'), - 'option2' => $this->t('Option 2'), - ], - '#ajax' => [ - 'url' => Url::fromRoute('system.ajax'), - ], - ]; - return $form; - } - - /** - * {@inheritdoc} - */ - public function submitForm(array &$form, FormStateInterface $form_state) { - } - -} diff --git a/core/modules/system/tests/modules/ajax_forms_test/src/Form/AjaxFormsTestSimpleForm.php b/core/modules/system/tests/modules/ajax_forms_test/src/Form/AjaxFormsTestSimpleForm.php index 225664de7728..2fcfd1f334af 100644 --- a/core/modules/system/tests/modules/ajax_forms_test/src/Form/AjaxFormsTestSimpleForm.php +++ b/core/modules/system/tests/modules/ajax_forms_test/src/Form/AjaxFormsTestSimpleForm.php @@ -57,7 +57,7 @@ class AjaxFormsTestSimpleForm extends FormBase { ); // This is for testing invalid callbacks that should return a 500 error in - // \Drupal\system\FormAjaxController::content(). + // \Drupal\Core\Form\FormAjaxResponseBuilderInterface::buildResponse(). $invalid_callbacks = array( 'null' => NULL, 'empty' => '', diff --git a/core/modules/system/tests/modules/form_test/form_test.module b/core/modules/system/tests/modules/form_test/form_test.module index d2979457ca52..2a970a22c77e 100644 --- a/core/modules/system/tests/modules/form_test/form_test.module +++ b/core/modules/system/tests/modules/form_test/form_test.module @@ -75,19 +75,6 @@ function _form_test_tableselect_get_data() { return array($header, $options); } -/** - * Implements hook_form_FORM_ID_alter(). - * - * @see form_test_state_persist() - */ -function form_test_form_form_test_state_persist_alter(&$form, FormStateInterface $form_state) { - // Simulate a form alter implementation inserting form elements that enable - // caching of the form, e.g. elements having #ajax. - if (\Drupal::request()->get('cache')) { - $form_state->setCached(); - } -} - /** * Implements hook_form_FORM_ID_alter() for the registration form. */ diff --git a/core/modules/system/tests/modules/form_test/src/Form/FormTestStorageForm.php b/core/modules/system/tests/modules/form_test/src/Form/FormTestStorageForm.php index 9510ef4f7931..7f6255772fc7 100644 --- a/core/modules/system/tests/modules/form_test/src/Form/FormTestStorageForm.php +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestStorageForm.php @@ -82,17 +82,33 @@ class FormTestStorageForm extends FormBase { '#value' => 'Save', ); - if (\Drupal::request()->get('cache')) { + // @todo Remove this in https://www.drupal.org/node/2524408, because form + // cache immutability is no longer necessary, because we no longer cache + // forms during safe HTTP methods. In the meantime, because + // Drupal\system\Tests\Form still has test coverage for a poisoned form + // cache following a GET request, trick $form_state into caching the form + // to keep that test working until we either remove it or change it in + // that issue. + if ($this->getRequest()->get('immutable')) { + $form_state->addBuildInfo('immutable', TRUE); + if ($this->getRequest()->get('cache') && $this->getRequest()->isMethodSafe()) { + $form_state->setRequestMethod('FAKE'); + $form_state->setCached(); + } + } + + return $form; + } + + /** + * {@inheritdoc} + */ + public function validateForm(array &$form, FormStateInterface $form_state) { + if ($this->getRequest()->get('cache')) { // Manually activate caching, so we can test that the storage keeps working // when it's enabled. $form_state->setCached(); } - - if ($this->getRequest()->get('immutable')) { - $form_state->addBuildInfo('immutable', TRUE); - } - - return $form; } /** @@ -105,7 +121,7 @@ class FormTestStorageForm extends FormBase { // This presumes that another submitted form value triggers a validation error // elsewhere in the form. Form API should still update the cached form storage // though. - if (\Drupal::request()->get('cache') && $form_state->getValue('value') == 'change_title') { + if ($this->getRequest()->get('cache') && $form_state->getValue('value') == 'change_title') { $form_state->set(['thing', 'changed'], TRUE); } } diff --git a/core/modules/system/tests/modules/form_test/src/Form/FormTestStoragePageCacheForm.php b/core/modules/system/tests/modules/form_test/src/Form/FormTestStoragePageCacheForm.php index e8eef2272dc1..9f3ab5aeb300 100644 --- a/core/modules/system/tests/modules/form_test/src/Form/FormTestStoragePageCacheForm.php +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestStoragePageCacheForm.php @@ -48,7 +48,6 @@ class FormTestStoragePageCacheForm extends FormBase { ); $form['#after_build'] = array(array($this, 'form_test_storage_page_cache_old_build_id')); - $form_state->setCached(); return $form; } @@ -70,6 +69,17 @@ class FormTestStoragePageCacheForm extends FormBase { $form_state->setRebuild(); } + /** + * {@inheritdoc} + */ + public function validateForm(array &$form, FormStateInterface $form_state) { + // Test using form cache when re-displaying a form due to validation + // errors. + if ($form_state->hasAnyErrors()) { + $form_state->setCached(); + } + } + /** * {@inheritdoc} */ diff --git a/core/modules/system/tests/modules/form_test/src/Form/FormTestValidateForm.php b/core/modules/system/tests/modules/form_test/src/Form/FormTestValidateForm.php index e403887ee617..100e4f85f3e4 100644 --- a/core/modules/system/tests/modules/form_test/src/Form/FormTestValidateForm.php +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestValidateForm.php @@ -49,10 +49,6 @@ class FormTestValidateForm extends FormBase { '#value' => 'Save', ); - // To simplify this test, enable form caching and use form storage to - // remember our alteration. - $form_state->setCached(); - return $form; } @@ -70,6 +66,10 @@ class FormTestValidateForm extends FormBase { // Trigger a form validation error to see our changes. $form_state->setErrorByName(''); + + // To simplify this test, enable form caching and use form storage to + // remember our alteration. + $form_state->setCached(); } } diff --git a/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php index 9e5364528b73..09445ce33fd4 100644 --- a/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php @@ -283,7 +283,7 @@ class FormBuilderTest extends FormTestBase { } /** - * Tests the rebuildForm() method. + * Tests the rebuildForm() method for a POST submission. */ public function testRebuildForm() { $form_id = 'test_form_id'; @@ -303,6 +303,9 @@ class FormBuilderTest extends FormTestBase { $form = $this->formBuilder->buildForm($form_arg, $form_state); $original_build_id = $form['#build_id']; + $this->request->setMethod('POST'); + $form_state->setRequestMethod('POST'); + // Rebuild the form, and assert that the build ID has not changed. $form_state->setRebuild(); $input['form_id'] = $form_id; @@ -310,11 +313,51 @@ class FormBuilderTest extends FormTestBase { $form_state->addRebuildInfo('copy', ['#build_id' => TRUE]); $this->formBuilder->processForm($form_id, $form, $form_state); $this->assertSame($original_build_id, $form['#build_id']); + $this->assertTrue($form_state->isCached()); // Rebuild the form again, and assert that there is a new build ID. $form_state->setRebuildInfo([]); $form = $this->formBuilder->buildForm($form_arg, $form_state); $this->assertNotSame($original_build_id, $form['#build_id']); + $this->assertTrue($form_state->isCached()); + } + + /** + * Tests the rebuildForm() method for a GET submission. + */ + public function testRebuildFormOnGetRequest() { + $form_id = 'test_form_id'; + $expected_form = $form_id(); + + // The form will be built four times. + $form_arg = $this->getMock('Drupal\Core\Form\FormInterface'); + $form_arg->expects($this->exactly(2)) + ->method('getFormId') + ->will($this->returnValue($form_id)); + $form_arg->expects($this->exactly(4)) + ->method('buildForm') + ->will($this->returnValue($expected_form)); + + // Do an initial build of the form and track the build ID. + $form_state = new FormState(); + $form_state->setMethod('GET'); + $form = $this->formBuilder->buildForm($form_arg, $form_state); + $original_build_id = $form['#build_id']; + + // Rebuild the form, and assert that the build ID has not changed. + $form_state->setRebuild(); + $input['form_id'] = $form_id; + $form_state->setUserInput($input); + $form_state->addRebuildInfo('copy', ['#build_id' => TRUE]); + $this->formBuilder->processForm($form_id, $form, $form_state); + $this->assertSame($original_build_id, $form['#build_id']); + $this->assertFalse($form_state->isCached()); + + // Rebuild the form again, and assert that there is a new build ID. + $form_state->setRebuildInfo([]); + $form = $this->formBuilder->buildForm($form_arg, $form_state); + $this->assertNotSame($original_build_id, $form['#build_id']); + $this->assertFalse($form_state->isCached()); } /** @@ -338,6 +381,7 @@ class FormBuilderTest extends FormTestBase { // Do an initial build of the form and track the build ID. $form_state = (new FormState()) ->addBuildInfo('files', [['module' => 'node', 'type' => 'pages.inc']]) + ->setRequestMethod('POST') ->setCached(); $form = $this->formBuilder->buildForm($form_arg, $form_state); @@ -407,6 +451,7 @@ class FormBuilderTest extends FormTestBase { ->with($form_build_id); $form_state = new FormState(); + $form_state->setRequestMethod('POST'); $form_state->setCached(); $this->simulateFormSubmission($form_id, $form_arg, $form_state); } diff --git a/core/tests/Drupal/Tests/Core/Form/FormStateTest.php b/core/tests/Drupal/Tests/Core/Form/FormStateTest.php index b92c5f7b86be..24904e35fbfa 100644 --- a/core/tests/Drupal/Tests/Core/Form/FormStateTest.php +++ b/core/tests/Drupal/Tests/Core/Form/FormStateTest.php @@ -422,6 +422,11 @@ class FormStateTest extends UnitTestCase { 'cache' => $cache_key, 'no_cache' => $no_cache_key, ]); + + $form_state->setMethod('POST'); + $this->assertSame($expected, $form_state->isCached()); + + $form_state->setMethod('GET'); $this->assertSame($expected, $form_state->isCached()); } @@ -463,6 +468,28 @@ class FormStateTest extends UnitTestCase { return $data; } + /** + * @covers ::setCached + */ + public function testSetCachedPost() { + $form_state = new FormState(); + $form_state->setRequestMethod('POST'); + $form_state->setCached(); + $this->assertTrue($form_state->isCached()); + } + + /** + * @covers ::setCached + * + * @expectedException \LogicException + * @expectedExceptionMessage Form state caching on GET requests is not allowed. + */ + public function testSetCachedGet() { + $form_state = new FormState(); + $form_state->setRequestMethod('GET'); + $form_state->setCached(); + } + /** * @covers ::isMethodType * @covers ::setMethod diff --git a/core/tests/Drupal/Tests/Core/Render/Element/RenderElementTest.php b/core/tests/Drupal/Tests/Core/Render/Element/RenderElementTest.php new file mode 100644 index 000000000000..a554b8a5b20c --- /dev/null +++ b/core/tests/Drupal/Tests/Core/Render/Element/RenderElementTest.php @@ -0,0 +1,116 @@ +requestStack = new RequestStack(); + $this->container = new ContainerBuilder(); + $this->container->set('request_stack', $this->requestStack); + \Drupal::setContainer($this->container); + } + + /** + * @covers ::preRenderAjaxForm + */ + public function testPreRenderAjaxForm() { + $request = Request::create('/test'); + $request->query->set('foo', 'bar'); + $this->requestStack->push($request); + + $prophecy = $this->prophesize('Drupal\Core\Routing\UrlGeneratorInterface'); + $url = '/test?foo=bar&ajax_form=1'; + $prophecy->generateFromRoute('', [], ['query' => ['foo' => 'bar', FormBuilderInterface::AJAX_FORM_REQUEST => TRUE]], FALSE) + ->willReturn($url); + + $url_generator = $prophecy->reveal(); + $this->container->set('url_generator', $url_generator); + + $element = [ + '#type' => 'select', + '#id' => 'test', + '#ajax' => [ + 'wrapper' => 'foo', + 'callback' => 'test-callback', + ], + ]; + + $element = RenderElement::preRenderAjaxForm($element); + + $this->assertTrue($element['#ajax_processed']); + $this->assertEquals($url, $element['#attached']['drupalSettings']['ajax']['test']['url']); + } + + /** + * @covers ::preRenderAjaxForm + */ + public function testPreRenderAjaxFormWithQueryOptions() { + $request = Request::create('/test'); + $request->query->set('foo', 'bar'); + $this->requestStack->push($request); + + $prophecy = $this->prophesize('Drupal\Core\Routing\UrlGeneratorInterface'); + $url = '/test?foo=bar&other=query&ajax_form=1'; + $prophecy->generateFromRoute('', [], ['query' => ['foo' => 'bar', 'other' => 'query', FormBuilderInterface::AJAX_FORM_REQUEST => TRUE]], FALSE) + ->willReturn($url); + + $url_generator = $prophecy->reveal(); + $this->container->set('url_generator', $url_generator); + + $element = [ + '#type' => 'select', + '#id' => 'test', + '#ajax' => [ + 'wrapper' => 'foo', + 'callback' => 'test-callback', + 'options' => [ + 'query' => [ + 'other' => 'query', + ], + ], + ], + ]; + + $element = RenderElement::preRenderAjaxForm($element); + + $this->assertTrue($element['#ajax_processed']); + $this->assertEquals($url, $element['#attached']['drupalSettings']['ajax']['test']['url']); + } + +}