Updated: Comment #38

Problem/Motivation

Looking at validation function now, we have really unfortunate and confusing calls like:


    if (!$url_is_valid) {
      \Drupal::formBuilder()->setError($element['url'], $form_state, $this->t('The URL %url is not valid.', array('%url' => $element['url']['#value'])));
    }

The code to set an error is just manipulating the values of the form state, so there is actually no need do call or inject the form builder.

Proposed resolution

let's make the form state into an object that knows how to set errors and do other internal manipulations so that we can work directly with it in validation functions. We can use the ArrayObject interface to simplify backward compatibility and conversion.


    if (!$url_is_valid) {
      $form_state->setError($element['url'], $this->t('The URL %url is not valid.', array('%url' => $element['url']['#value']));
    }

Remaining tasks

todo

User interface changes

N/A

API changes

If FormBuilderInterface::buildForm() is called directly, it must be an instance of FormStateInterface.

Before:

$form_state = array(
  'rebuild' => TRUE;
);
$form_builder->buildForm('Drupal\my_module\Form\MyForm', $form_state);

After:

use Drupal\Core\Form\FormInterface;
$form_state = new FormState(array(
  'rebuild' => TRUE;
));
$form_builder->buildForm('Drupal\my_module\Form\MyForm', $form_state);

Because FormState uses ArrayAccess, you can still access properties as an array, but the method is preferred:

Before:
$form_state['rebuild'] = TRUE;
 
After:
$form_state->set('rebuild', TRUE);

In order to do things like setting errors, you no longer need the whole form builder:

Before:
\Drupal::formBuilder()->setError($element, $form_state, $message);

After:
$form_state->setError($element, $message);

The following interface methods have been changed from array &$form_state to FormStateInterface $form_state:

FormInterface::buildForm()
FormInterface::validateForm()
FormInterface::submitForm()
EntityFormDisplayInterface::buildForm()
EntityFormDisplayInterface::validateFormValues()
EntityFormDisplayInterface::extractFormValues()
FieldItemInterface::settingsForm()
FieldItemInterface::instanceSettingsForm()
FieldItemListInterface::defaultValuesForm()
FieldItemListInterface::defaultValuesFormValidate()
FieldItemListInterface::defaultValuesFormSubmit()
FormatterInterface::settingsForm()
WidgetBaseInterface::form()
WidgetBaseInterface::extractFormValues()
WidgetBaseInterface::flagErrors()
WidgetBaseInterface::getWidgetState()
WidgetBaseInterface::setWidgetState()
WidgetInterface::settingsForm()
WidgetInterface::formElement()
WidgetInterface::errorElement()
WidgetInterface::massageFormValues()
FormBuilderInterface::buildForm()
FormBuilderInterface::submitForm()
ImageToolkitInterface::settingsFormSubmit()
MenuLinkFormInterface::extractFormValues()
PluginFormInterface::buildConfigurationForm()
PluginFormInterface::validateConfigurationForm()
PluginFormInterface::submitConfigurationForm()
BlockPluginInterface::blockForm()
BlockPluginInterface::blockValidate()
BlockPluginInterface::blockSubmit()
BookManagerInterface::addFormElements()
CKEditorPluginConfigurableInterface::settingsForm()
ContentTranslationHandlerInterface::entityFormAlter()
EditorPluginInterface::settingsForm()
EditorPluginInterface::settingsFormValidate()
EditorPluginInterface::settingsFormSubmit()
SelectionInterface::validateAutocompleteInput()
FilterInterface::settingsForm()
SearchInterface::searchFormAlter()
WizardInterface::buildForm()
WizardInterface::validateView()
WizardInterface::createView()
ViewsFormInterface::getFormState()
CommentFileSizeAuthor
#65 form_state-2225353-65.patch1.12 MBtim.plunkett
#64 interdiff.txt599 bytestim.plunkett
#64 form_state-2225353-64.patch1.13 MBtim.plunkett
#62 form_state-2225353-62.patch1.13 MBtim.plunkett
#57 form_state-2225353-57.patch1.13 MBtim.plunkett
#57 interdiff.txt3.39 KBtim.plunkett
#52 interdiff.txt2.45 KBtim.plunkett
#52 form_state-2225353-52.patch1.13 MBtim.plunkett
#49 form_state-2225353-49.patch1.13 MBtim.plunkett
#48 form_state-2225353-48-do-not-test.patch118.41 KBtim.plunkett
#47 interdiff.txt36.28 KBtim.plunkett
#47 form_state-2225353-47.patch1.12 MBtim.plunkett
#45 form_state-2225353-45.patch1.11 MBtim.plunkett
#45 interdiff.txt59 KBtim.plunkett
#44 form_state-2225353-44.patch1.09 MBtim.plunkett
#42 form_state-2225353-42.patch1.1 MBtim.plunkett
#42 interdiff.txt992 bytestim.plunkett
#38 form_state-2225353-38.patch1.1 MBtim.plunkett
#38 interdiff.txt3.67 KBtim.plunkett
#37 form_state-2225353-37.patch1.09 MBtim.plunkett
#37 interdiff.txt1.08 KBtim.plunkett
#35 form_state-2225353-35.patch1.09 MBtim.plunkett
#30 interdiff.txt2.84 KBtim.plunkett
#30 form_state-2225353-30.patch1.08 MBtim.plunkett
#25 form_state-2225353-25.patch1.08 MBtim.plunkett
#25 interdiff.txt808.73 KBtim.plunkett
#23 form_state-2225353-23.patch1.08 MBtim.plunkett
#23 interdiff.txt40.58 KBtim.plunkett
#22 interdiff.txt458 bytestim.plunkett
#22 form_state-2225353-22.patch1.07 MBtim.plunkett
#20 interdiff.txt25.99 KBtim.plunkett
#20 form_state-2225353-20.patch1.07 MBtim.plunkett
#19 form_state-2225353-19.patch1.06 MBtim.plunkett
#17 form_state-2225353-17.patch1.05 MBtim.plunkett
#15 interdiff.txt10.29 KBtim.plunkett
#15 form_state-2225353-15.patch1.06 MBtim.plunkett
#13 interdiff.txt2.82 KBtim.plunkett
#13 form_state-2225353-13.patch819.3 KBtim.plunkett
#11 form_state-2225353-11.patch816.48 KBtim.plunkett
#8 form-2225353-8.patch788.34 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Issue summary: View changes
tim.plunkett’s picture

I wish you luck, sir. I tried really hard to do this when I was working on the initial FormBuilder code.

Consider code like this:

function content_translation_add_page(EntityInterface $entity, Language $source = NULL, Language $target = NULL) {
  $source = !empty($source) ? $source : $entity->language();
  $target = !empty($target) ? $target : \Drupal::languageManager()->getCurrentLanguage(Language::TYPE_CONTENT);
  // @todo Exploit the upcoming hook_entity_prepare() when available.
  content_translation_prepare_translation($entity, $source, $target);
  $form_state['langcode'] = $target->id;
  $form_state['content_translation']['source'] = $source;
  $form_state['content_translation']['target'] = $target;
  $form_state['content_translation']['translation_form'] = !$entity->access('update');
  return \Drupal::service('entity.form_builder')->getForm($entity, 'default', $form_state);
}

$form_state does not exist, and is initialized as an array.
Unless every entry point into FAPI code has code like

if (is_array($form_state)) {
  $form_state = new FormState($form_state);
}

, I don't see how else to avoid this.

Also consider that even if you try to use ArrayObject or whatever, it will not satisfy the typehint array $form_state, no matter how hard you try.

tim.plunkett’s picture

I should clarify. I think this would be wonderful for DX. There are so many weird parts of $form_state (look at FormBuilder getUncacheableKeys() and getFormStateDefaults()) that have hard-to-find documentation, and making these methods on a class would be really awesome. I just couldn't get it to work, and didn't want to mess with every array typehint in existence.

larowlan’s picture

Shouldn't it be FormElement->setError where SelectElement et al extend FormElement :)

tim.plunkett’s picture

#4:

a) OO-ifying the form elements is D9 territory, and should explore symfony forms
b) The errors are canonically stored in the form_state, not on the element themselves.

pwolanin’s picture

Thanks for the warnings of dragons - I also suggested a trait on the for class, but that would be a lot of code duplicate under the covers.

I'll tackle other more obvious issues first if this on is such a rathole.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

So on a whim, I'm trying this again.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
788.34 KB

This is going to be large.

This doesn't even provide any useful methods yet, just the necessary conversions.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
816.48 KB

Rerolled after #2268761: Remove support for function-based forms, and fixed installer.

tim.plunkett’s picture

Priority: Normal » Major
Status: Needs work » Needs review
Issue tags: +beta deadline
FileSize
819.3 KB
2.82 KB

Missed a spot.

Before patch

$ grep -nr "function .*&\$form_state" * | wc -l
    1580
$ grep -cr "function .*&\$form_state" * | grep -v ":0$" | wc -l
     650

With patch so far

$ grep -nr "function .*&\$form_state" * | wc -l
     518
$ grep -cr "function .*&\$form_state" * | grep -v ":0$" | wc -l
     257

I got all of the ones in interfaces, all of the methods should happen, not as worried about form_alter yet.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.06 MB
10.29 KB

Not pictured in the interdiff: All of &$form_state is now typehinted

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.05 MB
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.06 MB
tim.plunkett’s picture

FileSize
1.07 MB
25.99 KB

Okay, here are some example methods, with one example each.

I also deprecated FormErrorInterface.

I would not like to expand the scope past this point at all. The typehint renames already make this patch monstrously huge :(

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.07 MB
458 bytes

:)

tim.plunkett’s picture

FileSize
40.58 KB
1.08 MB

Fixed all known docblocks to stop calling form_state an array.

dawehner’s picture

Status: Needs review » Needs work

Reviewing the file using whatthepatch, see http://nbviewer.ipython.org/9559bcc48fccbf49c265

+function hook_field_widget_form_alter(&$element, \Drupal\Core\Form\FormStateInterface &$form_state, $context) {

Unneeded reference.

diff --git a/core/modules/field_ui/field_ui.api.php b/core/modules/field_ui/field_ui.api.php
index 070f020..b67d89b 100644
--- a/core/modules/field_ui/field_ui.api.php
+++ b/core/modules/field_ui/field_ui.api.php
@@ -21,7 +21,7 @@
  *   The entity view mode.
  * @param array $form
  *   The (entire) configuration form array.
- * @param array $form_state
+ * @param \Drupal\Core\Form\FormStateInterface $form_state
  *   The form state.
  *
  * @return array
@@ -54,7 +54,7 @@ function hook_field_formatter_third_party_settings_form(\Drupal\Core\Field\Forma
  *   The entity form mode.
  * @param array $form
  *   The (entire) configuration form array.
- * @param array $form_state
+ * @param \Drupal\Core\Form\FormStateInterface $form_state
  *   The form state.
  *
  * @return array

Missing changes of actual code.

+function hook_node_validate(\Drupal\node\NodeInterface $node, $form, \Drupal\Core\Form\FormStateInterface &$form_state) {

references, more in other api.php files

+  public function buildForm(array $form, FormStateInterface &$form_state) {

This is also in other files, pretty much everywhere

-   *   Use this to pass additional information to the form, such as the
+   * @param array $form_state_additions
+   *   (optional) An associative array used to build the current state of the
+   *   form. Use this to pass additional information to the form, such as the
    *   langcode. Defaults to an empty array.
    *
    * @code
@@ -37,6 +37,6 @@
    * @return array
    *   The processed form for the given entity and operation.
    */
-  public function getForm(EntityInterface $entity, $operation = 'default', array $form_state = array());
+  public function getForm(EntityInterface $entity, $operation = 'default', $form_state_additions = array());

This should be explained in the issue summary or somewhere.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update
FileSize
808.73 KB
1.08 MB

I'm leaving the references on $form_state for all of FormBuilder, FormValidator, and FormSubmitter. They need to physically replace the $form_state, which can only be done if it is passed by reference. The same thing goes for views_ajax_form_wrapper().

I removed the rest of the references.

I need to explain the change to EntityFormBuilder::getForm() in the issue summary, but I have to run for now.

dawehner’s picture

Continued review.

+   * @param string $key
+   *   The key of the data to store.
+  public function setIfNotExists($key, $value);
+  /**
+   * Gets any arbitrary property.
+   *
+   * @param string $property
+   *   The property to retrieve.
+   *
+   * @return mixed
+   *   The value for that property, or NULL if the property does not exist.
+   */
+  public function &get($property);

We should try to have the same parameter names here.

       // Store $form_state information in the batch definition.
-      // We need the full $form_state when either:
-      // - Some submit handlers were saved to be called during batch
-      //   processing. See self::executeSubmitHandlers().
-      // - The form is multistep.
-      // In other cases, we only need the information expected by
-      // self::redirectForm().
-      if ($batch['has_form_submits'] || !empty($form_state['rebuild'])) {
-        $batch['form_state'] = $form_state;
-      }
-      else {
-        $batch['form_state'] = array_intersect_key($form_state, array_flip(array('programmed', 'rebuild', 'storage', 'no_redirect', 'redirect', 'redirect_route')));

This is such a great change!

--- a/core/lib/Drupal/Core/Form/FormSubmitterInterface.php

The current user-submitted data is stored
+   *   in $form_state['values'],

It would be great to also use $form_state->get('values') here or maybe getValues() (not sure whether there is a specific method for that).

+  /**
+   * Flags an element as having an error.
+   */
+  public function setError(&$element, $message = '');

Can we have a @return $this just for consistency on all set*() methods?

+++ b/core/lib/Drupal/Core/Form/FormSubmitter.php

+      $batch['form_state'] = clone $form_state;

It would be great to document here, why we need a clone here.

+ * Provides an interface for an object containing the current state of a form.
  */
-interface FormErrorInterface {
+interface FormStateInterface {

Let's be honest, this is not enough. We could add some @see when the form_state is created and processed for example. Everyone
will use that interface

+  /**
+   * @param array $form_state_additions
+   */
+  public function __construct(array $form_state_additions = array()) {
+    $this->formState = $form_state_additions;
+  }

Lazyness is flying around here. Can we also use a better name instead of form_state_additions? We don't really add something here.

+    $this->formState = $form_state_additions + $this->formState;
+    return $this;

Did we considered to merge the arrays?

+  protected function getUncacheableKeys() {
+ return array(

Did we considered to just store a property?

+  /**
+   * {@inheritdoc}
+   */
+  public function offsetExists($offset) {
+    return isset($this->formState[$offset]);
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function &offsetGet($offset) {

Is this code which will be kept in the future?

  public function setIfNotExists($key, $value) {
+    if (!array_key_exists($key, $this->formState)) {
+      $this->set($key, $value);
+    }
+  }

we could consider to use the isset optimization here.

dawehner’s picture

Issue tags: +API change

This is an API change which will affect everyone, so adding yet another tag.

pounard’s picture

I didn't read the full thread, but I think this is an extremely good idea to move the form API toward being more maintainable. In the long term (I don't know if you planned it or not once conversion is done in every form) I think that technical properties of the form state should become object properties with default values set to make it even more comprehensible, and you would not need the addFormStateDefaults(), getUncacheableKeys() and getCacheableArray() methods anymore. Values should be an object of its own, as well as storage: doing that you could return easily storage or values without need to specify the & in front of the get() method which can have weird side effects on object internal state. And you could just cache those two without having to check for (un|)cacheable keys.

Aside of that, that's great work you're doing!

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.08 MB
2.84 KB

I think that technical properties of the form state should become object properties with default values set to make it even more comprehensible

Ideally yes, but I think that can happen after this conversion goes in.

I addressed some of #28 but not all of it, I just wanted to get this applying again. I'll have to keep rerolling until beta blockers go in, I'll have more time for this Sunday night and then next week.

tim.plunkett’s picture

Status: Needs work » Needs review

Random fail. Retested.

tim.plunkett’s picture

I wanted to look at #29 more, but I hit an issue with the Views UI. Splitting off to #2307885: Refactor FormState to use dedicated properties instead of an associative array.

Here's a reroll.

tim.plunkett’s picture

FileSize
1.09 MB
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.08 KB
1.09 MB
tim.plunkett’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
FileSize
3.67 KB
1.1 MB

Trying to remove the clone mentioned in #27, and also moving setValue.

Also updated the issue summary.

We have WAY too many interfaces that deal with plugins :(

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
992 bytes
1.1 MB

So much for that!

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.09 MB

Oh, whoops, it seems it was the setValue() change?! Sorry for the noise.
Removed that addition.

tim.plunkett’s picture

Issue summary: View changes
FileSize
59 KB
1.11 MB

Added more docs, moved tests to the right place, renamed some internal properties.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.12 MB
36.28 KB

Whew, got that all figured out.

tim.plunkett’s picture

Okay, ready for more reviews, I'm happy with this one.

Here is an attempt to remove the typehint and use statements from the patch, highlighting just the functional changes.

tim.plunkett’s picture

FileSize
19.75 KB
1.13 MB

Based on some IRC conversations, I went ahead and added methods we're likely going to need.

Code is in my sandbox: http://cgit.drupalcode.org/sandbox-tim.plunkett-1698392/log/?h=2225353-form

dawehner’s picture

I really love this patch, reviewed #48

  1. +++ b/core/lib/Drupal/Core/Entity/EntityFormBuilder.php
    @@ -44,10 +44,11 @@ public function __construct(EntityManagerInterface $entity_manager, FormBuilderI
    +  public function getForm(EntityInterface $entity, $operation = 'default', $form_state_additions = array()) {
    ...
    +    $form_state = new FormState($form_state_additions);
    

    Are we sure we want to not explicit typehint it to array? I know you are not a fan of it, but it helps people to know what they should put in.

  2. +++ b/core/lib/Drupal/Core/Form/FormBase.php
    @@ -172,34 +165,21 @@ private function container() {
    +  protected function setFormError($name, FormStateInterface $form_state, $message = '') {
    +    $form_state->setErrorByName($name, $message);
         return $this;
       }
    

    Is it just me that there is no point in having this method? Is this done to have a small patch? If yes, feel free to open up a followup.

  3. +++ b/core/modules/views_ui/src/Form/Ajax/Display.php
    @@ -35,9 +35,9 @@ public function getFormKey() {
       public function getFormState(ViewStorageInterface $view, $display_id, $js) {
    -    return array(
    -      'section' => $this->type,
    -    ) + parent::getFormState($view, $display_id, $js);
    +    $form_state = parent::getFormState($view, $display_id, $js);
    +    $form_state['section'] = $this->type;
    +    return $form_state;
       }
    

    Do we have a follow up what to do with this special snowflake in views?

  4. +++ b/core/tests/Drupal/Tests/Core/Form/FormStateTest.php
    @@ -0,0 +1,162 @@
    +class FormStateTest extends UnitTestCase {
    

    This is just awesome!

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.13 MB
2.45 KB

#51.1 That's a fine suggestion.

#51.2 Correct, it's just used way more than you'd think. Opened #2308821: Replace FormErrorInterface with $form_state->setErrorByName() and $form_state->setError()

#51.3 Honestly this doesn't really bother me. I don't think it's any worse now than it was before.

#51.4 <3 unit tests

I forgot a key line in setValue(), that should fix the installation.

dawehner’s picture

Status: Needs review » Postponed

I think this is RTBC

tim.plunkett’s picture

Just to note, since this conflicts with two beta blockers, I asked @dawehner to postpone this instead of actually RTBCing.
I'll be posting rerolls of this after each of those go in, leaving postponed for now.

YesCT’s picture

this is gonna be great to be able to use!
------------
i. note 52 applied on 5776d98

ii. I looked up this while reading the patch
start a PHP function with an ampersand?
http://stackoverflow.com/questions/1676897/what-does-it-mean-to-start-a-...

iii. I only read carefully the first half of #48 (up through the FormStateInterface)

iv. I would have rerolled this and just fixed some of my nits, but it's assigned and work is in a sandbox... and didn't want to disturb work that might already be in progress.

v. some of these might be dismissed with "we are not doing that" or "that will be done in issue XX"

  1. +++ b/core/lib/Drupal/Core/Entity/EntityFormBuilderInterface.php
    @@ -37,6 +37,6 @@
    -  public function getForm(EntityInterface $entity, $operation = 'default', array $form_state = array());
    +  public function getForm(EntityInterface $entity, $operation = 'default', $form_state_additions = array());
    

    why did this take out the array type hint? can we add that back in?

  2. +++ b/core/lib/Drupal/Core/Form/FormBase.php
    @@ -172,34 +165,21 @@ private function container() {
    -   * @see \Drupal\Core\Form\FormErrorInterface::setErrorByName()
    +   * @deprecated Use \Drupal\Core\Form\FormStateInterface::setErrorByName().
    

    a. We have more verbose @deprecated info template now.
    https://www.drupal.org/node/1354#deprecated

    Something like

    577: * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
    578- *   Use \Drupal::service('update.manager')->projectStorage().
    579- */
    580-function update_project_storage($key) {
    

    b. Does this actual replace all the usages of setFormError? I dont think so based on the comments so far, so we need a follow-up issue.. which we have: #2308821: Replace FormErrorInterface with $form_state->setErrorByName() and $form_state->setError() Ok.

  3. +++ b/core/lib/Drupal/Core/Form/FormState.php
    @@ -0,0 +1,696 @@
    + * @todo Remove usage of \ArrayAccess.
    + */
    +class FormState implements FormStateInterface, \ArrayAccess {
    

    a. need a child issue for this and reference the url in the @todo.

    b. OH! would this be the issue to actually use the methods everywhere?

  4. +++ b/core/lib/Drupal/Core/Form/FormState.php
    @@ -0,0 +1,696 @@
    +   *   - args: A list of arguments to pass to the form constructor.
    +   *   - files: An optional array defining include files that need to be loaded
    ...
    +   * @var array
    +   */
    +  protected $build_info = array(
    +    'args' => array(),
    +    'files' => array(),
    +  );
    

    a. is args also optional?

    b. Does it help to say an array of arguments instead of list?

  5. +++ b/core/lib/Drupal/Core/Form/FormState.php
    @@ -0,0 +1,696 @@
    +   * confirmation forms. Normally, $form_state['rebuild'] is set by a submit
    +   * handler, since its is usually logic within a submit handler that determines
    +   * whether a form is done or requires another step. However, a validation
    +   * handler may already set $form_state['rebuild'] to cause the form processing
    +   * to bypass submit handlers and rebuild the form instead, even if there are
    +   * no validation errors.
    +   *
    +   * This property is uncacheable.
    +   *
    +   * @var bool
    +   */
    +  protected $rebuild = FALSE;
    

    should the example $form_state['rebuild'] be $form_state->rebuild ? or $form_state->setRebuild()?

  6. +++ b/core/lib/Drupal/Core/Form/FormState.php
    @@ -0,0 +1,696 @@
    +   * file download. If you use the $form_state['redirect'] key, it will be used
    +   * to build a \Symfony\Component\HttpFoundation\RedirectResponse and will
    +   * populate this key.
    +   *
    +   * @var \Symfony\Component\HttpFoundation\Response|null
    +   */
    +  protected $response;
    

    similar question here if the doc should say: If you set $form_state->redirect, it will... (or the redirect methods)

  7. +++ b/core/lib/Drupal/Core/Form/FormState.php
    @@ -0,0 +1,696 @@
    +   * Used to redirect the form on submission. It may either be a  string
    +   * containing the destination URL, or an array of arguments compatible with
    

    nit. double space "be a string"

  8. +++ b/core/lib/Drupal/Core/Form/FormState.php
    @@ -0,0 +1,696 @@
    +  protected $redirect_route;
    

    a. these property names have to stay redirect_route and not be renamed redirectRoute since the usages are still going to be $form_state['redirect_route'] and not converted to $form_state->set('redirect_route') ... uh, $form_state->setRebuild() ?

    b. do we want a follow-up to convert array accesses to method calls like that later?

    c. do we want a follow-up to rename the properties to lowerCamelCase?

  9. +++ b/core/lib/Drupal/Core/Form/FormState.php
    @@ -0,0 +1,696 @@
    +   * 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
    

    nit. IDs (not ids). But this doc is just being moved... so maybe not worry about it.

  10. +++ b/core/lib/Drupal/Core/Form/FormState.php
    @@ -0,0 +1,696 @@
    +   * Contains references to details elements to render them within vertical tabs.
    

    a. this can be wrapped for 80 chars.

    b. can we provide an example?

    c. should other array() properties also provide examples, or document their keys and values? or can we @see some set method that lists out what keys are in the array?

  11. +++ b/core/lib/Drupal/Core/Form/FormState.php
    @@ -0,0 +1,696 @@
    +  /**
    +   * @param array $form_state_additions
    +   */
    +  public function __construct(array $form_state_additions = array()) {
    

    nit. missing one line summary here. Maybe just:
    Constructs a \Drupal\Core\Form\FormState object.

  12. +++ b/core/lib/Drupal/Core/Form/FormState.php
    @@ -0,0 +1,696 @@
    +  public function &offsetGet($offset) {
    

    should we name this getOffset()?

  13. +++ b/core/lib/Drupal/Core/Form/FormState.php
    @@ -0,0 +1,696 @@
    +  public function offsetSet($offset, $value) {
    +    $this->set($offset, $value);
    +  }
    

    a. why doesn't this return $this? We wont need to chain this one?

    b. should we name this setOffset() ?

  14. +++ b/core/lib/Drupal/Core/Form/FormState.php
    @@ -0,0 +1,696 @@
    +  public function offsetUnset($offset) {
    

    similarly, unsetOffset() ?

  15. +++ b/core/lib/Drupal/Core/Form/FormState.php
    @@ -0,0 +1,696 @@
    +      // @todo Remove once all redirects are converted to Url.
    

    is there already an issue we can link to for this @todo? Maybe #2189661: Replace $form_state['redirect_route'] with setRedirect() ?

  16. +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
    @@ -0,0 +1,272 @@
    +   *   The value will be one of the following:
    +   *     - A fully prepared \Symfony\Component\HttpFoundation\RedirectResponse.
    +   *     - An instance of \Drupal\Core\Url to use for the redirect.
    

    nit, standards thing,
    https://www.drupal.org/node/1354#lists
    the items in a list following : should not be indented.

  17. +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
    @@ -0,0 +1,272 @@
    +   * Gets any arbitrary property.
    ...
    +   * @return mixed
    +   *   The value for that property, or NULL if the property does not exist.
    +   */
    +  public function &get($property);
    

    gets/returns a *reference* to any arbitrary property?

Stopped after the interface.

tim.plunkett’s picture

Thanks for not rerollinf, I'm keeping it fresh in the sandbox. I'll address the rest of your comments later today

tim.plunkett’s picture

Status: Postponed » Reviewed & tested by the community
FileSize
3.39 KB
1.13 MB

Keep in mind this issue is RTBC and is just waiting for the conflicting beta blocker to go in.
That said, it doesn't hurt to have more in-code issue links for @todos.

Will mark postponed again, just want to get a new test run.

#55.1 This was done in #52.
#55.2 We don't know when this will be removed, I'm not okay with promising that.
#55.3 #2310255: [meta] Remove ArrayAccess from FormState, added in code reference

#55.4 These docs are copy/paste from the old ones.
#55.5 These docs are copy/paste from the old ones.
#55.6 These docs are copy/paste from the old ones.
#55.7 These docs are copy/paste from the old ones.
#55.8a Yes
#55.8b See 55.3
#55.8c It might happen as part of the other issue, but I'm not worried about it either way
#55.9 These docs are copy/paste from the old ones.
#55.10 These docs are copy/paste from the old ones.
#55.11 Sure

#55.12 This is a method from ArrayAccess, we have no choice.
#55.13 This is a method from ArrayAccess, we have no choice.
#55.14 This is a method from ArrayAccess, we have no choice.

#55.15 Sure
#55.16 Fixed

tim.plunkett’s picture

Status: Reviewed & tested by the community » Postponed

Great!

tim.plunkett’s picture

Added a draft change notice: https://www.drupal.org/node/2310411

jibran’s picture

I think we should add all the methods of \Drupal\Core\Form\FormStateInterface. to the change notice.

.

tim.plunkett’s picture

I updated the draft with a link to where FormStateInterface will live on api.d.o, but no we're not replicating a list of all the methods on the change notice. We don't do that in other change notices.

tim.plunkett’s picture

Status: Postponed » Reviewed & tested by the community
FileSize
1.13 MB

The blocker went in. No other changes since RTBC.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.13 MB
599 bytes

Weird, must have been a bad rebase.

tim.plunkett’s picture

Dries’s picture

Issue tags: +Avoid commit conflicts

I couldn't do a full review over this patch but like it directionally. It seems like the right thing to do. It most likely needs a re-roll after the menu link commit.

tim.plunkett’s picture

We cross-posted; #65 is the reroll after all of the menu link patches. This is ready!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

  • Dries committed fc5e111 on 8.0.x
    Issue #2225353 by tim.plunkett: Convert  to an object and provide...
jhodgdon’s picture

Hey, great job at fixing up the documentation in conjunction with this patch -- much appreciated! I filed a small followup:
#2313089: Form generation topic needs quick follow-up fix
It's a novice issue; I expect it will be snapped up by one of our great novice patchers before too long and fixed.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Issue tags: -Avoid commit conflicts +FormState

Thanks!

jhodgdon’s picture

I just filed a near-critical documentation follow-up on this:
#2313603: FormStateInterface::getValues() says "sanitized" -- vague and misleading

It would be great if someone who worked on this issue could add a note there as to what should be done, then add tag "Novice" so someone will quickly make a patch.

tim.plunkett’s picture

That text was added on Mon May 14 13:43:38 2007, in #138706: FormAPI 3: Ready to rock.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.