? ? Pending

User tests: Successful: Unsuccessful:

avatar Hackwar
Hackwar
18 Mar 2020

Fixes regression of #16708/#20294

Summary of Changes

The ltrim() breaks all absolute unix filesystem paths, which results in a bug in Fabrik with Joomla 3.9.16 https://fabrikar.com/forums/index.php?threads/error-after-joomla-update-to-3-9-16.51358/

avatar Hackwar Hackwar - open - 18 Mar 2020
avatar Hackwar Hackwar - change - 18 Mar 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 18 Mar 2020
Category Libraries
avatar mbabker
mbabker - comment - 19 Mar 2020

Hmm, if only there were unit tests for the class which could be updated to include a regression test against the exact scenario that is vaguely hinted at between the Fabrikar forum thread and the comment thread on the merge commit, instead of monkey patching Joomla.

avatar richard67
richard67 - comment - 7 Apr 2020

@Hackwar By code review this PR looks good and reasonable to me. Do you have any testing instructions for a real test?

avatar richard67
richard67 - comment - 7 Apr 2020

I have tested this item successfully on 8f6c1e7

Code review.


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

avatar richard67 richard67 - test_item - 7 Apr 2020 - Tested successfully
avatar Hackwar Hackwar - change - 7 Apr 2020
Labels Added: ? ?
avatar joomla-cms-bot joomla-cms-bot - change - 7 Apr 2020
Category Libraries Libraries Unit Tests
avatar Hackwar
Hackwar - comment - 7 Apr 2020

Trying to give a better explanation here:
This field supports two types of directory values, a relative one to JPATH_ROOT and an absolute one. In case of a relative path, the value is prepended with JPATH_ROOT. There are cases where you wouldn't have the directory attribute hardcoded, but configurable and thus set somewhere in your model for example. In that case you would most likely use an absolute path. So there are valid usecases for an absolute path.

All of this works fine as is on Windows machines. However on Unix machines, absolute paths start with a / and due to the ltrim(), we are removing any leading / and then prepending that with JPATH_ROOT. So if you would for example set the directory to /var/www/modules, with the ltrim() in place, the code would make var/www/modules out of that and since that is not a valid path in the context of the script, it would try to make an absolute path out of that and that would result in /var/www/var/www/modules. I hope that makes it clear why the ltrim() breaks this feature here.

avatar trob
trob - comment - 10 Apr 2020

With #28393 Fabrik is working again (Fabrik3.9 without the modifications made to skip this Joomla issue).

avatar richard67
richard67 - comment - 10 Apr 2020

@trob That means you have tested this Pull Request (PR) here with success? If so, could you mark your test result at the Joomla Issue Tracker here? https://issues.joomla.org/tracker/joomla-cms/28393. Just use the "Test this" button at the top left corner, then select the appropriate test result and use the "Submitt" button. Thanks in advance.

avatar trob
trob - comment - 10 Apr 2020

I have tested this item successfully on df7615d


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

avatar trob trob - test_item - 10 Apr 2020 - Tested successfully
avatar richard67
richard67 - comment - 10 Apr 2020

Thanks @trob . Now as this PR has 2 good tests, it will be set to RTC (Ready to Commit), then a maintainer, mostly the release lead will possibly review it and merge it. If that happens soon, we can find the correction in the next release 3.9.17. This is how it works.

By the way we (Joomla community) always need more contibutors and testers, so if you have a bit spare time sometimes and are a fan of Joomla, you could test other PR's, or find bugs and make PR's to correct them, or even just comment in an issue if you can reproduce it or have observed it so we know it is confirmed ... all that would be very helpful.

avatar richard67
richard67 - comment - 10 Apr 2020

I have tested this item successfully on df7615d

Code review.


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

avatar richard67 richard67 - test_item - 10 Apr 2020 - Tested successfully
avatar richard67 richard67 - change - 10 Apr 2020
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 10 Apr 2020

RTC


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

avatar HLeithner HLeithner - change - 10 Apr 2020
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2020-04-10 10:50:40
Closed_By HLeithner
Labels Added: ?
Removed: ?
avatar HLeithner HLeithner - close - 10 Apr 2020
avatar HLeithner HLeithner - merge - 10 Apr 2020
avatar HLeithner
HLeithner - comment - 10 Apr 2020

Thanks

Add a Comment

Login with GitHub to post a comment