[#34277] - [4.0] New form field validation rules for paths to existing folders or files
- Closed
- 30 May 2021
- Medium
- Build: 4.0-dev
- # 34277
- Diff
- richard67:4.0-dev-new-validation-rules-files-and-folers-exist
User tests: Successful: Unsuccessful:
Pull Request for Issue #34262 and possibly others.
Summary of Changes
Currently, when you enter a valid path to a not existing file or folder, e.g. the path to files and images in the com_media options or the attachment selection in com_mails, you can save the form without any notice that the file or folder doesn't exist.
See issue #34262 for the filelist field or the discussion in PR #34233 for the attachment folder field in com_mails options, the discussions or issues related to com_media folders I have to search for, but I'm sure there are some.
This pull request (PR) fixes that by adding two new validation rules, which can be used with text fields or filelist fields or folderlist fields.
- "FilePathExistsRule" for paths to files
- "FolderPathExistsRule" for paths to folders
Both rules extend from rule "FilePathRule" and first call the check method of this parent.
After that, the file or folder is checked for existence if the field is required.
This PR adds use of these rules at following places, other places can be done in later PR's:
- "Path to Files Folder" and "Path to Images Folder" text fields in Media Manager (com_media) options
- "Attachments Folder" text field in Mail Templates (com_mails) options
- "File" filelist field for an attachment in a Mail Template
Testing Instructions
The test starts on an installation of current 4.0-dev branch or latest 4.0 nightly build without the patch of this PR applied.
All actions are performed in backend.
-
Go to media manager options.
-
In one or both of the fields "Path to Files Folder" and "Path to Images Folder", enter a path which is valid e.g. regarding forbidden characters but which doesn't exist below the Joomla root, e.g. "gaga" or "images/gaga".
-
Save and close.
Result: Changes are saved, green success message shown. -
Go to "System -> Templates -> Mail Templates" and edit the options.
-
In field "Attachments Folder", enter a path which is valid e.g. regarding forbidden characters but which doesn't exist below the Joomla root, e.g. "gaga" or "images/gaga".
-
Save the change.
Result: Changes are saved, green success message shown. -
Change field "Attachments Folder" back to a path which is valid and exists and which contains files which can be attached to email, e.g. "images", and save and close.
-
Go to "System -> Templates -> Mail Templates" and edit one of the mail templates, e.g. "Global Configuration: Test Mail".
-
At the bottom in section "Attachments", add an attachment but don't select a file, or select a file and modify the value to an invalid one in the DOM using your browser's developer tools like it is described in issue #34262 .
-
Save the change.
Result: Changes are saved, green success message shown. -
Apply the patch of this PR.
-
Go to media manager options which have still the path to not existing folder(s) from step 2.
-
Without making any changes, use the "Save" button.
Result: The form is not saved, you get an alert about invalid field. -
Change the path(s) so they are valid and point to existing folder(s), and save and close.
-
Edit again the mail template you have edited in step 8 but don't make changes.
-
Use the "Save" button.
Result: The form is not saved, you get an alert about invalid field. -
Select a file for the attachment, and use again the "Save" button.
Result: The form is saved.
Actual result BEFORE applying this Pull Request
- "File" filelist field for an attachment in a Mail Template: See issue #34262 .
- "Path to Files Folder" and "Path to Images Folder" text fields in Media Manager (com_media) options and "Attachments Folder" text field in Mail Templates (com_mails) options: See section "What this PR does not fix" in PR #34233 .
Expected result AFTER applying this Pull Request
Will be added soon.
Other information
The "FilePathExistsRule" doesn't work with fields of type "filelist" if the "stripext" element of that field is set to "true".
Documentation Changes Required
Documentation of form field validation rules in J4 needs to be extended by the two new rules, including what's mentioned in the previous section "Other information".
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration com_media Libraries |
| Title |
|
||||||
| Labels |
Added:
?
|
||
| Category | Administration com_media Libraries | ⇒ | Administration com_media Libraries Unit Tests |
Why do you have to add "validate="folderPathExists" to the xml ? What is the usecase where you would ever not want to validate it
Why do you have to add "validate="folderPathExists" to the xml ? What is the usecase where you would ever not want to validate it
@brianteeman We already do it that way for the images and files paths of the media manager with the rule FilePathRule, and I think in this context this question has already been discussed.
Just because it was done before doesnt make it right
it absolutely is in the scope of this pr. You are adding a validation rule - great. But why is it optional? Thats what this pr is doing and its making it off by default
@brianteeman The scope of this PR is to improve existing mechanisms, not to establish new ones. A form field used for folders cannot know if we want to enter the path to an existing folder or to a folder which shall be created. Being able to chose different validation rules makes it possible to use the same field for both purposes. Making it possible to define that in the XML enables 3rd party extension developers to use own rules or no rules.
| Labels |
Added:
?
|
||
Unit tests have been added. Ready for testing.
Why do you have to add "validate="folderPathExists" to the xml ? What is the usecase where you would ever not want to validate it
How else should it be?
It is how validation works in Joomla in general. The PR is good for what it doing.
| Labels |
Added:
?
Removed: ? |
||
| Labels |
Added:
?
Removed: ? |
||
Ready for testing. I think.
Just thinking that this might be a bit over engineered with the check for the directory element, because that’s only needed for filelist and folderlist fields where the files or folders have to exist for getting into the list field, so a validation with the options rule should be enough for these 2 kinds of fields, and the new validation rules added by this PR here would be needed only for text fields. It i have to check that first to be sure.
@wilsonge What’s your opinion on this?
On the other side, a filelist or folderlist field can contain additional values, like "none selected", and the new validation rules would make sure that something was selected, while the options rule wouldn't.
Thinking over if more, we should use text fields for files or folders only where we want to create new files or folders but not for selecting files or folders, and for creating the existing FilePathRule is enough.
Selecting files or folders should be done either with filelist and folderlist fields or in case of files with a media field, and in case of folders it would be nice to have a kind of media folder field which works like the media field, you can browse and select a folder.
There is PR #29721 which does that for the folders of the file system plugin, but it has an issue with the "exclude" element being checked as regular expression and so not working as expected.
So I am thinking about closing this PR and make another one for using validation="options" for filelist and folderlist fields where it is missing, and maybe help with fixing PR #29721 or another way not to use text fields for choosing existing files or folders.
| Labels |
Added:
?
Removed: ? |
||
I have tested this item
Additional tests
- Remove current path and save - saved with default value - good
- Remove current path and replace with two spaces - saved - bad
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34277.
@brianteeman Could you explain your additional test more detailed? I don't understand what you mean with "Remove current path". Which field was it, and did you manipulate the value in DOM, or the "directory" element of the filelist field for the attachment?
@brianteeman I can confirm your finding, but I also can confirm it on a 4.0-dev branch without this PR being applied so the field still uses the FilePathRule. So it's not caused by or specific to the new rules added by this PR.
The new rule is supposed to ensure that only a valid path is saved. But as shown you can save invalid paths
@brianteeman The same applies to the old FilePathRule, and this is where the issue is located.
@brianteeman But thanks for your review suggestions and the finding. I hope I did not waste too much of your time with this PR in case if I decide to close it due to my thoughts in my previous comments.
@brianteeman The problem with and without this PR is that Path::check or Path::clean result in JPATH_ROOT being used if the value is empty or consists of spaces only, and that's valid, and so validation passes. That's nothing I can change with this PR except if I change the new rules to allow only files in folder below the Joomla root but not in the Joomla root, and not allow empty value or spaces which result in the Joomla root being used, in opposite to the existing FilePathRules which allows that.
@brianteeman Final one: For files we are ok because they can't be empty, and for folder we should not allow the Joomla root. The old FilePathRule was not aware of if we want to check a file or a folder, and so you are right: It has to be fixed here in the PR for the FolderPathExists" rule.
Hmm, it would need to set required="true" for the field to not allow empty field or spaces. But that would mean the form doesn't save and reset to the default when using an empty field.
Why an empty field is set to the default value and a field with spaces only is not when saving, that's another issue which can't really be solved with this PR here.
| Labels |
Added:
?
Removed: ? |
||
@brianteeman Ok, I've fixed it finally.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-05-30 12:26:05 |
| Closed_By | ⇒ | richard67 | |
| Labels |
Added:
?
Removed: ? |
||
I will add some new unit tests soon.