User tests: Successful: Unsuccessful:
Fixes regression of #16708/#20294
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/
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries |
I have tested this item
Code review.
Labels |
Added:
?
?
|
Category | Libraries | ⇒ | Libraries Unit Tests |
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.
@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.
I have tested this item
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.
I have tested this item
Code review.
Status | Pending | ⇒ | Ready to Commit |
RTC
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: ? |
Thanks
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.