Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix folderlist on Windows-based web servers #12841

Closed
wants to merge 2 commits into from

Conversation

mgilkes
Copy link
Contributor

@mgilkes mgilkes commented Nov 8, 2016

Pull Request for Issue # .

Summary of Changes

On line 219, the root folder path is trimmed on the end of both back and forward slashes, and the resulting folder is trimmed of both back and forward slashes.

Testing Instructions

The changes address the issue of folderlist on Windows-based web servers. You can test this on IIS or WAMP, just access any module (or plugin) that uses folderlist, and you will see "C:\wamp\www/images" or "C:\inetpub\wwwroot/images", when it should only show as "images".

Since there are no built-in extensions that use folder list, you can add the following field to any module's xml file to test it:

<field name="test_field" type="folderlist" default="images" label="TEST_LABEL" directory="" description="TEST_DESC" hide_none="true" hide_default="true" exclude="^[Aa]dministrator$|^[Cc]ache$|^[Cc]omponents$|^[Cc]li$|^[Ii]ncludes$|^[Ll]anguage$|^[Ll]ibraries$|^[Ll]ogs$|^[Mm]odules$|^[Pp]lugins$|^[Tt]emplates$|^[Xx]mlrpc$" />

Documentation Changes Required

On non-windows-based web servers, the current code works fine, but on windows-based web servers, like IIS or WAMP, the folderlist options will look like "C:\wamp\www/images" or "C:\inetpub\wwwroot/images", when it should only show as "images". This messes up any extension that uses folderlist on windows-based web servers.

I propose to change line 219 from:
$folder = trim(str_replace($path, '', $folder), '/');
to:
$folder = trim(str_replace(rtrim($path, '/\\'), '', $folder), '/\\');

This essentially accommodates Windows systems to make the folderlist work as it should.

On non-windows-based web servers, the current code works fine, but on windows-based web servers, like IIS or WAMP, the folderlist options will look like "C:\wamp\www\/images" or "C:\inetpub\wwwroot\/images", when it should only show as "images". This messes up any extension that uses folderlist on windows-based web servers. 

I propose to change line 219 from:
$folder = trim(str_replace($path,  '', $folder), '/');
to:
$folder = trim(str_replace(rtrim($path, '/\\'), '', $folder), '/\\');

This essentially accommodates Windows systems to make the folderlist work as it should.
@brianteeman
Copy link
Contributor

Is this the same as #11524 and #11288 ??

@mgilkes
Copy link
Contributor Author

mgilkes commented Nov 9, 2016

This issue still exists in Joomla 3.6.4. My solution is different from those proposed. One of the problems with the current code is that the $path variable string actually has a trailing slash, which is why the folder root path is not removed by the str_replace on line 219. It should be trimmed.

@ggppdk
Copy link
Contributor

ggppdk commented Nov 11, 2016

I have tested this item 🔴 unsuccessfully on 972fa74

This PR does not work in all cases

If the original folder has "/" then JFolder will "clean" it thus replace the "/" with "" (windows seperator)
in that case the str_replace to remove the given original folder and make subpath relative, will fail


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12841.

@mgilkes
Copy link
Contributor Author

mgilkes commented Nov 11, 2016

@ggppdk I don't quite understand your comment. When you say "original folder" are you referring to the value of $path or $folder (just before line 219)?

Because $path is the root path of the folder list, and $folder is the full path of a subfolder in the folder list, the purpose of line 219 is to remove the root path from the full path, and leave only the subfolder, without any slashes before or after it. Could you better clarify the use case in which:

$folder = trim(str_replace(rtrim($path, '/\\'), '', $folder), '/\\');

would fail to provide the subfolder name? Thanks.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12841.

@ggppdk
Copy link
Contributor

ggppdk commented Nov 11, 2016

JFolder::folders($path, ...

Returns:
Folders LIKE: CLEANED($path) /somefolder1/someofolder2

  • thus CLEANED($path) will contain current server directory separator, thus it will contain \

Now this
$folder = trim(str_replace(rtrim($path, '/'), '', $folder), '/');
will fail if $path has any INNER / and you are in windows

e.g. thus you PR will fail with

<field ... type="folderlist" ...  directory="images/sampledata"

but you PR will fail even fail with:

<field ... type="folderlist" ...  directory="images" 

because folderlist does:

if (!is_dir($path))
{
    $path = JPATH_ROOT . '/' . $path;
}

In any case please test this PR: #11288
(it has conflicts and needs to be rebased) , it would have been best if you had searched and post at that PR, but i guess you missed it

I needed to clean the $path contents since it will be used later when trying to remove it from the subfolder's full path. It is cleaned inside of JFolder::folders, but only in the scope of that function. It needs to be cleaned before it is used by JFolder::folder
@mgilkes
Copy link
Contributor Author

mgilkes commented Nov 12, 2016

Thanks for the clarification. You are right. I think the best thing to do is to clean the $path variable before using it to find the subfolders with JFolder. See my update: f73febd


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12841.

@wilsonge
Copy link
Contributor

Closing this in favour of #14139 as we had 4 different PR's for the same issue :)

@wilsonge wilsonge closed this Feb 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants