[#37261] - [4.1] Change a conditional to check on the first character
- Closed
- 13 Mar 2022
- Medium
- Build: 4.1-dev
- # 37261
- Diff
- dgrammatiko:patch-15
User tests: Successful: Unsuccessful:
Pull Request for Issue #37241 (comment) .
Summary of Changes
- Change the conditional from checking if a folder exists to determine if it's a relative or absolute path to checking the first character
Testing Instructions
- Goto administrator/index.php?option=com_languages&view=language&layout=edit&lang_id=1 and check that the selector for the language images still works
- Edit to something like (provide a folder outside of Joomla's root that has some files)
joomla-cms/administrator/components/com_languages/forms/language.xml
Lines 47 to 59 in cb07dbd
<field
name="image"
type="filelist"
label="COM_LANGUAGES_FIELD_IMAGE_LABEL"
stripext="1"
directory="/Users/dgrammatiko/Downloads"
hide_none="1"
hide_default="1"
>
<option value="">JNONE</option>
</field>Actual result BEFORE applying this Pull Request
Expected result AFTER applying this Pull Request
- Relative path:
- Absolute path:
Documentation Changes Required
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
| Labels |
Added:
?
|
||
@richard67 done, but I think what @HLeithner was suggesting was to not rely on the folder check for deciding if a folder is absolute or relative. Anyways I'll leave that to you...
@richard67 done, but I think what @HLeithner was suggesting was to not rely on the folder check for deciding if a folder is absolute or relative. Anyways I'll leave that to you...
@dgrammatiko Yes, I understood that, so I thought it should be changed in the FolderlistField.php, too. Now you did it vice versa :-)
For testing instructions, the folder list field could be tested e.g. here:
@richard67 I think we need a quick test if an absolute folder works on windows, otherwise it's obvious that the choice of is_dir for the conditional was done to satisfy that OS
@richard67 I think we need a quick test if an absolute folder works on windows, otherwise it's obvious that the choice of
is_dirfor the conditional was done to satisfy that OS
@dgrammatiko Yes, I know. Unfortunately my Windows environment is not ready yet. Will check later tonight.
yes sorry my fault thanks richard
5 locs, 50+ commits...
5 locs, 50+ commits...
@dgrammatiko True team work
I don't wanna ruin the party, but ... isn't it some kind of a b/c break? In past, if someone did not have any base dir restriction which blocked that, the check e.g. for /somefolder existing in the root failed, so we prepended JPATH_ROOT and treated it like a relative folder, and if that existed all was fine from the user's point of view. Now this will not work anymore.
@dgrammatiko @HLeithner There could be indeed a problem on Windows when using a drive letter. When the htdocs folder and the folder used in the absolute path are on the same drive C:, an absolute path like "/Users/Richard/Pictures" works. But "C:/Users/Richard/Pictures" doesn't work anymore because that absolute path starts with a drive letter and not a slash.
Without this PR, an absolute path with a drive letter like "C:/Users/Richard/Pictures" works.
Without this PR, an absolute path with a drive letter like "C:/Users/Richard/Pictures" works.
Well, there we have our answer why the conditional was using is_dir
I think this one served it's purpose, so I'm closing
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-03-13 21:25:21 |
| Closed_By | ⇒ | dgrammatiko |
@dgrammatiko There is a similar field for folder list where we have slightly different code:
https://github.com/joomla/joomla-cms/blob/4.1-dev/libraries/src/Form/Field/FolderlistField.php#L188-L202
But from my point of view those 2 fields FilelistField and FolderlistField should behave in the same way regarding the "directory" option.
Could you change it there, too, in the same way as in this PR here?
@HLeithner Or should we do it differently?