Rewrite Wizard Support for 8.x-3.x [#2468609]

Since we're rewriting from scratch for D8, let's kick this off with Wizard!

We must allow for multi-step wizard to vary the step based upon input values as well. I've not tested that yet, but I believe this code lays the ground work for it.

The patch includes a number of files. About half of this was for testing purposes, so this is not ready to be committed, but I wanted to get what I was working on out there and a bit more visible so that other people could poke holes if there are any.

The classes worth looking at for the actual raw underlying code are:

Drupal\ctools\Controller\WizardFormController.php
Drupal\ctools\EventSubscriber\WizardControllerSubscriber.php
Drupal\ctools\Wizard\FormWizardInterface.php
Drupal\ctools\Wizard\FormWizardBase.php

FormWizardBase is the lion share of the work contains a sane default implementation (or at least my first pass at such an attempt). The notable architectural elements of this approach are a '_wizard' route attribute controller implementation that mimics what core does for '_form', but does so in a way that satisfies the Wizard system's base needs from page to page without forcing us into the ContainerInjectionInterface driven paradigm core. That being said, this paradigm is quite useful for forms in general, so the approach also preserves this approach for any individual step. Adopting this approach means each step can set its own dependencies through the normal Drupal 8 container injection approach w/o the Wizard class that weaves it all together needing to have every step's dependencies injected into it.

Getting the current step or the current operation requires the cached values submitted by the user, this is done so that specific wizard implementations could pick the current step/operation based upon submitted values by the users.

To see my rather simple example implementation look at Drupal\ctools\Wizard\WizardTest.php This 44 line file does the bare minimum of what's necessary to implement a wizard. Each form step is a normal FormInterface class.

Outstanding questions/todos:

Should the form_id of the wizard change to reflect the form id of the current step. I tend to thing "yes" but I've not implemented it.
Test coverage

Feedback?

Eclipse

Comment	File	Size	Author
#8	2468609-8.patch	39.51 KB	EclipseGc
#8	2468609-interdiff.txt	7.54 KB	EclipseGc
#3	2468609-3.patch	39.37 KB	EclipseGc
#3	2468609-interdiff.txt	36.26 KB	EclipseGc
#2	2468609-2.patch	23.48 KB	EclipseGc
#2	2468609-interdiff.txt	8.06 KB	EclipseGc
#1	2468609-1.patch	23.29 KB	EclipseGc

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

EclipseGc CreditAttribution: EclipseGc at Acquia commented 9 April 2015 at 22:10

Status:

Active

» Needs review

File	Size
2468609-1.patch	23.29 KB

code!

Comment #2

EclipseGc CreditAttribution: EclipseGc at Acquia commented 10 April 2015 at 13:28

File	Size
2468609-interdiff.txt	8.06 KB
2468609-2.patch	23.48 KB

This interdiff isn't 100% accurate cause some of it landed in the previous patch, but it'll give you an idea of the work done in-between (which was mostly documentation in nature).

That being said, I did update the wizard wrapper to return the current step's form id instead of its own custom one. This should make form_alters a ton easier to deal with in a wizard context. I do sort of wonder if we shouldn't prefix those form_ids in some way per each wizard they're used in. That would make for custom alterations ala a hook_forms style approach within wizards super easy.

Eclipse

Comment #3

EclipseGc CreditAttribution: EclipseGc at Acquia commented 21 April 2015 at 21:18

Assigned:	Unassigned	» EclipseGc
Priority:	Normal	» Major

File	Size
2468609-interdiff.txt	36.26 KB
2468609-3.patch	39.37 KB

Ok, with added tests and tons of cleanups!

Added in an entity wizard. No tests for this, but the lion share of my development the last 11 days has been around making that very solid. I'll try to get some tests written shortly. The testing that's been added is for vanilla wizards w/o an entity of any sort backing it. These are, in some ways, more complicated than entity wizards since with an entity we can at least sanely suspect that you want the entity's values for your wizard, and with a non-entity wizard we have no clue. In order to facilitate this, I've added the event dispatcher into the system and an event happens during load time that allows us to add/alter values for the wizard. More could probably be done around various event dispatching needs, so I'm injecting the dispatcher into the wizard class for the time being since it would be the arbiter of most of that. We should begin to identify useful events and add them to the FormWizardInterface.

I tried to document the crap out of this. A lot of my work has been around getting CTools Condition Ruleset ready for D8 as well (good test bed for Page Manager's needs). Once I commit this, that will be the next major thing to land. I'd say it's about 50% done (the wizard 50, still gotta write a condition plugin and derivative handler and various other clean ups).

I'm super happy with this code, assuming tests pass I'll be committing it so I can keep moving CTools forward. (Speak now or hold your peace :-D)

Eclipse

Comment #4

EclipseGc CreditAttribution: EclipseGc at Acquia commented 21 April 2015 at 22:27

I'm ready to commit this, trying to let it be visible for review for a little bit before I do.

Eclipse

Comment #5

EclipseGc CreditAttribution: EclipseGc at Acquia commented 21 April 2015 at 22:28

3 files were hidden/shown/deleted

File	Size
2468609-1.patch	23.29 KB
2468609-interdiff.txt	8.06 KB
2468609-2.patch	23.48 KB

Comment #6

bojanz CreditAttribution: bojanz commented 21 April 2015 at 23:06

Nice work! I like how the code looks. It's sufficiently small and easy to understand.

+   *
+   * @todo Consider moving this to EntityFormWizardBase.
+   */
+  protected function getDefaultFormElements($cached_values) {

Agreed. Feels weird to have those form elements on the base class.

+    $form['name']['id'] = array(
+      '#description' => t('A unique machine-readable name for this View. It must only contain lowercase letters, numbers, and underscores.'),

Once the move happens, might wanna make that description more generic ;)

Comment #7

EclipseGc CreditAttribution: EclipseGc at Acquia commented 22 April 2015 at 01:03

Bah, I'll fix the description in the next patch. :-(

The reason I haven't moved this yet is that non-entity wizards will have to do some interesting things to get a machine name and values. Ideally this is backed by CMI, and perhaps their machine name is just the config id, but this still seems like an open question to me. Should we just move it now and figure out non-entity wizard workflow later? At least if we did that, no one would be needlessly depending on form values that we want to remove.

Eclipse

Comment #8

EclipseGc CreditAttribution: EclipseGc at Acquia commented 22 April 2015 at 05:50

File	Size
2468609-interdiff.txt	7.54 KB
2468609-8.patch	39.51 KB

Ok, I refactored this a bit, moving entity defaults into the entity, but leaving the protected method on the base wizard since I can imagine defaults for the first step being a common need. This had the side effect of moving the machine_name into the route information, which was quite natural and simple to do, and make perfect sense for a one-off form wizard. Storage is still up to the developer of that type for wizard, but the path is easy enough to follow.

The test became a tad simpler due to fewer form elements. If this looks good to everyone by EOD 4/22/15, I'm committing this. Wizard step theming I'll nail down in a follow up issue.

Eclipse