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

folderlist form field returning full paths on Windows (issue #11102) #11288

Closed
wants to merge 3 commits into
base: staging
from

Conversation

Projects
None yet
@cheesegrits
Contributor

cheesegrits commented Jul 24, 2016

Pull Request for Issue #11102 .

Issue is that folderlist on Windows now returns full paths instead of just folder names.

Summary of Changes

Fix for JFolder::_items() to use DIRECTORY_SEPARATOR instead of / when building full paths, and to rtrim the separator before concatenating file to path, to avoid double separators.

Fix for JFormFieldFolderList, to clean the path, which will then correctly match the path coming back from JFolder::folders(), and trim DIRECTORY_SEPARATOR instead of /.

Testing Instructions

Add a folderlist field to a form in the backend. I used com_categories. Edit ./administrator/components/com_categories/models/forms/category.xml. Around line 246, in the 'basic' fieldset, add ...

            <field
                directory="/administrator/components/com_categories/"
                label="Folder Test"
                name="folderlist_test"
                description="A test for the folderlist form field"
                type="folderlist"
                recursive="true"
            />

Open the J! backend, and edit any existing Category (under the Content menu). Go to the Options tab. You will see a new dropdown, "Folder Test". On a UN*X machine, it'll be just the folder names. On a Windows machine, it will be full absolute paths.

Apply the PR.

Reload the page. The folder dropdown will now just be folder name.

@cheesegrits cheesegrits changed the title from Issue 11102 to folderlist form field returning full paths on Windows (issue #11102) Jul 24, 2016

@wronax

This comment has been minimized.

Show comment
Hide comment
@wronax

wronax Jul 29, 2016

Is this PR need more checks to go into 3.6.1?

wronax commented Jul 29, 2016

Is this PR need more checks to go into 3.6.1?

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Jul 29, 2016

Contributor

yes

Contributor

brianteeman commented Jul 29, 2016

yes

@wronax

This comment has been minimized.

Show comment
Hide comment
@wronax

wronax Jul 29, 2016

What I need to test it? I'm new on GitHub. Could you please send me any tutorial?

wronax commented Jul 29, 2016

What I need to test it? I'm new on GitHub. Could you please send me any tutorial?

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Jul 29, 2016

Contributor

See https://docs.joomla.org/Testing_Joomla!_patches

On 29 July 2016 at 12:06, wronax notifications@github.com wrote:

What I need to test it? I'm new on GitHub. Could you please send me any
tutorial?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#11288 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8ZqcdV_7vZCx2gqXcrzfMG9h-INVks5qad6_gaJpZM4JTrkH
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Contributor

brianteeman commented Jul 29, 2016

See https://docs.joomla.org/Testing_Joomla!_patches

On 29 July 2016 at 12:06, wronax notifications@github.com wrote:

What I need to test it? I'm new on GitHub. Could you please send me any
tutorial?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#11288 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8ZqcdV_7vZCx2gqXcrzfMG9h-INVks5qad6_gaJpZM4JTrkH
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@hardiktailored

This comment has been minimized.

Show comment
Hide comment
@hardiktailored

hardiktailored Jul 29, 2016

I have tested this item successfully on 793db3a

Tested on Windows-7 and Ubuntu 12.04 machine. Before it was showing full path 'var/www/..../', after applying patch it resolved issue on both the machine. Now showing folder name only.


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

hardiktailored commented Jul 29, 2016

I have tested this item successfully on 793db3a

Tested on Windows-7 and Ubuntu 12.04 machine. Before it was showing full path 'var/www/..../', after applying patch it resolved issue on both the machine. Now showing folder name only.


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

@kevinscheithauer

This comment has been minimized.

Show comment
Hide comment
@kevinscheithauer

kevinscheithauer Aug 1, 2016

I have tested this item successfully on a622f45

I have tested this issue @icampus PBF with the proposed solution and can confirm that the fix worked. Tested on Windows 7 with Firefox. I edited the com_categories component with the folderlist field in /models/forms/category.xml. It showed the full path before the fix and applying the patch resolved the issue for me (showed folder names only).


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

kevinscheithauer commented Aug 1, 2016

I have tested this item successfully on a622f45

I have tested this issue @icampus PBF with the proposed solution and can confirm that the fix worked. Tested on Windows 7 with Firefox. I edited the com_categories component with the folderlist field in /models/forms/category.xml. It showed the full path before the fix and applying the patch resolved the issue for me (showed folder names only).


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

@hardiktailored

This comment has been minimized.

Show comment
Hide comment
@hardiktailored

hardiktailored Aug 1, 2016

I have tested this item successfully on a622f45

Tested on Windows 7 and Ubuntu 12.04 (Chrome, Firefox).


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

hardiktailored commented Aug 1, 2016

I have tested this item successfully on a622f45

Tested on Windows 7 and Ubuntu 12.04 (Chrome, Firefox).


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

@hardiktailored

This comment has been minimized.

Show comment
Hide comment
@hardiktailored

hardiktailored Aug 1, 2016

Check results below:

Before patch:
screen shot 2016-08-01 at 05 10 50

After patch:
screen shot 2016-08-01 at 05 11 07


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

hardiktailored commented Aug 1, 2016

Check results below:

Before patch:
screen shot 2016-08-01 at 05 10 50

After patch:
screen shot 2016-08-01 at 05 11 07


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

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Aug 1, 2016

Contributor

Thanks for testing - seems that it works but it is failing the unit tests
@wilsonge is this a false positive? Do the unit tests need updating? Or is there a bugger problem with this PR

Contributor

brianteeman commented Aug 1, 2016

Thanks for testing - seems that it works but it is failing the unit tests
@wilsonge is this a false positive? Do the unit tests need updating? Or is there a bugger problem with this PR

@alimpam

This comment has been minimized.

Show comment
Hide comment
@alimpam

alimpam Aug 1, 2016

I have tested this item successfully on a622f45

tested @icampus:
successfully tested following the given instructions for categories component.
i tested it also for another component (in my case newsfeeds component) and it works.


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

alimpam commented Aug 1, 2016

I have tested this item successfully on a622f45

tested @icampus:
successfully tested following the given instructions for categories component.
i tested it also for another component (in my case newsfeeds component) and it works.


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

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Aug 2, 2016

Contributor

This isn't a false positive on the test - it seems like a genuine failure. But it's as a result of adding https://github.com/joomla/joomla-cms/pull/11288/files#diff-ec4b92ba2051d70bff7613c1a6199c5bR192 but that's just a sanity check - I'm not sure if it's a dodgy test or the sanity check is going to kill things on some (linux?) filesystems

Contributor

wilsonge commented Aug 2, 2016

This isn't a false positive on the test - it seems like a genuine failure. But it's as a result of adding https://github.com/joomla/joomla-cms/pull/11288/files#diff-ec4b92ba2051d70bff7613c1a6199c5bR192 but that's just a sanity check - I'm not sure if it's a dodgy test or the sanity check is going to kill things on some (linux?) filesystems

@pepperstreet

This comment has been minimized.

Show comment
Hide comment
@pepperstreet

pepperstreet Aug 3, 2016

Thought this would make it into Joomla 3.6.1 ?!


BTW, I have tested successfully with a 3rd-party extension Cobalt CCK by MintJoomla. Which uses the field "folderlist" in global configuration. There is a parameter for Community-Integration, which lists the respective subfolder-name.
OSX El Capitan - MAMP 3.4

pepperstreet commented Aug 3, 2016

Thought this would make it into Joomla 3.6.1 ?!


BTW, I have tested successfully with a 3rd-party extension Cobalt CCK by MintJoomla. Which uses the field "folderlist" in global configuration. There is a parameter for Community-Integration, which lists the respective subfolder-name.
OSX El Capitan - MAMP 3.4

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Aug 3, 2016

Contributor

@pepperstreet it was not included because it is failing the unit tests

Contributor

brianteeman commented Aug 3, 2016

@pepperstreet it was not included because it is failing the unit tests

@pepperstreet

This comment has been minimized.

Show comment
Hide comment
@pepperstreet

pepperstreet Aug 4, 2016

@brianteeman Thanks for your comment, but what is a "unit test"? (seriously)

pepperstreet commented Aug 4, 2016

@brianteeman Thanks for your comment, but what is a "unit test"? (seriously)

@wronax

This comment has been minimized.

Show comment
Hide comment
@wronax

wronax Aug 4, 2016

I'm also curious why it failed unit test. Is it possible that unit test is written incorrectly?

This bug is critical for many extensions and template frameworks, because they use folderlist field as a theme option what causes that no theme is loaded after saving extension/template settings and as a result website is messed up with no possibility fix it from Joomla back-end.

wronax commented Aug 4, 2016

I'm also curious why it failed unit test. Is it possible that unit test is written incorrectly?

This bug is critical for many extensions and template frameworks, because they use folderlist field as a theme option what causes that no theme is loaded after saving extension/template settings and as a result website is messed up with no possibility fix it from Joomla back-end.

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Aug 4, 2016

Contributor

@wilsonge provided the link above

Contributor

brianteeman commented Aug 4, 2016

@wilsonge provided the link above

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Sep 9, 2016

Contributor

I have tested this item successfully on a622f45

Tested and it is still an issue which has to be fixed.

Can we do something about the failed unit tests ourself?


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

Contributor

Bakual commented Sep 9, 2016

I have tested this item successfully on a622f45

Tested and it is still an issue which has to be fixed.

Can we do something about the failed unit tests ourself?


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

@Globulopolis

This comment has been minimized.

Show comment
Hide comment
@Globulopolis

Globulopolis Sep 27, 2016

I have tested this item successfully on a622f45


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

Globulopolis commented Sep 27, 2016

I have tested this item successfully on a622f45


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

@pepperstreet

This comment has been minimized.

Show comment
Hide comment
@pepperstreet

pepperstreet Oct 19, 2016

Still not in J! 3.6.3 ? Any news about it? How to get this merged? ;)

pepperstreet commented Oct 19, 2016

Still not in J! 3.6.3 ? Any news about it? How to get this merged? ;)

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Oct 19, 2016

