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://...

Members fund testing for the Drupal project. Drupal Association Learn more

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
Jo Fitzgerald’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.