Remove @todos from migrate credentials form

We tried to understand what's being asked for here on our UX call but couldn't figure it out. Issue summary could do with a bit more detail :-)

Referencing a comment in another issue that also only explains things in code doesn't count :)

What is the actual proposed user interface change here?

Nothing to review at this moment.

Assigning to myself.

Did some testing of the form by adding the #limit_validation_error back and changing #required on the database and username fields. I discovered that the credential form is setup by a call to getFormOptions() which set those fields to required. MigrateUpgradeForm has code to set required to false on those fields, and it does (debugged that), but the form still displays those fields as required. Despite clearing the cache.

In the API documentation for #limit_validation_errors it uses an example of a "Previous" button on a form where you might want to submit despite not having valid data in the fields. Now, that makes sense but this form doesn't have a "Previous" or a "Cancel" . Maybe it should?

Added a screenshot, IS upated.

Setting to NR to get some feed back on what to do here. Restore #limit_validation_errors or not?

I'm not so sure this is really that major, with all the other priorities in migrate space. Lowering priority. There's a couple ways to mark something as required in the FAPI. And there's even a way to do in using the Javascript States API. I think the general idea is we want to conditionally make some of these fields require. Using the States API would be the preferred method, but setting things required in that manner has been traditionally complicate. But let's try it first.

And now actually lowering priority.

Needs steps to reproduce the problem. Let's get those documented in here and update the IS.

@alexpott / @quietone is this issue still relevant? If so can you provide some steps to test that we can use to work on a patch?

Talked to pameeela on Slack, just adding my comments from there.

I am not familiar with #limit_validation_errors'. But I do know that if one makes the database and username fields required on the Credential Form then you can't successfully submit the form. Even when those fields are filled in you get errors,

    Database name field is required.
    Database username field is required.

so perhaps this is a won't fix.

In this case I agree. However, is this exposing a problem in the #limit_validation_errors or #required in multi steps forms that should be addressed or is this another example of the uniqueness of migrate?

In this case I agree. However, is this exposing a problem in the #limit_validation_errors or #required in multi steps forms that should be addressed or is this another example of the uniqueness of migrate?

It's just that we're using #states here - if we were using #ajax we could rebuild the form and set the #required as appropriate based on the user's selections

I'm inclined to won't fix too

Respectfully disagree, '#states' was introduced into the form in 8.5.x fcb7dd9 whereas the @todo about limitation errors was added when the form was still part of the contrib module Migrate Upgrade, #2647470: Write tests.

Ok, no worries

Does this work as an alternative?

Adds html5 required attribute

Probably helps if I do it properly

I have manually tested the patch in #33 and #34 and they do not change the behavior of the form. And the second one causes a warning

Notice: Undefined index: #type in Drupal\Core\Form\FormHelper::processStates() (line 208 of core/lib/Drupal/Core/Form/FormHelper.php).

Drupal\Core\Form\FormHelper::processStates(Array) (Line: 398)
Drupal\Core\Render\Renderer->doRender(Array) (Line: 444)
Drupal\Core\Render\Renderer->doRender(Array) (Line: 444)
Drupal\Core\Render\Renderer->doRender(Array) (Line: 444)
Drupal\Core\Render\Renderer->doRender(Array) (Line: 444)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 200)
Drupal\Core\Render\Renderer->render(Array, ) (Line: 241)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 573)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 242)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object, Object) (Line: 132)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object)
call_user_func(Array, Object, 'kernel.view', Object) (Line: 123)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'kernel.view') (Line: 163)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 80)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 706)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

I really appreciate that larowlan took the time to make a patch for this. It was fun to test as learn a bit more about forms.

In summary, this is a won't fix, well sort of. What is left to do is to remove any @todo pointing to this issue. And I've made the patch.

Removing unneeded tags.

@quietone, I have applied ur patch on clean install of drupal 9.1x
1. Normally if user clicks on review upgrade then js pop is shown for required fields which is fine
2. If by mistake i add space to database name, and click on review upgrade i see the error message as quoted in issue description, can we add a trim to this field if only space is added.

@samiullah, I am not able to reproduce that result. I can't get an access denied for a database field that is empty or only contains spaces. Please provide complete steps to reproduce.

@quietone

Steps to replicate

1. Install standard drupal 9.1x
2. Enable migrate_drupal_ui
3. Go to /upgrade
4. Click continue to get to upgrade/credentials
5. Apply the latest patch
6. Clear the cache
7. Disable browser JS
8. In Database Name field enter space only
9. Click on review upgrade
10. Observe the error message

@samiullah, thank you for the steps to reproduce. I do get the same results as in #38 with javascript disabled. The only difference is that I did not apply the latest patch because it only changes some comments, it does not change any code. Without javascript the form is working well enough and that agrees with the comment in #28 that "without JS it's just that fields that should be marked as required are not but the error that's display is not bad at all".

Your second point about trimming the value entered in the database field needs is outside the scope of this one. Please make a new issue for that. Thanks.

@quietone I have created a new issue for trimming spaces only https://www.drupal.org/project/drupal/issues/3162323

Another straightforward review. I agree to not fix and remove the todos. I've adjusted the issue title,

IMO, the status should already be NW if the issue summary needs an update.

The patch in #37 needs a reroll, which certainly means the status should be NW. I am adding the appropriate issue tags.

Re-rolled the patch in #37. Retaining status as "Needs Work" because the issue summary needs an update as mentioned in #44...

Thank you!

Updated the IS

Green patch and updated IS, this is ready to go.