Break up MigrateUpgradeForm into smaller forms

tag and add related issue

How does one break this up into smaller forms? Not knowing forms, I did some searching and found examples where each form has a unique URL. Is that what we want here? The examples I found all use a base form and extend from that. How does that work here when the form is @internal?

@heddn, thanks

The easiest way to break these up I think is via a new form class and url route.

Can you expand on this? Do you mean each step becomes a new form class with a new url route? Or something else?

I'll try to make a start on this, this week.

The form is now split into new form classes, Overview, Crendential, IdConflict and Review, and it almost works as expected. The problem is with IdConflict form, it needs to be skipped if there are no conflicts, and currently the form is always displayed. In the original code in buildIdConflict form, when it is determined that there are no conflicts, the step is changed to the next one and the form rebuilt. The next step being the Review form. This is done with this code.

      $form_state->set('step', 'confirm');
      return $this->buildForm($form, $form_state);

The first try was to change that to a redirect, but the IdConflict form is still displayed. How to do this? Reading and looking at examples hasn't helped, they are all focused on how to redirect on submitForm, but here the fact that there are no IdConflicts and thus nothing to display is determined in buildForm. Suggestions?

@quietone: Can you post your work so I can take a look? I'm interested since I'll need to add another form for the post process migrations in #2912348: Handle entity_references related to Drupal 6 and 7 node translations with different IDs.

@maxocub, sure. I can do that tomorrow.

Here is what I've got so far, at least it does work.

What I don't like is that the logic is no longer in the submitForm method, so it isn't easy to see the flow and the code to determine if the IdConflict form is to be displayed is duplicated. Also, the @internal needs to be restored and something needs to change so it isn't possible to navigate to /upgrade/review before going through the whole series of forms.

Did some cleanup, mostly using inheritdoc for the form methods and found a method copied to a form where it was not needed.

These forms need to share data so MigrateUpgradeFormBase creates a private temporary store. The credential form save the legacy version value, the system data, and the migrations to the store. The IdConflict form uses the migrations, and the other two are used but the ReviewForm.

1. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
   @@ -0,0 +1,248 @@
   +   * @todo Private files directory not yet implemented, depends on
   +   *   https://www.drupal.org/node/2547125.
   

   This issue has been fixed.

2. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
   @@ -0,0 +1,248 @@
   +        // Store the retrived system data in form storage.
   

   s/retrived/retrieved/

Let's try that patch again.

This should fix the missing text errors about "What will be upgraded?".

Where is my head these days? I was not working from HEAD.

Left in a debugging line.

NR for testbot

Good. That should bring this up to date and ready for reviews.
Some questions I have

1. MigrateUpgradeFormBase injects all the services needed by the forms (it was easier), should that be separated so each form injects just what it needs?
2. Now that each form has a URL it is possible to navigate to the them out of order and bad things happen. How is that normally handled?

And a personal note, I don't like that the logic for which form is displayed is now spread across each form and in the event subscriber. It makes it harder to see the flow.

Made an issue for #14.1 #2941912: Remove incremental @todo from MigrateUpgradeForm

edit: get the list formatted correctly

The patch applies cleanly. Meets the requirements.

Thanks for trying out the patch, but it's not ready for prime time. There are a few questionable things that I'd like to refactor and test before this is ready for RTBC. I should get a chance to work on this today.

MigrateUpgradeFormBase injects all the services needed by the forms (it was easier), should that be separated so each form injects just what it needs?

Yes. I'll take care of this in refactoring.

Now that each form has a URL it is possible to navigate to the them out of order and bad things happen. How is that normally handled?

During the buildForm() stage, each form can validate that it has whatever data it needs in order to work (i.e., in tempstore) and throw exceptions if it doesn't.

Alright! I was able to improve the dependency management and remove the event subscriber in favor of returning redirect responses from the buildForm() methods. All tests passed locally. Fingers crossed!

@phenaproxima, thanks! Yes, that is better. I must have done something wrong when I tried a redirect from buildform.

Did a test. Made a fresh install, navigate to /upgrade and everything worked as expected, added a node, navigated to /upgrade and now the IdConflict form was displayed. From the final page /upgrade/review, I navigated to /upgrade/incremental and got this

