Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Reduces API Fragility The temporary form storage area has no api, and currently must be interacted with in old-style array-pi. This can lead to misunderstanding of use and even in appropriate usage circumstances can include generating quite a few lines of codes for what a single method could do. In short, mirroring the methods we have for non-temporary sections of the form-state will keep the API consistent and usable.
Disruption Not disruptive for core/contributed modules because it will only introduce a new API which is only used once in core and will streamline contribs use of this feature.

Problem/Motivation

#2339151: Conditions / context system does not allow for multiple configurable contexts, eg. language types (a major bug) starts to use FAPI's temporary storage, but it has proven cumbersome compared to regular storage.

$temporary = $form_state->getTemporary();
$temporary['foo'] = 'bar';
$form_state->setTemporary();

As opposed to:
$form_state->setTemporaryValue('foo', 'bar');

Proposed resolution

Add getTemporaryValue, setTemporaryValue, and hasTemporaryValue.

Remaining tasks

N/A

User interface changes

API changes

CommentFileSizeAuthor
#13 interdiff.txt429 bytestim.plunkett
#13 form-state-temp-2371853-13.patch6.08 KBtim.plunkett
#11 interdiff.txt1.69 KBtim.plunkett
#11 form-state-temp-2371853-9.patch5.88 KBtim.plunkett
#3 form-state-temp-2371853.2.patch4.18 KBlarowlan
#3 interdiff.txt1.21 KBlarowlan
#1 2371853-temporary-1.patch2.97 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
2371853-temporary-1.patch2.97 KB
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented 
+++ b/core/lib/Drupal/Core/Form/FormState.php
@@ -714,6 +714,31 @@ public function getTemporary() {
+  public function &getTemporaryValue($key) {
+    $value = &NestedArray::getValue($this->temporary, (array) $key);
+    return $value;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function setTemporaryValue($key, $value) {
+    NestedArray::setValue($this->temporary, (array) $key, $value, TRUE);
+    return $this;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function hasTemporaryValue($key) {
+    $exists = NULL;
+    NestedArray::getValue($this->temporary, (array) $key, $exists);
+    return $exists;
+  }
+

I think we need some tests for this? Tackling

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
FileSize
interdiff.txt1.21 KB
form-state-temp-2371853.2.patch4.18 KB

Adds tests
I consider this ready.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented 
+++ b/core/tests/Drupal/Tests/Core/Form/FormStateTest.php
@@ -514,6 +514,23 @@ public function providerTestIsMethodType() {
+    $this->assertFalse($form_state->hasTemporaryValue('rainbow_sparkles'));
...
+    $this->assertSame($form_state->getTemporaryValue('rainbow_sparkles'), 'yes please');
...
+    $form_state->setTemporaryValue(array('rainbow_sparkles', 'magic_ponies'), 'yes please');
+    $this->assertSame($form_state->getTemporaryValue(array('rainbow_sparkles', 'magic_ponies')), 'yes please');

Please change this to 2 data vars with some random names allowed

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
Status: Needs review » Reviewed & tested by the community

What a great change! Not to run over andy here, but that seems very nitpicky for something we can use now. RTBCing unless people really feel strongly about this.

Eclipse

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

Also #2339151: Conditions / context system does not allow for multiple configurable contexts, eg. language types has landed so I guess we should be able to make use of these additions in Core.

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
Issue summary: View changes
EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
Issue summary: View changes
EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Updated the IS

Eclipse

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work

What about

Also #2339151: Conditions / context system does not allow for multiple configurable contexts, eg. language types has landed so I guess we should be able to make use of these additions in Core.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Reviewed & tested by the community
FileSize
form-state-temp-2371853-9.patch5.88 KB
interdiff.txt1.69 KB

Addressing the other half of #6

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: form-state-temp-2371853-9.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Reviewed & tested by the community
FileSize
form-state-temp-2371853-13.patch6.08 KB
interdiff.txt429 bytes

Hah! Glad we tried to use this.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

This normal task reduces fragility by adding a better API so is allowed as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption. Committed 9bb4f4c and pushed to 8.0.x. Thanks!

diff --git a/core/tests/Drupal/Tests/Core/Form/FormStateTest.php b/core/tests/Drupal/Tests/Core/Form/FormStateTest.php
index 70149b9..2b2a193 100644
--- a/core/tests/Drupal/Tests/Core/Form/FormStateTest.php
+++ b/core/tests/Drupal/Tests/Core/Form/FormStateTest.php
@@ -521,7 +521,6 @@ public function providerTestIsMethodType() {
    */
   public function testTemporaryValue() {
     $form_state = New FormState();
-    $form_state->setTemporary(array());
     $this->assertFalse($form_state->hasTemporaryValue('rainbow_sparkles'));
     $form_state->setTemporaryValue('rainbow_sparkles', 'yes please');
     $this->assertSame($form_state->getTemporaryValue('rainbow_sparkles'), 'yes please');

So let's make the test cover what @tim.plunkett found in #13 too. Fixed on commit.

  • alexpott committed 9bb4f4c on 8.0.x 
    Issue #2371853 by tim.plunkett, larowlan: Add more helper methods around...

Status: Fixed » Closed (fixed)

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