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
Comment | File | Size | Author |
---|---|---|---|
#87 | errors.png | 13.15 KB | maxocub |
#85 | interdiff.txt | 2.89 KB | quietone |
#85 | 2562845-85.patch | 9.62 KB | quietone |
#84 | errors.png | 123.01 KB | maxocub |
#82 | interdiff.txt | 3.6 KB | quietone |
Comments
Comment #2
mikeryanTagging for Barcelona sprinting.
Comment #3
quietone CreditAttribution: quietone commentedHere's a start. One thing that needs improvement is the error messaging, which I hope is obvious from the image.
Comment #4
quietone CreditAttribution: quietone commentedComment #5
jofitz CreditAttribution: jofitz commentedI have made a slight change to the patch by @quietone because validation of
source_base_path
is unwanted when re-running a migration.Comment #6
drupalfox CreditAttribution: drupalfox commentedHi All
I have tested the patch and its working fine.
Appreciate your help on this issue.
Comment #7
drupalfox CreditAttribution: drupalfox commentedComment #8
mikeryanHmmm - 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?
Comment #9
xjmComment #12
iMiksuNeeds reroll. I assume drupalfox isn't working on this.
Comment #13
iMiksuTagging for code sprint.
Comment #14
iMiksuComment #15
iMiksuFrom #5 comment patch:
I left this out from the reroll because there's no limit validation errors used in HEAD now. Is that OK?
There is no
$upgrade_option
anymore present in validation.I did:
\Drupal::httpClient()
instead curl$this->t()
insteadt()
Needs tests.
Comment #16
iMiksuI'm setting needs review to look that existing tests wont fail..
Comment #18
mikeryanTested 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
So, we need to be prepared to catch Guzzle exceptions.
Comment #20
jhodgdonI 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.
Comment #22
quietone CreditAttribution: quietone commentedAdd tag and related issue
Comment #24
quietone CreditAttribution: quietone commentedStart with a reroll.
Comment #26
quietone CreditAttribution: quietone as a volunteer commentedThis should fix the failing test and includes new tests which handle Guzzle Exceptions as #18 pointed out needed to be done.
Comment #27
quietone CreditAttribution: quietone as a volunteer commentedChange title to a statment
Comment #28
quietone CreditAttribution: quietone as a volunteer commentedI reckon this would benefit from some manual testing.
Comment #29
heddnIs ClientException the only exception we want to catch? I thought that Guzzle threw a whole bunch of different exceptions?
Comment #30
quietone CreditAttribution: quietone as a volunteer commentedYes, 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.
Comment #31
heddnLooking good. Back to NW for the additional tidying mentioned in #30.
Comment #32
rakesh.gectcrComment #33
rakesh.gectcrLooks more tidier than previous.
Comment #34
heddnNit: 80 chars.
Comment #35
rakesh.gectcr:)
Comment #36
quietone CreditAttribution: quietone as a volunteer commentedThanks 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.
Comment #37
rakesh.gectcr:), here we go...
Comment #38
quietone CreditAttribution: quietone as a volunteer commentedThere 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.
Comment #40
quietone CreditAttribution: quietone as a volunteer commentedCorrect the error message in the test.
Comment #42
quietone CreditAttribution: quietone as a volunteer commentedMake the error msgs the same and append any Guzzle error.
Comment #44
quietone CreditAttribution: quietone as a volunteer commentedStill got the expected text string wrong.
Comment #45
heddnCan we see an updated screenshot?
Nit: seems like a good solution for array_map.
Comment #46
rakesh.gectcrIn array_map, I am not sure about the key
$form_state->setErrorByName($name);
Comment #47
heddnI 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.
Comment #48
heddnComment #49
quietone CreditAttribution: quietone as a volunteer commentedUpdated IS
Applied the patch and made new screenshots
Comment #50
quietone CreditAttribution: quietone as a volunteer commentedFix typo in title
Comment #51
heddnAll feedback addressed.
Comment #52
rakesh.gectcr+1
Comment #53
alexpottI think we can do
$errors = $drivers[$driver]->validateDatabaseSettings($database)
and remove all of the complexity here.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.
Should this be
$errors['driver' . '][0'] = $e->getMessage();
? Also I think $msg is not necessary.I'm surprised we need to add the
'][0'
here. I think it should be in $name already.Comment #54
rakesh.gectcrComment #55
rakesh.gectcr@alexpott
We needed the
$form_state->setErrorByName($name . '][0', $this->renderer->renderPlain($list));
Otherewise its not displaying we getting error like following
Comment #56
rakesh.gectcrmanual test screenshot
Comment #57
rakesh.gectcrHad a chat with Alex pott in slack about #55
Comment #60
quietone CreditAttribution: quietone as a volunteer commentedspace character on empty line
The key on these was '$database['driver']' not just 'driver' which is used as the element name. Doesn't that need to be restored?
Comment #61
quietone CreditAttribution: quietone as a volunteer commentedFixes 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;
Comment #62
alexpottThis 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.Comment #63
quietone CreditAttribution: quietone as a volunteer commented@alexpott, thx. What is problematic about it? I
Although I am not sure I follow what you are suggesting, but here is another try.
Comment #64
phenaproximaNice catch and nice patch.
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]
?Can we inject the HTTP client?
I'm not sure we need to pass translated strings to formatPlural().
Comment #65
jhodgdonRegarding 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?
Comment #66
quietone CreditAttribution: quietone as a volunteer commentedAll 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).
Comment #68
quietone CreditAttribution: quietone as a volunteer commentedNow, let's fix the test.
Comment #69
phenaproximaForgive 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.
Comment #70
quietone CreditAttribution: quietone as a volunteer commentedYou are correct. How about this version which uses Utility\UrlHelper::isExternal ?
Comment #71
quietone CreditAttribution: quietone as a volunteer commentedOn IRC phenaproxima said he was wondering if this should use '#element_validate'. This patch implements that for the source file paths.
Comment #73
quietone CreditAttribution: quietone as a volunteer commentedSpotted the error early and canceled the test but it still ran. Oh well.
Comment #74
phenaproximaI think this is pretty close to ready.
We should use this syntax here:
This can be combined into a single elseif.
Nit: Why don't we say "Private files" (plural)? I realize this is out of scope, let's open a follow-up for that.
Comment #75
quietone CreditAttribution: quietone as a volunteer commented1. 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'.
Comment #76
quietone CreditAttribution: quietone as a volunteer commentedFollow up made #2962594: Make source file labels consistent on Credential Form
Comment #77
phenaproximaDang -- 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 :)
Comment #78
quietone CreditAttribution: quietone as a volunteer commentedArg, I have been making more silly mistakes than usual lately.
Comment #79
quietone CreditAttribution: quietone as a volunteer commentedAs 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.
Comment #80
alexpottThis 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.This looks a bit odd because this guarantees that $this->errors[] will have something in even if there are no errors.
This should have a comment about what it is doing and why.
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.
Comment #81
rakesh.gectcrAdding for the sprint weekend
Comment #82
quietone CreditAttribution: quietone as a volunteer commented@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().
Comment #83
maxocub CreditAttribution: maxocub as a volunteer commentedAssinging for review
Comment #84
maxocub CreditAttribution: maxocub as a volunteer commentedFeed data?
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.
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.
Comment #85
quietone CreditAttribution: quietone as a volunteer commented@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.
Comment #87
maxocub CreditAttribution: maxocub as a volunteer commentedThanks @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.
Comment #88
quietone CreditAttribution: quietone as a volunteer commentedThis is blocking #2679835: Improve exception handling in Migrate UI and #2864849: Improve migrate UI database credential form
Comment #89
alexpottCrediting @heddn, @mikeryan, @phenaproxima, @alexpott and @jhodgdon for reviews that were material to the patch.
Comment #90
alexpottCommitted 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.
Fixed coding standard on commit.