The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">InvalidArgumentException</em>: The timestamp must be numeric. in <em class="placeholder">Drupal\Component\Datetime\DateTimePlus::createFromTimestamp()</em> (line <em class="placeholder">198</em> of <em class="placeholder">core/lib/Drupal/Component/Datetime/DateTimePlus.php</em>). <pre class="backtrace">Drupal\Core\Datetime\DateFormatter-&gt;format(NULL) (Line: 81)
Drupal\migrate_drupal_ui\Form\IncrementalForm-&gt;buildForm(Array, Object)
call_user_func_array(Array, Array) (Line: 514)
Drupal\Core\Form\FormBuilder-&gt;retrieveForm(&#039;migrate_drupal_ui_incremental_form&#039;, Object) (Line: 271)

Note that I didn't check incremental.

Reinstalled with a fresh db.
Navigate to
/upgrade/incremental - Same error as above
/upgrade/credentials - the credentials page is displayed
/upgrade/idconflicts - 404
/upgrade/review - 5 warnings, and 3 modules will be upgraded.

Presumably, these should all redirect to /upgrade/overview to start or restart the process each time.

#2941912: Remove incremental @todo from MigrateUpgradeForm got in, this needs a re-roll.

I needed to apply this patch on my machine, so I did the re-roll, might as well upload it.

Reroll works. Setting back to Needs Review.

This patch still needs work based on @quietone's review in #29

This fixes the problems identified in #29. Each form now begins with a check of the $step value (saved in the store) and if it is not this form, the step is set to 'overview' and then redirected to the overview form. Every redirect is accompanied with a corresponding setting of the step value.

Also removed the no longer used submitOverviewForm method.

Tagged for manual testing so that someone checks that it works as expected.

Missed setting $step with one of the redirects and fixed coding standard errors.

The flow of steps seems to be working as expected, I'm unable to reproduce the errors from #29.

Back to NW for a few nits:

1. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
   @@ -0,0 +1,260 @@
   +   * @todo Private files directory not yet implemented, depends on
   +   *   https://www.drupal.org/node/2547125.
   

   Out of scope nit, but this can be removed, this issue has been fixed.

2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeFormBase.php
   @@ -0,0 +1,123 @@
   +  /**
   +   * The Drupal version of the source site.
   +   *
   +   * @var string
   +   */
   +  protected $version;
   

   I don't see this used anywhere.

3. +++ b/core/modules/migrate_drupal_ui/src/Form/OverviewForm.php
   @@ -0,0 +1,92 @@
   +    return 'migrate_drupal_ui_form';
   

   Should this be migrate_drupal_ui_overview_form to be consistent with the other form IDs, or is it left like this for BC?

Addressed #38. Please review.

@chiranjeeb2410, thank for the patch but what is the reason for including a fix #38.1. which maxocub points out is out of scope?

Due to the scale of this patch, it seems to me it is better to stay in scope. That is why when I discovered another @todo which needed updating a new issue was made, #2941912: Remove incremental @todo from MigrateUpgradeForm, which had the additional bonus of being tagged novice. And yet, the fix has been made, it is relatively minor, so I'm OK with it staying in.

Unassigning phenaproxima, this needs a review and since he did a refactoring in #28, someone else needs to do that.

This is pretty much ready. As the IS suggest, any other clean-up or improvements should be done in follow-ups since this issue is only to break up the form.

It would be very nice if this issue lands, it would help with this Migrate Critical #2912348: Handle entity_references related to Drupal 6 and 7 node translations with different IDs since we need to add a new post process upgrade form that would greatly benefit from extending the new base form here.

Tests are green and manual testing has been done. Let's go with RTBC.

@quietone @maxocub,

Thanks for the review. Just thought #38.1 can be addressed with #38.2 and #38.3 as well, but was unnecessary. The other two just suffice.

Status changed by mistake. My bad.

@chiranjeeb2410, thanks for the quick reply. Tis all good. It is just a matter if a committer objects.

This is blocking an issue but it is effectively blocking all the Migrate UI issues (although they are not tagged as such) simply because of ease of working on an individual form instead of the monolith that MigrateUpgradeForm is right now. Speaking for myself, I want this is before continuing with the other Migrate UI issues.

1. <3 <3 <3 this patch. Really nice to see this. So much easier to understand.

2. +++ b/core/modules/migrate_drupal_ui/src/Form/IdConflictForm.php
   @@ -0,0 +1,204 @@
   +  protected function conflictsForm(array &$form, FormStateInterface $form_state, array $conflicts) {
   ...
   +  protected function i18nWarningForm(array &$form, FormStateInterface $form_state, array $conflicts) {
   

   Neither of this use $form_state. Do we think it is a good idea to pass it in. I'm not sure. Unused parameters always kind of bother me.

3. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeFormBase.php
   @@ -0,0 +1,116 @@
   +abstract class MigrateUpgradeFormBase extends ConfirmFormBase {
   

   I'm not sure that we really need to extend ConfirmFormBase. The only method I think we still have \Drupal\Core\Form\ConfirmFormBase::getFormName(). And that I think is just not necessary since it is only used by \Drupal\Core\Form\ConfirmFormBase::buildForm() (which we never call). Maybe followup material but it might be nice to get the class structure right here.

   Ah we use getConfirmText() but we could just declare this as an abstract on MigrateUpdateFormBase and MigrateUpdateFormBase could inherit directly from FormBase.

   Seems worth it to me. One less layer.

4. I agree that we should be trying to do as little as possible in this patch but I do think that we should endeavour to get the API and class inheritance structure as right as possible here - hence setting back to needs work.

@alexpott,

What do you suggest for #45.2 as of now? Although it makes sense to remove it as there's no usage of FormValidation for changing the submited form values to pass it to the submission handlers.

Also, from #45.3 could you finalize on the need of extending ConfirmFormBase or should we use the getConfirmText() by declaring it as an abstract and inheriting from FormBase as you suggested?

for #45.2 i'd remove the unused parameters. If we ever need form state in either method we can add it then because this is a form class and therefore not part of Drupal's API.

for #45.3 I'd make MigrateUpdateFormBase extend FormBase instead of ConfirmFormBase and add

  /**
   * Returns a caption for the button that confirms the action.
   *
   * @return string
   *   The form confirmation text.
   */
  abstract protected function getConfirmText();

to MigrateUpdateFormBase and change all of the getConfirmText() implementations in the Migrate update forms to protected - since there is no need for them to be public.

Addressed #45. Please review.
Hope to get a green flag!

@chiranjeeb2410 it won't be green. You need to make the following changes:

diff --git a/core/modules/migrate_drupal_ui/src/Form/IdConflictForm.php b/core/modules/migrate_drupal_ui/src/Form/IdConflictForm.php
index 840e48ce82..6ef5ab7a62 100644
--- a/core/modules/migrate_drupal_ui/src/Form/IdConflictForm.php
+++ b/core/modules/migrate_drupal_ui/src/Form/IdConflictForm.php
@@ -91,10 +91,10 @@ public function buildForm(array $form, FormStateInterface $form_state) {
       $form['#title'] = $this->t('Upgrade analysis report');
 
       if ($content_conflicts) {
-        $form = $this->conflictsForm($form, $form_state, $content_conflicts);
+        $form = $this->conflictsForm($form, $content_conflicts);
       }
       if ($translated_content_conflicts) {
-        $form = $this->i18nWarningForm($form, $form_state, $translated_content_conflicts);
+        $form = $this->i18nWarningForm($form, $translated_content_conflicts);
       }
       return $form;
     }
@@ -109,8 +109,6 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    *
    * @param array $form
    *   An associative array containing the structure of the form.
-   * @param \Drupal\Core\Form\FormStateInterface $form_state
-   *   The current state of the form.
    * @param \Drupal\migrate\Audit\AuditResult[] $conflicts
    *   The failing audit results.
    *
@@ -163,8 +161,6 @@ protected function formatConflicts(array $conflicts) {
    *
    * @param array $form
    *   An associative array containing the structure of the form.
-   * @param \Drupal\Core\Form\FormStateInterface $form_state
-   *   The current state of the form.
    * @param \Drupal\migrate\Audit\AuditResult[] $conflicts
    *   The failing audit results.
    *

I know this because I ran the \Drupal\Tests\migrate_drupal_ui\Functional\d7\MigrateUpgrade7Test test locally. Testing locally is a good litmus test of a patch.

@alexpott,

My bad. Shouldn't have been in a hurry. Fixing it asap.

@alexpott,

Addressed remaining issues as part of #49. Please review.

@chiranjeeb2410, thanks for the patch! The interdiff appears to be empty, though; can you regenerate that?

@phenaproxima,

The previous patch appears to be incorrect as it doesnt incorporate the changes mentioned in #49, which explains the blank interdiff.
Uploaded fresh patch contains all the latest changes as per #45 and #49 along with interdiff. Hope to show green and get RTBC soon!
Please review.

Interdiff looks good to me. Restoring RTBC on the assumption that Drupal CI will pass it. Thanks!

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeFormBase.php
@@ -0,0 +1,124 @@
+  public function getQuestion() {}
...
+  public function getCancelUrl() {}
...
+  public function getDescription() {}

Since this is not extending from ConfirmFormBase these are not needed.

Thanks, @quietone. Restoring RTBC.

Adding credit for my reviews that made a difference to the patch and for @heddn who opened the issue.

Committed da3818a and pushed to 8.6.x. Thanks!

Going to leave open against 8.5.x and ask the release managers about backport.

Sorry to crash the party, but I just found a use case where things are not optimal.

Currently, in this patch, some data is store in a TempStore (like legacy version, current step, upgrade performed, etc.) and other data is stored in state (fallback_state_key and database credentials). As the name indicates, TempStore is temporary and is lost after a week (default duration).

Since the incremental form checks for the step (temp) to see if it can be displayed and since the overview form redirect to the incremental form only if an upgrade has been performed (temp), if someone do an upgrade, wait more than a week, and then come back, the data from the TempStore does not exists anymore and the form behave like no upgrade has been performed.

I suggest we store the 'performed' into state so the site is always aware that an upgrade has been performed.

Should we revert this or create a follow-up?

Here's a complete list of what these forms store in TempStore:

• migrations
• performed
• source_base_path
• source_private_file_path
• step
• system_data
• version

Maybe we could store the version too in state? It could be useful in #2912348: Handle entity_references related to Drupal 6 and 7 node translations with different IDs, in which post process migrations could possibly be run more than a week after an upgrade.

Nice catch @maxocub - unfortunately I think we should revert this as it is a task and tasks should not introduce bugs. We should also test the scenario you've outlined if we continue to use tempstore.

I agree that reverting was the right move. Nice catch, @maxcoub. Let's get this fixed.

@maxocub, thanks for finding this. Working on it now.

The only change here is to use state to store the 'date_performed' value but I'm thinking that all the values listed in #60 should be moved to state. The forms expect the values currently in the store to be there and if they are not a screen full of errors will be the result. This needs a little more thought than my tired brain can do tonight.

What values should be saved in state?

Well didn't everything used to be in state? We could put it all back there and then open a followup to discuss whether that is reasonable. I don't think it is - things like migrations and system_data sound like things that should be cached or rebuilt more often or somehow locked into the migration you are running. Ah looking at the old code migrations was in form state so leaving in the temp store makes sense. So maybe that is the rule... what used to be form_state moves to private tempstore. What used to be state stays state. Therefore looking @maxocub's list:

migrations - used to be form state - store in private tempstore
performed - used to be state - store in state
source_base_path - used to be form state - store in private tempstore
source_private_file_path - used to be form state - store in private tempstore
step - used to be form state - store in private tempstore
system_data - used to be form state - store in private tempstore
version - used to be form state - store in private tempstore

So tldr; I think @quietone's fix in #65 is good. I wondered if we should move state constructor injection to the base class but CredentialForm and IdConflictForm don't use it so I think we're good here.

What heddn suggests makes sense. It also covers the use case that was bothering me, that is, incremental. Currently, if an upgrade has been performed there is no way to get to the credentials form so when the store variables are removed, the user would be truly stuck, unable to perform the incremental and unable to get to the credential form.

#68.2, Should be a follow up so this can stay about breaking up the form. See #44 and #45.4.

This adds the checks as described in #68 and improves the summary line per #68.1.

@heddn, did you mean 'when someone visits the incremental page'? I think that is what you meant, but I haven't made that change yet. I want to test the changes made so far and running them locally takes such a long time I'm using the testbot.

Above patch addresses the pointers as required in #68 and applies cleanly.
However, as mentioned by @quietone that some testing is still left to be done as per @heddn, which would take up some a bit of time, so not setting this to RTBC as of now.

I'll work on this this weekend.

1. Added a new test to check that the forms are displayed in the order expected which is, Overview or Increment, Credentials, IdConflict, Review. The tests are done with all the values expected to be in store for the form deleted/missing.
2. Removed a form rebuild in Incremental::validateForm() which AFAIKT isn't needed anymore.
3. Renamed migrate_drupal_ui.upgrade_id_conflict to migrate_drupal_ui.upgrade_idconflict so it was consistent with the others and I would stop misspelling it.
4. There is one change to the flow, that is that the Credential form is now displayed for Incremental. That means that the credential form is displayed every time you visit /upgrade which allows the db credentials to be changed or re-entered if the values in store are automatically deleted. The user is no longer stuck. The fail patch is specific to this change.

I'd like to add a test that a warning or error is not displayed, how is that done?

Update the MigrateUpgradeTest to reenter the database credentials during the Incremental migration.

On the form page, can we make it a little easier for folks and not require a new uid/pwd if we previously had a uid/pwd stored. And if it is empty, then don't overwrite the previous creds. We'll want to add a descriptive comment on the creds page stating the uid/pwd is optional if it doesn't change.

Agreed, but that sounds out of scope. Follow-up, perhaps?

This now include the helper function suggested in #68 to handle redirecting to the Overview page and another helper in the test to check that all the form paths go to the Overview page. The tests checking that the first form displayed is correct, either Overview or Incremental, are now in a loop ensuring the same tests are run for both situations.

I reread the comments from the revert, #63, onwards. Everything is done except possibly for one comment, in #68, which is "And we should probably add a test about what happens when the values in state are expired". Do values in state expire? The state API overview page says, "You can reset a system, losing all state. Its configuration remains." What does reset a system mean in Drupal? How does one lose state? If the state value 'performed' is somehow no longer available the process will start at the Overview page instead of the Incremental page. Does this mean that performed should be stored in config?

Edit: Adjusted the second paragraph to clearly identify the quote for comment #68.

Tests passed, this is ready for some feedback.

@quietone regular state in normal running of a Drupal site should not be lost. So the 'performed' state should not disappear automagically. The rest of the stuff can because it is in temp store and that is invalid after a week (by default). So I think the test coverage you've added here is correct. If some has hacked there site to remove the state we set then fair enough they are on their own.

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeFormStepsTest.php
@@ -0,0 +1,137 @@
+        $state->set('migrate_drupal_ui.performed', 1);

This should be a date value I think.

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeFormStepsTest.php
@@ -41,85 +42,68 @@
+      $this->drupalGet('/upgrade/idconflict');
+      $session->responseContains($expected[$type]);
...
+      $this->drupalGet('/upgrade/review');
+      $session->responseContains($expected[$type]);
...
+      $this->drupalGet('/upgrade/review');
+      $session->responseContains($expected[$type]);
...
+      $this->drupalGet('/upgrade/review');
+      $session->responseContains($expected[$type]);

@@ -130,2 +114,24 @@
+      $this->drupalGet('/upgrade' . $path);
+      $session->responseContains($expected);

I think rather than asserting what the page contains we should assert the URL because here I think we are expecting a redirect - no?

Thx, alexpott.

1. migrate_drupal_ui.performed is a UNIX timestamp, so 1 is OK. I have updated the comment to explain that it is a timestamp.
2. All the assertions have been changed to addressEquals, which make far more sense since the test is only checking that the correct form is displayed for various conditions.

Removed the needs test tag.

Nice work everyone! Committed and pushed 3af430ad54 to 8.6.x and fcb7dd9d0b to 8.5.x. Thanks!

As migrate_drupal_ui is still experimental this is eligible for backport.