Problem/Motivation

From https://www.drupal.org/node/2281691#comment-10286177...

webchick:

OBSERVATION There doesn't seem to be any validation around the file path field? Or at least I wasn't able to trigger it despite typing in garbage values for both local filepath and http:// address (that could've been due to "migrate already happened" errors tho, see below... but in any case we should probably validate the form first before we proceed).

mikeryan:

So, file_exists() on a local filepath and... get_headers() on an HTTP URL? Or, anything with ://, no reason you couldn't put the files on ftp://...

Current screenshot from #30 showing error when tested with Drupal 7 source and both of the file fields have errors

Proposed resolution

Add the validation. Since this will add new error messages, also make the messages look nice, or at least not terrible (see screenshot in #30). This involves fine code refactoring of all error printing on the page. There is another issue that is working on improving the display of error messages where that part can be explored in more detail #2864849: Improve migrate UI database credential form

Here is a screenshot made from patch in #85

Remaining tasks

Patch
Screenshots
review

commit

User interface changes

Yes, possible new error messages after submit of /upgrade/credentials

CommentFileSizeAuthor
#87 errors.png13.15 KBmaxocub
#85 interdiff.txt2.89 KBquietone
#85 2562845-85.patch9.62 KBquietone
#84 errors.png123.01 KBmaxocub
#82 interdiff.txt3.6 KBquietone
#82 2562845-82.patch10.76 KBquietone
#78 interdiff.txt622 bytesquietone
#78 2562845-78.patch10.09 KBquietone
#75 interdiff.txt1.5 KBquietone
#75 2562845-75.patch10.09 KBquietone
#73 interdiff.txt665 bytesquietone
#73 2562845-73.patch10.19 KBquietone
#71 interdiff.txt4.49 KBquietone
#71 2562845-71.patch10.2 KBquietone
#70 interdiff.txt1.37 KBquietone
#70 2562845-70.patch9.22 KBquietone
#68 2562845-68.patch9.19 KBquietone
#68 interdiff.txt1.29 KBquietone
#66 Selection_008.png52.84 KBquietone
#66 Selection_009.png27.71 KBquietone
#66 Selection_008.png52.84 KBquietone
#66 interdiff.txt4.43 KBquietone
#66 2562845-66.patch8.43 KBquietone
#63 interdiff.txt758 bytesquietone
#63 2562845-63.patch7.28 KBquietone
#61 interdiff.txt2.21 KBquietone
#61 2562845-61.patch7.28 KBquietone
#56 Screen Shot 2018-04-09 at 15.41.19.png34.4 KBrakesh.gectcr
#56 interdiff-2562845-44-56.txt1.42 KBrakesh.gectcr
#56 2562845-56.patch7.36 KBrakesh.gectcr
#55 Screen Shot 2018-04-09 at 15.39.13.png35.93 KBrakesh.gectcr
#54 interdiff-2562845-44-54.txt1.89 KBrakesh.gectcr
#54 2562845-54.patch7.58 KBrakesh.gectcr
#49 Selection_009.png18.37 KBquietone
#49 Selection_010.png9.96 KBquietone
#46 Screen Shot 2018-03-23 at 17.45.53.png16.82 KBrakesh.gectcr
#44 interdiff.txt1.1 KBquietone
#44 2562845-44.patch7.32 KBquietone
#42 interdiff.txt2.26 KBquietone
#42 2562845-42.patch7.41 KBquietone
#40 interdiff.txt2.21 KBquietone
#40 2562845-40.patch7.42 KBquietone
#38 interdiff.txt7.96 KBquietone
#38 2562845-38.patch8.17 KBquietone
#37 Screen Shot 2018-03-22 at 13.39.37.png20.43 KBrakesh.gectcr
#37 2562845-37.patch6.5 KBrakesh.gectcr
#37 interdiff-2562845-35-37.txt1.84 KBrakesh.gectcr
#36 Screenshot from 2018-03-22 10-45-57.png12.24 KBquietone
#35 interdiff-2562845-32-35.txt612 bytesrakesh.gectcr
#35 2562845-35.patch4.84 KBrakesh.gectcr
#33 Screen Shot 2018-03-21 at 16.23.45.png30.87 KBrakesh.gectcr
#32 interdiff-2562845-30-32.txt2.38 KBrakesh.gectcr
#32 2562845-32.patch4.83 KBrakesh.gectcr
#30 Screenshot from 2018-03-21 20-18-11.png9.69 KBquietone
#30 Screenshot from 2018-03-21 20-11-04.png19.76 KBquietone
#30 interdiff.txt1.72 KBquietone
#30 2562845-30.patch4.15 KBquietone
#26 interdiff.txt4.3 KBquietone
#26 2562845-26.patch3.86 KBquietone
#24 2562845-24.patch1.31 KBquietone
#15 2562845-15.patch1.43 KBiMiksu
#5 2562845-5.patch2.02 KBjofitz
#3 Selection_008.png8.3 KBquietone
#3 2562845-3.patch1.6 KBquietone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeryan created an issue. See original summary.

mikeryan’s picture

Issue tags: +Barcelona2015, +Novice

Tagging for Barcelona sprinting.

quietone’s picture

FileSize
1.6 KB
8.3 KB

Here's a start. One thing that needs improvement is the error messaging, which I hope is obvious from the image.

error messages

quietone’s picture

Status: Active » Needs review
jofitz’s picture

FileSize
2.02 KB

I have made a slight change to the patch by @quietone because validation of source_base_path is unwanted when re-running a migration.

drupalfox’s picture

Hi All

I have tested the patch and its working fine.

Appreciate your help on this issue.

drupalfox’s picture

Assigned: Unassigned » drupalfox
mikeryan’s picture

Status: Needs review » Needs work
Issue tags: -Barcelona2015, -Novice
+++ b/src/Form/MigrateUpgradeForm.php
@@ -824,10 +825,34 @@ class MigrateUpgradeForm extends FormBase implements ConfirmFormInterface {
+        $ch = curl_init();

Hmmm - can we be sure cURL is present? Take note of aggregator_requirements()... We could require it in hook_requirements(), but is it worth making the requirement just for validation? Is there a means of validation using only PHP functions we can guarantee are present?

xjm’s picture

Project: Migrate Upgrade » Drupal core
Version: 8.x-1.x-dev » 8.1.x-dev
Component: User interface » migration system

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

iMiksu’s picture

Assigned: drupalfox » Unassigned
Issue tags: +Needs reroll

Needs reroll. I assume drupalfox isn't working on this.

iMiksu’s picture

Issue tags: +DCampBaltics

Tagging for code sprint.

iMiksu’s picture

Assigned: Unassigned » iMiksu
iMiksu’s picture

Assigned: iMiksu » Unassigned
Issue tags: -Needs reroll +Needs tests
FileSize
1.43 KB

From #5 comment patch:

@@ -807,6 +807,7 @@ class MigrateUpgradeForm extends FormBase implements ConfirmFormInterface {
       '#limit_validation_errors' => [
         ['driver'],
         [$default_driver],
+        ['source_base_path'],
       ],
       '#validate' => ['::validateCredentialForm'],
       '#submit' => ['::submitCredentialForm'],

I left this out from the reroll because there's no limit validation errors used in HEAD now. Is that OK?

   public function validateCredentialForm(array &$form, FormStateInterface $form_state) {
     // Skip if rollback was chosen.
-    if ($form_state->getValue('upgrade_option') == static::MIGRATE_UPGRADE_ROLLBACK) {
+    $upgrade_option = $form_state->getValue('upgrade_option');
+    if ($upgrade_option == static::MIGRATE_UPGRADE_ROLLBACK) {
       return;
     }

There is no $upgrade_option anymore present in validation.

I did:

  • Added comment prior to file path validation: Check that source base path is either a valid local file path or valid remote file path.
  • Used \Drupal::httpClient() instead curl
  • EDIT: Used $this->t() instead t()

Needs tests.

iMiksu’s picture

Status: Needs work » Needs review

I'm setting needs review to look that existing tests wont fail..

The last submitted patch, 5: 2562845-5.patch, failed testing.

mikeryan’s picture

Status: Needs review » Needs work

Tested locally - works fine with both good and bad local files, and good domains, but I get a WSOD with http://www.afakedomain.name/. dblog shows

GuzzleHttp\Exception\ConnectException: cURL error 6: Couldn't resolve host 'www.afakedomain.name' (see http://curl.haxx.se/libcurl/c/libcurl-errors.html) in GuzzleHttp\Handler\CurlFactory::createRejection() (line 186 of /Users/mikeryan/Sites/8.3.x/vendor/guzzlehttp/guzzle/src/Handler/CurlFactory.php).

So, we need to be prepared to catch Guzzle exceptions.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhodgdon’s picture

I ran into something related to this today when testing migrate from a d6 site. The file directory I entered ended in / and Migrate could not find a single file there, because the files it was looking for had // in the middle of their paths. Ooops.

So I think that this should also validate that the file path doesn't end in / because otherwise all files will not be found.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

Issue tags: +Migrate UI
Related issues: +#2647470: Write tests

Add tag and related issue

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

Status: Needs work » Needs review
FileSize
1.31 KB

Start with a reroll.

Status: Needs review » Needs work

The last submitted patch, 24: 2562845-24.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.86 KB
4.3 KB

This should fix the failing test and includes new tests which handle Guzzle Exceptions as #18 pointed out needed to be done.

quietone’s picture

Title: Validate the file path field before executing migration? » Validate the file path fields before executing migration

Change title to a statment

quietone’s picture

Issue tags: +Needs manual testing

I reckon this would benefit from some manual testing.

heddn’s picture

+++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
@@ -224,6 +225,41 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+          catch (ClientException $e) {

Is ClientException the only exception we want to catch? I thought that Guzzle threw a whole bunch of different exceptions?

quietone’s picture

Yes, it does. The full list is at http://docs.guzzlephp.org/en/latest/quickstart.html#exceptions. All the exceptions extend from TransferException so that is now used. That meant a change to the displayed message since there may not be a status code.

While testing this it is exposing problems with the displayed error messages such as seen in this screenshot. So I think this should look into that and tidy up the way the error message are displayed.

heddn’s picture

Status: Needs review » Needs work

Looking good. Back to NW for the additional tidying mentioned in #30.

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
4.83 KB
2.38 KB
rakesh.gectcr’s picture


Looks more tidier than previous.

heddn’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
@@ -224,6 +225,69 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+            // Make the request. Guzzle throws an exception for anything but 200.

Nit: 80 chars.

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
4.84 KB
612 bytes

:)

quietone’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
12.24 KB

Thanks rakesh.gectcr.

I did some testing and found a way to get a not so nice display of the error messages which also doesn't into the title line 'Resolve the issues ....' It can be reproduced by selecting the wrong drupal legacy version. Only have time to point this out now.

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
1.84 KB
6.5 KB
20.43 KB

:), here we go...

quietone’s picture

There a fair bit of duplicated code here to create the form element that can be in a helper method. I've don't that here and used it for all errors detected in validate.

Status: Needs review » Needs work

The last submitted patch, 38: 2562845-38.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
7.42 KB
2.21 KB

Correct the error message in the test.

Status: Needs review » Needs work

The last submitted patch, 40: 2562845-40.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
7.41 KB
2.26 KB

Make the error msgs the same and append any Guzzle error.

Status: Needs review » Needs work

The last submitted patch, 42: 2562845-42.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
7.32 KB
1.1 KB

Still got the expected text string wrong.

heddn’s picture

Can we see an updated screenshot?

+++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
@@ -192,24 +198,24 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+      foreach ($driver_errors as $name => $message) {
+        $errors[$name] = $message;
       }

Nit: seems like a good solution for array_map.

rakesh.gectcr’s picture

In array_map, I am not sure about the key
$form_state->setErrorByName($name);

heddn’s picture

Title: Validate the file path fields before executing migration » Validat file path and db on Credential form
Status: Needs review » Needs work
Issue tags: -Needs manual testing

I like what we are doing here. My only concern at this point is that we are doing more than what the title of this issue says. Here's an attempt at re-titling to more closely align with what is ocuring. All that is left is a quick update of the IS and this can move on to RTBC.

heddn’s picture

quietone’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
9.96 KB
18.37 KB

Updated IS
Applied the patch and made new screenshots

quietone’s picture

Title: Validat file path and db on Credential form » Validate file path and db on Credential form

Fix typo in title

heddn’s picture

Title: Validate file path and db on Credential form » Validate file path on Credential form
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

All feedback addressed.

rakesh.gectcr’s picture

+1

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
    @@ -177,7 +183,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +    $errors = [];
    
    @@ -192,24 +198,24 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    -    if ($errors = $drivers[$driver]->validateDatabaseSettings($database)) {
    -      foreach ($errors as $name => $message) {
    -        $form_state->setErrorByName($name, $message);
    +    if ($driver_errors = $drivers[$driver]->validateDatabaseSettings($database)) {
    +      foreach ($driver_errors as $name => $message) {
    +        $errors[$name] = $message;
           }
    -      return;
         }
    

    I think we can do
    $errors = $drivers[$driver]->validateDatabaseSettings($database) and remove all of the complexity here.

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
    @@ -192,24 +198,24 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
         // Validate the driver settings and just end here if we have any issues.
    

    I think this comment can be removed (1) because it is now incorrect (2) because once we do the simplification suggested it is pretty redundant.

  3. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
    @@ -217,13 +223,69 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +      $msg = $e->getMessage();
    +      $errors[$database['driver']] = $msg;
    

    Should this be $errors['driver' . '][0'] = $e->getMessage();? Also I think $msg is not necessary.

  4. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
    @@ -217,13 +223,69 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +    $form_state->setErrorByName($name . '][0', $this->renderer->renderPlain($list));
    

    I'm surprised we need to add the '][0' here. I think it should be in $name already.

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
7.58 KB
1.89 KB
rakesh.gectcr’s picture

Status: Needs review » Needs work
FileSize
35.93 KB

@alexpott
We needed the $form_state->setErrorByName($name . '][0', $this->renderer->renderPlain($list));
Otherewise its not displaying we getting error like following

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
7.36 KB
1.42 KB
34.4 KB

manual test screenshot

rakesh.gectcr’s picture

Had a chat with Alex pott in slack about #55

Alex Pott [3:48 PM]
@rakeshjames the thing is some of the keys have '][0' added in already
@rakeshjames imo the name should be set correctly in $error and not changed in buildErrorList

The last submitted patch, 54: 2562845-54.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 56: 2562845-56.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

  1. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
    @@ -190,26 +195,21 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +    ¶
    

    space character on empty line

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
    @@ -190,26 +195,21 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +        $errors['driver' . '][0'] = $this->t('Source database does not contain a recognizable Drupal version.');
    ...
    +        $errors['driver' . '][0'] = $this->t('Source database is Drupal version @version but version @selected was selected.',
    
    @@ -217,15 +217,70 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +      $errors['driver' . '][0'] = $e->getMessage();
    

    The key on these was '$database['driver']' not just 'driver' which is used as the element name. Doesn't that need to be restored?

quietone’s picture

Status: Needs work » Needs review
FileSize
7.28 KB
2.21 KB

Fixes for the points in #60. However, instead of restoring $database['driver'] this uses $driver which is the equivalent because a few lines above there is $database['driver'] = $driver;

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
@@ -217,13 +217,68 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+      $errors[$driver . '][0'] = $e->getMessage();
...
+      $form_state->setErrorByName($name);
...
+    $name = current(array_keys($errors));
+    $form_state->setErrorByName($name . '][0', $this->renderer->renderPlain($list));

This still looks problematic to me. I think the last form_state->setErrorByName doesn't need the . '][0'. If it does then the first form_state->setErrorByName() in buildErrorList() does.

quietone’s picture

Status: Needs work » Needs review
FileSize
7.28 KB
758 bytes

@alexpott, thx. What is problematic about it? I

Although I am not sure I follow what you are suggesting, but here is another try.

phenaproxima’s picture

Status: Needs review » Needs work

Nice catch and nice patch.

  1. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
    @@ -191,25 +196,20 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +        $errors[$driver . '][0'] = $this->t('Source database does not contain a recognizable Drupal version.');
    

    This is very jarring to read. Can we do $errors["$driver][0"] instead? Or better yet, create an $error_key variable for that, so we can just do $errors[$error_key]?

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
    @@ -217,13 +217,68 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +          try {
    +            \Drupal::httpClient()->head($source);
    +          }
    

    Can we inject the HTTP client?

  3. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
    @@ -217,13 +217,68 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +    $msg_singular = $this->t('Resolve the issue below to continue the upgrade.');
    +    $msg_plural = $this->t('Resolve the issues below to continue the upgrade.');
    +    $msg = $this->formatPlural(count($errors), $msg_singular, $msg_plural);
    

    I'm not sure we need to pass translated strings to formatPlural().

jhodgdon’s picture

Regarding formatPlural, you should definitely *not* pass in translated variables. They need to be put into formatPlural() as literal untranslated strings, so that other languages can translate the plural forms correctly. The way this is done, many languages who have != 2 plural forms will not be able to translate this at all.

A separate question though is whether a plural form is even necessary here. The message could just be "Resolve all issues below to continue the upgrade" and it be fine whatever the count is?

quietone’s picture

Status: Needs work » Needs review
FileSize
8.43 KB
4.43 KB
52.84 KB
27.71 KB
52.84 KB

All items in #53 fixed.

@jhodgdon, thank you. I have implemented you suggestion. Much simpler!

I've uploaded a few screenshots of the errors but I haven't run the associated test locally (it just takes so so long).

Status: Needs review » Needs work

The last submitted patch, 66: 2562845-66.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
1.29 KB
9.19 KB

Now, let's fix the test.

phenaproxima’s picture

+++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
@@ -217,13 +229,65 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+        if (!isset($parsed_source['scheme'])) {
+          if (!file_exists($source) || (!is_dir($source)) || (!is_readable($source))) {
+            $errors[$key] = $msg;
+          }
+        }

Forgive me if I'm wrong, but won't this fail to account for local URIs like public:// or private://? Those will probably break with Guzzle.

quietone’s picture

You are correct. How about this version which uses Utility\UrlHelper::isExternal ?

quietone’s picture

On IRC phenaproxima said he was wondering if this should use '#element_validate'. This patch implements that for the source file paths.

Status: Needs review » Needs work

The last submitted patch, 71: 2562845-71.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
10.19 KB
665 bytes

Spotted the error early and canceled the test but it still ran. Oh well.

phenaproxima’s picture

Status: Needs review » Needs work

I think this is pretty close to ready.

  1. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
    @@ -145,6 +171,9 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      '#element_validate' => [
    +        [$this, 'validatePaths'],
    +      ],
    

    We should use this syntax here:

    '#element_validate' => ['::validatePaths']
    
  2. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
    @@ -217,16 +248,64 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +      else {
    +        if (!file_exists($source) || (!is_dir($source)) || (!is_readable($source))) {
    +          $this->errors[$element['#name']] = $msg;
    +        }
    +      }
    

    This can be combined into a single elseif.

  3. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeExecuteTestBase.php
    @@ -70,16 +70,35 @@ public function testMigrateUpgradeExecute() {
    +      $session->responseContains('Unable to read from Private file directory.');
    

    Nit: Why don't we say "Private files" (plural)? I realize this is out of scope, let's open a follow-up for that.

quietone’s picture

Status: Needs work » Needs review
FileSize
10.09 KB
1.5 KB

1. Hmm, I thought I tried that. Anyway fixed now.
2. Fixed
3. The language used on admin/config/media/file-system is the singular, 'file', as in 'Public file' and 'Private file'.

quietone’s picture

phenaproxima’s picture

+++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
@@ -217,16 +242,62 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+      else if (!file_exists($source) || (!is_dir($source)) || (!is_readable($source))) {

Dang -- one thing. This needs be 'elseif' for coding standards compliance.

Otherwise, this is RTBC. Feel free to mark it as such when this one nitpick is fixed and tests are green :)

quietone’s picture

Arg, I have been making more silly mistakes than usual lately.

quietone’s picture

Status: Needs review » Reviewed & tested by the community

As per #77, if this comes back green I can set it to RTBC. I double checked the patch and it really does change the 'else if' to 'elseif' so I am setting to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
    @@ -217,16 +242,62 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +  protected function buildErrorList(array $errors, FormStateInterface $form_state) {
    +    $items = [];
    +    foreach ($errors as $name => $message) {
    +      $form_state->setErrorByName($name);
    +      $items[] = $message;
    +    }
    +
    +    $list = [
    +      '#title' => $this->t('Resolve all issues below to continue the upgrade.'),
    +      '#theme' => 'item_list',
    +      '#items' => $items,
    +    ];
    +    $name = current(array_keys($errors));
    +    $form_state->setErrorByName($name . '][0', $this->renderer->renderPlain($list));
    

    This function still surprises me. In the loop we do $form_state->setErorByName($name, ...

    And then outside the loop we do
    form_state->setErrorByName($name . '][0', ...

    Why are we adding . '][0'? To me this looks like it assumes the error will be attached to the driver.

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
    @@ -191,25 +219,22 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +    foreach ($drivers[$driver]->validateDatabaseSettings($database) as $key => $value) {
    +      $this->errors[$key] = $value;
         }
    

    This looks a bit odd because this guarantees that $this->errors[] will have something in even if there are no errors.

  3. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
    @@ -217,16 +242,62 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +    foreach ($errors as $name => $message) {
    +      $form_state->setErrorByName($name);
    +      $items[] = $message;
    +    }
    

    This should have a comment about what it is doing and why.

  4. I really wonder what happens here if inline_form_errors is enabled. I suspect this change results in a really bad user experience. I think by rolling all the errors up into 1 we might be breaking Drupal's form error handling system and assumptions made by other modules. But we are hitting a limitation of not being able to set multiple errors against the same element which sucks.

rakesh.gectcr’s picture

Issue tags: +Nwdug_may18

Adding for the sprint weekend

quietone’s picture

Status: Needs work » Needs review
FileSize
10.76 KB
3.6 KB

@rakesh.gectcr, this was on my mind this weekend and wanted to make some progress. Hope the sprint goes well.

1. A comment is added to explain. Is that enough?
2. Fixed. And now there is an if statement so that we don't attempt to make the database connection if validateDatabaseSettings returns an error.
3. Comment added. I've been testing with in-line errors enabled and haven't had a bad user experience. The screenshot was made with in line errors enabled and an error in the prefix and the file directory. With in-line errors enabled there is the additional message stating the number of errors found and links to the fields. Note that I wonder if this method of displaying errors is somewhat based on SiteSettingsForm::getDatabaseErrors().

maxocub’s picture

Assigned: Unassigned » maxocub

Assinging for review

maxocub’s picture

Assigned: maxocub » Unassigned
Status: Needs review » Needs work
FileSize
123.01 KB
  1. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
    @@ -22,16 +25,33 @@ class CredentialForm extends MigrateUpgradeFormBase {
       /**
    +   * The HTTP client to fetch the feed data with.
    +   *
    +   * @var \GuzzleHttp\ClientInterface
    +   */
    +  protected $httpClient;
    

    Feed data?

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
    @@ -194,36 +222,90 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +          $this->errors[$database['driver'] . '][database'] = $this->t('Source database does not contain a recognizable Drupal version.');
    ...
    +        $this->errors[$driver] = $e->getMessage();
    

    Just an idea, instead of $this->errors[$driver], we could do $this->errors[$driver . '][database'], like we do above. This way not all database credentials fields will have an error and we won't need the '][0' part latter.

  3. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
    @@ -194,36 +222,90 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +  protected function buildErrorList(array $errors, Fand weormStateInterface $form_state) {
    +    $items = [];
    +    // To display all the errors found, set the error on the form field name for
    +    // error and add each message to an array for later display.
    +    foreach ($errors as $name => $message) {
    +      $form_state->setErrorByName($name);
    +      $items[] = $message;
         }
    +
    +    $list = [
    +      '#title' => $this->t('Resolve all issues below to continue the upgrade.'),
    +      '#theme' => 'item_list',
    +      '#items' => $items,
    +    ];
    +    $name = current(array_keys($errors));
    +    // Use the field name of the first error as the name for setErrorByName. Add
    +    // '][0' so that all children of the field are included. Only the database
    +    // connection has children and since the errors are generic, except for
    +    // the prefix which is tested in validateDatabaseSettings, we do not have
    +    // any specific key to attach them to; therefore, we just put them in the
    +    // error array with standard numeric keys.
    +    $form_state->setErrorByName($name . '][0', $this->renderer->renderPlain($list));
       }
    

    I'm also puzzled by this function. By setting the errors with setErrorByName($name) and adding the messages later, we end up with weird markup: a list inside a div inside another list (see screenshot). This makes the error icon not line up with the error label. If we do the change above, I think we won't need to do the '][0' business. We could just add the messages with the errors: $this->setErrorByName($name, $messages). If we really want a label, we could use setError() on the entire form and put a h3 label.

quietone’s picture

Status: Needs work » Needs review
FileSize
9.62 KB
2.89 KB

@maxocub, thanks for the review!

1. Fixed
2. Fixed
3. Made the changes suggested. What has been lost in the process is that the errors are not shown as a list with bullet points. And just a note that this may be based on SiteSettingsForm::getDatabaseErrors() which does the "'][0' business". I don't know if it produces the same type of markup and has the error icon offset as well. And it is too late to check now. And too late to make screen shots.

Status: Needs review » Needs work

The last submitted patch, 85: 2562845-85.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

maxocub’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
FileSize
13.15 KB

Thanks @quietone, I like this! It's true that there's no more bullet points, but it's not bad at all, here's a screenshot.

quietone’s picture

alexpott’s picture

Crediting @heddn, @mikeryan, @phenaproxima, @alexpott and @jhodgdon for reviews that were material to the patch.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 175fa94bc8 to 8.6.x and 36730910ba to 8.5.x. Thanks!

Backported to 8.5.x because the migrate ui is still experimental.

diff --git a/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
index 718dadde29..deeebe5987 100644
--- a/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
+++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
@@ -2,7 +2,7 @@
 
 namespace Drupal\migrate_drupal_ui\Form;
 
-use Drupal\Component\Utility;
+use Drupal\Component\Utility\UrlHelper;
 use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\Render\RendererInterface;
 use Drupal\Core\TempStore\PrivateTempStoreFactory;
@@ -266,7 +266,7 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   public function validatePaths($element, FormStateInterface $form_state) {
     if ($source = $element['#value']) {
       $msg = $this->t('Unable to read from @title.', ['@title' => $element['#title']]);
-      if (Utility\UrlHelper::isExternal($source)) {
+      if (UrlHelper::isExternal($source)) {
         try {
           $this->httpClient->head($source);
         }

Fixed coding standard on commit.

  • alexpott committed 175fa94 on 8.6.x
    Issue #2562845 by quietone, rakesh.gectcr, iMiksu, Jo Fitzgerald,...

  • alexpott committed 3673091 on 8.5.x
    Issue #2562845 by quietone, rakesh.gectcr, iMiksu, Jo Fitzgerald,...

Status: Fixed » Closed (fixed)

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