Member

Fixing the unit test failure for starters. Either determining if the test failure is an indicator of an actual code issue or if it's a bad test. If it's a bad test, it needs to be rewritten to ensure the behavior is valid going forward.

There's more to getting code merged than just opening a pull request and asking for it to be merged 😉

Member

mbabker commented Oct 19, 2016

Fixing the unit test failure for starters. Either determining if the test failure is an indicator of an actual code issue or if it's a bad test. If it's a bad test, it needs to be rewritten to ensure the behavior is valid going forward.

There's more to getting code merged than just opening a pull request and asking for it to be merged 😉

@mo-matictec

This comment has been minimized.

Show comment
Hide comment
@mo-matictec

mo-matictec Nov 10, 2016

I have tested this item successfully on a622f45

We have tested it successfully on our Windows Server 2012 R2 servers and PHP 7.


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

mo-matictec commented Nov 10, 2016

I have tested this item successfully on a622f45

We have tested it successfully on our Windows Server 2012 R2 servers and PHP 7.


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

@ggppdk

This comment has been minimized.

Show comment
Hide comment
@ggppdk

ggppdk Nov 11, 2016

Contributor

I have tested this item successfully on a622f45

This works, but needs to be rebased it has conflicts,
maybe after resolving conflicts, and after replacing check() with clean(),
test units will succeed ?

