FormState::set(), get(), and has() should explain they work with storage, and crosslinked with getStorage()

Does the applied description add enough clarity?

The test result failure seems to be unrelated to the patch provided. The patch needs to be re-tested.

Thanks! This is a good start, but it still needs some work:

1. +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
   @@ -192,7 +192,9 @@ public function &getStorage();
   +   * Sets a value to an arbitrary property which saves it to the form state storage.
   

   This line goes over 80 characters. The first line of a function doc block needs to be a one-line description of less than 80 chars.

2. +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
   @@ -192,7 +192,9 @@ public function &getStorage();
   +   * @see FormState::getStorage()
   

   @see goes at the end of a doc block, not in the middle. See https://www.drupal.org/node/1354#order

   But in any case, we should not be doing an @see to FormState::getStorage, but rather to FormStateInterface::getStorage().

   Also it needs the full namespace.

   Also we need an @see on getStorage to this method. And all related methods should also be linked in this way.

Done as said by Jhodgdon.

Good, but not complete... We should have @see references between all of the following methods, I think:
- setStorage
- getStorage
- get
- set
- has

They all work with the storage. We should also update the get() and has() methods in a similar way to what set() has in this patch (to say in the first line that they work with the storage).

But I also think we need to make sure that the terminology is consistent between getStorage(), setStorage(), get(), set(), and has(). Currently, in this patch, set() says "Sets value to an arbitrary property and saves it to the form state storage." What I think it should probably say is more like "Sets a value for a property in the form state storage." ("a value to an arbitrary property" doesn't really make sense to me). And then all the other 4 methods should use that same terminology.

Come to think of it, I am not sure the @param $property description on get(), set(), and has() actually makes much sense either. They are on an interface, but they reference $storage, which is not defined on the interface so doesn't really exist. This should also be fixed, so that it talks about the "form state storage" rather than $storage.

Adding tag for sprints on Friday. The patch needs work to address the comments above.

I'm taking a look at this at Drupalcon Dublin

Hi @jhodgdon,

reviewing your comments on #7, I've uploaded a patch that cross references setStorage, getStorage, get, set, has with @see comments for each . Is this what you meant?

On the following comment:
This should also be fixed, so that it talks about the "form state storage" rather than $storage

I've discussed with one of the sprint mentors, and wanted to check this. setStorage does refer to the $storage array, and getStorage returns this array, so could you clarify why we shouldn't refer to $storage?

Many thanks,

Finn

I am now looking at this issue

Patch that Finn provided works OK, comments are rectified as requested. Setting this issue to "review" to see if it will pass tests.

Updated the description of the issue to reflect all the changes as advised in the comment #7

Removed "Needs issue summary update" tag

Hiding patches #6 and #2 as patch #14 includes the code of the former two patches

Unassigning from me as I have been explained by my mentor that we should use comments rather that assigning issue to us. I am still working on this issue

Test have now passed for both php 5 and 7 on 8.6.x branch

setting to "Reviewed & tested by the community"

Hey @dark-o thanks for working on this.

+++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
@@ -173,11 +178,16 @@ public function setStorage(array $storage);
+ * Gets a value for a property in the form state storage.

I think "Gets the value" reads better than "Gets a value" (same for "Sets the value") but that might also just be overly nit-picky so fine by me if you don't agree. Just thought I would point it out.

I think we should also update the docblock for \Drupal\Core\Form\FormState::$storage as well. It currently reads:

  /**
   * This is not a special key, and no specific support is provided for it in
   * the Form API. By tradition it was the location where application-specific
   * data was stored for communication between the submit, validation, and form
   * builder functions, especially in a multi-step-style form. Form
   * implementations may use any key(s) within $form_state (other than the keys
   * listed here and other reserved ones used by Form API internals) for this
   * kind of storage. The recommended way to ensure that the chosen key doesn't
   * conflict with ones used by the Form API or other modules is to use the
   * module name as the key name or a prefix for the key name. For example, the
   * entity form classes use $this->entity in entity forms, or
   * $form_state->getFormObject()->getEntity() outside the controller, to store
   * information about the entity being edited, and this information stays
   * available across successive clicks of the "Preview" button (if available)
   * as well as when the "Save" button is finally clicked.
   *
   * @var array
   */
  protected $storage = [];

But, this isn't really accurate anymore. As the rest of the changes in this issue show we now have get/set/has methods that are part of the API and work exclusively with this variable.

suggestion on

"Gets the value" reads better than "Gets a value"

looks good to me, hence re-rolled the patch from #14.

My bad, I just updated for 'Gets' and missed out the 'Sets', will update the patch and re-roll the same.

If anyone wants to review this issue we should make sure that the @neerajsingh's patch in #31 addresses @eojthebrave's comments and update the issue summary to indicate the proposed resolution of reviewed patch as well as any info. that a core committer might need to know while evaluating the issue.

I think we still need to get the text for

\Drupal\Core\Form\FormState::$storage

updated. This text for example, is no longer true: "This is not a special key, and no specific support is provided for it in the Form API."

And then there's a bunch of maybe useful, maybe not, details about making keys unique. I would argue this could probably be removed since I believe just using these methods, and $storage, is the recommended best-practice now. But, I don't know that for sure. :/

I think updating the text to something like:

Storage for application specific information and communication between the submit, validation, and form builder functions, especially in a multi-step-style form. This data should be accessed using the related getter/setter methods.

And then @see those.

I'm around at the Sprint in Nashville if anyone is working on this today.

Reviewed at a documentation triage meeting.

The changes in the patch are still valid but it needs a reroll. The issue summary is out of date,. Adding tags. This is suitable for a novice, leaving that tag.

Re-rolled patch #31 against 10.1.x-dev
Please review it.

ignore this comment

I have considered the above suggestions and incorporated the changes in this patch.

Those were changes suggested by the code snipper uploading another patch by removing those changes.

Committed/pushed to 10.1.x, thanks!