We currently have our own subformstate, which is a behemoth of a class - and not a very well tested one. Drupal core 8.2 has it's own subform class and tests.

I think we should require 8.2 (it's been released) and remove our own version of the subforms and use core's instead. That'll be less code we have to maintain ourselves.

CommentFileSizeAuthor
#21 use_core_s_subform-2813707-21.patch23.5 KBborisson_
#21 interdiff.txt1.64 KBborisson_
#19 interdiff.txt3.77 KBborisson_
#19 use_core_s_subform-2813707-19.patch24.66 KBborisson_
#14 use_core_s_subform-2813707-14.patch23.37 KBborisson_
#14 interdiff.txt3.08 KBborisson_
#10 use_core_s_subform-2813707-10.patch23.23 KBborisson_
#10 interdiff.txt1.16 KBborisson_
#7 use_core_s_subform-2813707-7.patch22.07 KBborisson_
#4 use_core_s_subform-2813707-4.patch21.8 KBborisson_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

borisson_ created an issue. See original summary.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Issue tags: +Novice

Actionable items:

- Remove our own SubFormState class.
- Change the use statement for every time we use new SubFormState() to the core class.

Upload a patch to see if test still pass, test locally to see if the admin forms still work.

Change the facets.info.yml to also depend on system (>=8.2)

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented

https://www.drupal.org/node/2690229#comment-11713165 is a good example of the changes that need to be done here.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Status: Active » Needs review
Issue tags: -Novice
FileSize
use_core_s_subform-2813707-4.patch21.8 KB

I think this should do it.

Status: Needs review » Needs work

The last submitted patch, 4: use_core_s_subform-2813707-4.patch, failed testing.

The last submitted patch, 4: use_core_s_subform-2813707-4.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Status: Needs work » Needs review
FileSize
use_core_s_subform-2813707-7.patch22.07 KB

I tried this again, it still doesn't work. I hoped this was going to be easier. No interdiff as this is from scratch.

Status: Needs review » Needs work

The last submitted patch, 7: use_core_s_subform-2813707-7.patch, failed testing.

The last submitted patch, 7: use_core_s_subform-2813707-7.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.16 KB
use_core_s_subform-2813707-10.patch23.23 KB

I asked Xano for help, and he mentioned that we should trigger an error when not using subform state. So I added that. Still no idea why this is failing though. Will ask @drunken_monkey for help.

Status: Needs review » Needs work

The last submitted patch, 10: use_core_s_subform-2813707-10.patch, failed testing.

The last submitted patch, 10: use_core_s_subform-2813707-10.patch, failed testing.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented 
diff --git a/src/Form/FacetForm.php b/src/Form/FacetForm.php
index ac2a43c..f4c43f4 100644
--- a/src/Form/FacetForm.php
+++ b/src/Form/FacetForm.php
@@ -229,10 +229,7 @@ class FacetForm extends EntityForm {
           '#access' => !$processor->isHidden(),
         );
 
-        $processor_form_state = new SubFormState(
-          $form_state,
-          ['facet_settings', $processor_id, 'settings']
-        );
+        $processor_form_state = SubformState::createForSubform($form['facet_settings'], $form, $form_state);
         $processor_form = $processor->buildConfigurationForm($form, $processor_form_state, $facet);

If the keys of the subform were ['facet_settings', $processor_id, 'settings'], why are you now changing that to just $form['facet_settings']? Seems to me like it should be $form['facet_settings'][$processor_id]['settings'].

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Status: Needs work » Needs review
FileSize
interdiff.txt3.08 KB
use_core_s_subform-2813707-14.patch23.37 KB

Fixed #13, let's see how the testbot feels.

Status: Needs review » Needs work

The last submitted patch, 14: use_core_s_subform-2813707-14.patch, failed testing.

The last submitted patch, 14: use_core_s_subform-2813707-14.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented

Looks like that didn't really help. I'll have another go at this later.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Issue tags: +beta blocker
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Status: Needs work » Needs review
FileSize
use_core_s_subform-2813707-19.patch24.66 KB
interdiff.txt3.77 KB

Status: Needs review » Needs work

The last submitted patch, 19: use_core_s_subform-2813707-19.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.64 KB
use_core_s_subform-2813707-21.patch23.5 KB

borisson_ credited Xano.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Issue tags: +drupalironcamp2016

Crediting Xano

  • borisson_ committed b61159f on 8.x-1.x 
    Issue #2813707 by borisson_, Xano: Use core's subform state instead of...
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Status: Needs review » Fixed

This is finally green! Committed. Thanks Bart!

Status: Fixed » Closed (fixed)

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