e.g. about conflicts it extends different class:

-class JFormFieldFolderList extends JFormAbstractlist
+class JFormFieldFolderList extends JFormFieldList


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

ggppdk commented Nov 11, 2016

I have tested this item successfully on a622f45

This works, but needs to be rebased it has conflicts,
maybe after resolving conflicts, and after replacing check() with clean(),
test units will succeed ?

e.g. about conflicts it extends different class:

-class JFormFieldFolderList extends JFormAbstractlist
+class JFormFieldFolderList extends JFormFieldList


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

This comment has been minimized.

Show comment
Hide comment
@jeckodevelopment

jeckodevelopment Nov 17, 2016

Member

This PR has 7 successful tests, but it needs to make Travis "green".
So @cheesegrits can you please look at the conflicts?

Member

jeckodevelopment commented Nov 17, 2016

This PR has 7 successful tests, but it needs to make Travis "green".
So @cheesegrits can you please look at the conflicts?

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Nov 17, 2016

Member

It doesn't matter how many users have tested it, the unit tests still need to be fixed. If the unit tests have a bad behavior, the tests need to be rewritten. If the tests are working correctly, then this indicates there is an incorrect functional change and the patch needs to be adjusted.

Member

mbabker commented Nov 17, 2016

It doesn't matter how many users have tested it, the unit tests still need to be fixed. If the unit tests have a bad behavior, the tests need to be rewritten. If the tests are working correctly, then this indicates there is an incorrect functional change and the patch needs to be adjusted.

@CoalaWeb

This comment has been minimized.

Show comment
Hide comment
@CoalaWeb

CoalaWeb Jan 27, 2017

Any progress on updating this so it passes the unit tests?

CoalaWeb commented Jan 27, 2017

Any progress on updating this so it passes the unit tests?

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Feb 23, 2017

Contributor

Closing this in favour of #14139 which should fix this issue and doesn't have unit test issues

Contributor

wilsonge commented Feb 23, 2017

Closing this in favour of #14139 which should fix this issue and doesn't have unit test issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment