Remove FormState ArrayAccess usage from core

Discussed with @tim.plunkett per #2310255-12: [meta] Remove ArrayAccess from FormState. I think this order might be the easiest way to get this in:

1. Non-views
2. Views
3. Remove BC layer

...but splitting more granularly than that wouldn't make sense. (@tim.plunkett did the conversions locally directory by directory, so it's too much work to try to reverse-engineer the patch into method-per-method.)

1. +++ b/core/lib/Drupal/Core/FileTransfer/Form/FileTransferAuthorizeForm.php
   diff --git a/core/lib/Drupal/Core/Form/FormState.php b/core/lib/Drupal/Core/Form/FormState.php
   index 557ac0f..b91eb2f 100644
   
   index 557ac0f..b91eb2f 100644
   --- a/core/lib/Drupal/Core/Form/FormState.php
   
   --- a/core/lib/Drupal/Core/Form/FormState.php
   +++ b/core/lib/Drupal/Core/Form/FormState.php
   
   +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -810,61 +808,6 @@ public function &getCompleteForm() {
       * {@inheritdoc}
   -   *
   -   * @deprecated in Drupal 8.0.x, might be removed before Drupal 8.0.0.
   -   */
   -  public function offsetExists($offset) {
   -    return isset($this->{$offset}) || isset($this->storage[$offset]);
   -  }
   -
   -  /**
   

   Sad contrib!!

2. +++ b/core/modules/content_translation/content_translation.module
   @@ -674,11 +674,12 @@ function content_translation_element_info_alter(&$type) {
   +  if (!$context = $form_state->get(['language', $key])) {
   +    $context = [];
      }
   

   You can those kind of bits by just casting the result to an array.

3. +++ b/core/modules/editor/editor.module
   @@ -90,11 +89,11 @@ function editor_form_filter_admin_overview_alter(&$form, FormStateInterface $for
   +    $editor = editor_load($format_id);
   

   oh I thought we got rid of all entity_load calls.

4. +++ b/core/modules/field/field.module
   @@ -349,10 +349,11 @@ function field_form_config_admin_import_form_alter(&$form, FormStateInterface $f
   +  $storage_comparer = $form_state->get('storage_comparer');
   +  if ($storage_comparer && empty($user_input)) {
   

   Yeah another instance of a potential behaviour change, isset vs. boolean casting.

5. +++ b/core/modules/field/tests/modules/field_test/src/Form/NestedEntityTestForm.php
   @@ -29,13 +29,15 @@ public function getFormId() {
   -    $form_state['entity_1'] = $entity_1;
   -    $form_state['form_display_1'] = EntityFormDisplay::collectRenderDisplay($entity_1, 'default');
   -    $form_state['form_display_1']->buildForm($entity_1, $form, $form_state);
   +    $form_state->set('entity_1', $entity_1);
   +    $form_display_1 = EntityFormDisplay::collectRenderDisplay($entity_1, 'default');
   +    $form_state->set('form_display_1', $form_display_1);
   +    $form_display_1->buildForm($entity_1, $form, $form_state);
   

   It is really odd that we store an entire form object in form state, but well this is what the test is doing atm. already!

6. +++ b/core/modules/system/src/Tests/Form/FormDefaultHandlersTest.php
   @@ -47,28 +47,36 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -    $form_state['test_handlers']['validate'][] = __FUNCTION__;
   +    $test_handlers = $form_state->get('test_handlers');
   +    $test_handlers['validate'][] = __FUNCTION__;
   +    $form_state->set('test_handlers', $test_handlers);
   ...
   -    $form_state['test_handlers']['validate'][] = __FUNCTION__;
   +    $test_handlers = $form_state->get('test_handlers');
   +    $test_handlers['validate'][] = __FUNCTION__;
   +    $form_state->set('test_handlers', $test_handlers);
      }
   ...
   -    $form_state['test_handlers']['submit'][] = __FUNCTION__;
   +    $test_handlers = $form_state->get('test_handlers');
   +    $test_handlers['submit'][] = __FUNCTION__;
   +    $form_state->set('test_handlers', $test_handlers);
   ...
   -    $form_state['test_handlers']['submit'][] = __FUNCTION__;
   +    $test_handlers = $form_state->get('test_handlers');
   +    $test_handlers['submit'][] = __FUNCTION__;
   +    $form_state->set('test_handlers', $test_handlers);
   

   For those examples having a reference would be cool but yeah I don't care much.

7. +++ b/core/modules/system/src/Tests/Form/FormTest.php
   @@ -111,8 +111,8 @@ function testRequiredFields() {
   +          $form_state->setFormObject(new StubForm($form_id, $form));
   +          $form_state->setMethod('post');
   

   Should we use 'POST' now directly? I would say so, if possible (see previous discussion)

8. +++ b/core/modules/system/src/Tests/Form/ProgrammaticTest.php
   @@ -86,9 +86,7 @@ private function submitForm($values, $valid_input) {
   -      // By fetching the values from $form_state['storage'] we ensure that the
   -      // submission handler was properly executed.
   ...
   +      $stored_values = $form_state->get('programmatic_form_submit');
   

   Why did we get rid of the doc. line?

9. +++ b/core/modules/system/src/Tests/Form/TriggeringElementTest.php
   @@ -10,7 +10,7 @@
   + * Tests that FAPI correctly determines the triggering_element.
   
   @@ -24,7 +24,7 @@ class TriggeringElementTest extends WebTestBase {
   +   * Test the determination of the triggering element when no button
   

   Let's be consistent with the space or rather point to the method itself.

10. +++ b/core/modules/system/src/Tests/Form/TriggeringElementTest.php
    @@ -94,11 +91,10 @@ function testAttemptAccessControlBypass() {
         $this->assertNoText('The clicked button is button1.', '$form_state[\'triggering_element\'] not set to a restricted button.');
         $this->assertText('The clicked button is button2.', '$form_state[\'triggering_element\'] not set to a restricted button.');
    

    We should also change the text of the assertion message. More places above ...

11. +++ b/core/modules/system/tests/modules/batch_test/src/Form/BatchTestMultiStepForm.php
    @@ -56,9 +59,10 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +      $step++;
    +      $form_state->set('step', $step);
    

    Meh, you could also keep the ++$step inlined.

12. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestFormStateDatabaseForm.php
    @@ -38,10 +38,10 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -    if (isset($form_state['storage']['database_connection_found'])) {
    +    if ($form_state->has('database_connection_found')) {
    
    +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestRebuildPreserveValuesForm.php
    @@ -49,7 +49,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -    if (empty($form_state['storage']['add_more'])) {
    +    if (!$form_state->has('add_more')) {
    

    Just in general, it seems not to be clear when the patch uses has and when get, but then also a boolean. On top of that sometimes isset, and sometimes empty is replaced. It is probably not important but yeah there are tiny potential changes which could be bugs.

13. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestStorageForm.php
    @@ -32,22 +32,24 @@ public function getFormId() {
    +    if (empty($storage)) {
    ...
    +      $storage = [
    +        'thing' => [
               'title' => 'none',
               'value' => '',
    -        ),
    -      );
    +        ],
    +      ];
    +      $form_state->setStorage($storage);
    

    You could improve readability here by getting rid of one level of the array and using set() instead.

14. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestStorageForm.php
    @@ -119,7 +121,7 @@ public function continueSubmitForm(array &$form, FormStateInterface $form_state)
    -    if (isset($form_state['storage']['thing']['changed'])) {
    +    if ($form_state->has(['thing', 'changed'])) {
           drupal_set_message("The thing has been changed.");
         }
    

    Ah cool, so internally we already store in storage.

15. +++ b/core/modules/user/src/Form/UserLoginForm.php
    @@ -174,18 +174,18 @@ public function validateAuthentication(array &$form, FormStateInterface $form_st
    -      // Set $form_state['uid'] as a flag for self::validateFinal().
    ...
    +      // Store uid in form state as a flag for self::validateFinal().
    

    What about using $uid here instead?

16. +++ b/core/modules/user/src/Form/UserLoginForm.php
    @@ -174,18 +174,18 @@ public function validateAuthentication(array &$form, FormStateInterface $form_st
    -      $form_state['uid'] = $this->userAuth->authenticate($form_state->getValue('name'), $password);
    ...
    +      $form_state->set('uid', $this->userAuth->authenticate($form_state->getValue('name'), $password));
    

    Given how important that step is, extracting the authenticated user into a var would improve readability!

17. +++ b/core/modules/user/src/Tests/UserAccountFormFieldsTest.php
    @@ -31,9 +31,8 @@ class UserAccountFormFieldsTest extends DrupalUnitTestBase {
    -    $install_state = install_state_defaults();
         $form_state = new FormState();
    -    $form_state['build_info']['args'][] = &$install_state;
    +    $form_state->addBuildInfo('args', [install_state_defaults()]);
    

    Still same question as before!

18. +++ b/core/modules/user/user.module
    index c6daa71..16f4546 100644
    --- a/core/modules/views/includes/ajax.inc
    

    Views: The form state management system.

19. +++ b/core/modules/views/src/Form/ViewsExposedForm.php
    @@ -63,15 +63,15 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -    $form_state['must_validate'] = TRUE;
    +    $form_state->setValidationEnforced();
    

    Did we maybe considered "enforceValidation"? *just curious*

20. +++ b/core/modules/views/src/Form/ViewsFormMainForm.php
    @@ -108,7 +108,7 @@ public function buildForm(array $form, FormStateInterface $form_state, ViewExecu
    -    $view = $form_state['build_info']['args'][0];
    +    $view = $form_state->getBuildInfo()['args'][0];
    

    General question: did we considered to use a hashmap here together with the name passed along into the form method?

21. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -1675,10 +1676,10 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +          $form[$section] = [
                 '#tree' => TRUE,
    -          );
    -          $plugin->buildOptionsForm($form[$form_state['section']], $form_state);
    +          ];
    

    Yeah short array!

22. +++ b/core/modules/views/src/Plugin/views/exposed_form/ExposedFormPluginBase.php
    @@ -143,14 +143,14 @@ public function renderExposedForm($block = FALSE) {
    -      unset($form_state['rerender']);
    +      $form_state->set('rerender', NULL);
    

    Can we introduce an unSet method?

23. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
    @@ -1090,12 +1094,14 @@ protected function buildExposedFiltersGroupForm(&$form, FormStateInterface $form
    -      $form_state['js settings'] = array_merge($form_state['js settings'], $js);
    +    $js_settings = $form_state->get('js settings');
    ...
    +    $form_state->set('js settings', $js_settings);
    

    Let's open a follow up to get rid of the space. (novice task)

24. +++ b/core/modules/views_ui/src/Form/Ajax/Display.php
    @@ -84,11 +84,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
     
    -    $name = NULL;
    -    if (isset($form_state['update_name'])) {
    -      $name = $form_state['update_name'];
    -    }
    -
    +    $name = $form_state->get('update_name');
    

    This piece of change is really nice!

25. +++ b/core/modules/views_ui/src/Form/Ajax/Display.php
    @@ -97,10 +93,12 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +    $view = $form_state->get('view');
    +    $display_id = $form_state->get('display_id');
    +    $view->getExecutable()->displayHandlers->get($display_id)->validateOptionsForm($form['options'], $form_state);
    

    We don't have to be radically here about introducing extra variables but yeah you already done it, so this is fine.

26. +++ b/core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php
    @@ -164,8 +164,8 @@ public function getForm(ViewStorageInterface $view, $display_id, $js) {
    -      if (!empty($form_state['#page_title'])) {
    ...
    +      if ($page_title = $form_state->get('#page_title')) {
    +        $response->addCommand(new Ajax\ReplaceTitleCommand($page_title));
    

    Did you opened a novice follow up to get rid of "#" here? Would be great if you do.

27. +++ b/core/modules/views_ui/src/ViewPreviewForm.php
    @@ -135,9 +135,11 @@ public function submitPreview($form, FormStateInterface $form_state) {
    -    $form_state['build_info']['args'][0] = $new_view;
    -    $form_state['show_preview'] = TRUE;
    -    $form_state['rebuild'] = TRUE;
    +    $build_info = $form_state->getBuildInfo();
    +    $build_info['args'][0] = $new_view;
    +    $form_state->setBuildInfo($build_info);
    

    Did anyone considered it to allow passing it by reference?

28. +++ b/core/modules/views_ui/src/ViewUI.php
    @@ -330,11 +323,11 @@ public function getStandardButtons(&$form, FormStateInterface $form_state, $form
    -    if (empty($form_state['ok_button'])) {
    +    if (!$form_state->get('ok_button')) {
    
    @@ -364,19 +357,14 @@ public function getStandardButtons(&$form, FormStateInterface $form_state, $form
    -      '#value' => empty($form_state['ok_button']) ? t('Cancel') : t('Ok'),
    +      '#value' => !$form_state->has('ok_button') ? t('Cancel') : t('Ok'),
           '#submit' => array($cancel_submit),
    

    has or boolean get? Let's try to be consistent.

29. +++ b/core/modules/views_ui/src/ViewUI.php
    @@ -364,19 +357,14 @@ public function getStandardButtons(&$form, FormStateInterface $form_state, $form
    -      if (isset($form_state['build_info']['callback_object'])) {
    -        $form['actions']['submit']['#validate'][] = array($form_state['build_info']['callback_object'], 'validateForm');
    -      }
    -      if (function_exists($form_id . '_validate')) {
    -        $form['actions']['submit']['#validate'][] = $form_id . '_validate';
    -      }
    +      $form['actions']['submit']['#validate'][] = [$form_state->getFormObject(), 'validateForm'];
    

    Nice!

30. +++ b/core/modules/views_ui/src/ViewUI.php
    @@ -477,16 +465,17 @@ public function addFormToStack($key, $display_id, $type, $id = NULL, $top = FALS
    +    $display_id = $form_state->get('display_id');
    ...
    +      $display = &$this->executable->displayHandlers->get($display_id);
    

    This kind of increased the patch size a bit more than needed. anyway this is a good change.

Re 27):
I thought about being able to use:

$build_info = &$form_state->getBuildInfo();
$build_info['args'][0] = $new_view;

without anything more.

Okay cool!

A couple of comments look wrong.

// Populate the 'array_parents' information in $form_state['field'] after

and

// $form_state['field']['#parents'][...$parents...]['#fields'][$field_name],

Also this issue should be linked to the relevant CR

262k? Heh, I think this is less of a band-aid and more of a chainsaw. ;)

But at any rate, these changes are all consistent with others added in other patches. Tim also informs me that once this is in, that's it for Form API refactoring which makes me very, very, very, very happy.

I had a few questions in IRC which Tim was able to address. Namely:

- Why is it sometimes getFoo()/setFoo() and other times get('foo')/set('foo')... the general idea is that setFoo/getFoo are methods relating to Form API properties that are part of the actual API. get/set are for properties added by random contrib modules / other parts of core. And you can't use get/set on "proper" first-class properties; you have to use the methods.

Why?

"somethings we don't want to let you do. for example, some setters for the API don't actually take a parameter. like setSubmitted(FALSE) will not work, it is just setSubmitted() that does $this->submitted = TRUE; internally"

So given that we have a rational explanation for that, and given this patch looks like an enormous pain in the ass to re-roll, I'm going to go ahead and get this in. Definitely better to break things now than post-zero-beta-blockers.

Committed and pushed to 8.x. Thanks!

+++ b/core/modules/views/src/Plugin/views/filter/ManyToOne.php
@@ -111,7 +111,7 @@ function operators() {
-    if (empty($form_state['exposed'])) {
+    if ($form_state->get('exposed')) {

This is the trouble with massive patches; stuff just doesn't get reviewed properly :/

It is sad to see the array access go IMO...

Anyway, opened #2337897: ManyToOne filter should not display ManyToOneHelper valueForm when exposed!