[#19781] - Avoid duplicate paths if path is not a directory
- Fixed in Code Base
- 11 Dec 2019
- Medium
- Build: staging
- # 19781
- Diff
- Sophist-UK:patch-1
- Success hound No violations found. Woof! Details
- Success continuous-integration/drone/pr the build was successful Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
User tests: Successful: Unsuccessful:
Form path arrays contain duplicates when they shouldn't.
Current code:
foreach ($new as $path)
{
if (!in_array($path, $paths))
{
array_unshift($paths, trim($path));
}
if (!is_dir($path))
{
array_unshift($paths, trim($path));
}
}
Proposed code:
foreach ($new as $path)
{
$path = trim($path);
if (!in_array($path, $paths) && is_dir($path))
{
array_unshift($paths, $path);
}
}
Summary of Changes
Path arrays currently have duplicate path in them - this PR removes the code which creates duplicates.
Testing Instructions
$form = new JForm('test');
var_dump($form->addFieldPath('/')); // Exists
var_dump($form->addFieldPath('/directory_doesnt_exist')); // Does not exist
Existing result
Returned array shows / and /directory_doesnt_exist twice.
Fixed result
Returned array shows / once and /directory_doesnt_exist is omitted.
Documentation Changes Required
None.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
| Labels |
Added:
?
|
||
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19781.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19781.
brianteeman
- | Milestone |
Added: |
||
The unit test failure needs to be addressed before this can be merged.
| Milestone |
Removed: |
||
| Status | Ready to Commit | ⇒ | Pending |
| Category | Libraries | ⇒ | Libraries Unit Tests |
Apologies - I had not spotted that. Working on it now.
| Labels |
Added:
?
|
||
| Category | Libraries Unit Tests | ⇒ | Libraries |
The tests I did when coding was to discover that the original code resulted in weird arrays on my Joomla system, fix the code and see that the arrays on my real Joomla system were now as expected.
This is what is in these variables:
fieldpaths
/home/travis/build/joomla/joomla-cms/tests/unit/suites/libraries/joomla/form
/home/travis/build/joomla/joomla-cms/tests/unit/suites/libraries/joomla/form/_testfields
/home/travis/build/joomla/joomla-cms/libraries/joomla/form/fields
/home/travis/build/joomla/joomla-cms/libraries/src/Form/fields
formpaths
/home/travis/build/joomla/joomla-cms/tests/unit/suites/libraries/joomla/form
/home/travis/build/joomla/joomla-cms/libraries/src/Form/forms
rulepaths
/home/travis/build/joomla/joomla-cms/tests/unit/suites/libraries/joomla/form/_testrules
/home/travis/build/joomla/joomla-cms/tests/unit/suites/libraries/joomla/form
/home/travis/build/joomla/joomla-cms/libraries/src/Form/rules
I have no idea why these strings are in there, nor why field1/2/3 etc. should be in there, nor why my change means they are no longer in there.
I need some help to understand this, and then I can fix the tests.
Ok - I found where the paths came from - XML in JFormDataHelper.php.
These are not being added because the directory for e.g. field1 doesn't exist.
It should be easy to make the existing tests work by removing the check that the path is to a directory and allow adding files as well as directories, however I am unclear whether these arrays should be of directories only, or whether we should also include files. So I need a steer on this.
P.S. The tests only test that the paths are added from XML - they do not currently check the correct operation of the addXxxxxPath i.e. that directories are added and files and non-existent paths are not. I will add these tests when I fix this.
| Labels |
Removed:
?
|
||
This should now be mergable.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19781.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19781.
| Status | Pending | ⇒ | Ready to Commit |
RTC.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19781.
Though I don't understand why do we add non-existing paths anyways.
| Labels |
Added:
?
|
||
Though I don't understand why do we add non-existing paths anyways.
It doesn't hurt anything to have non-existing paths in the registry as long as at runtime when that registry is used the paths are checked to make sure they exist. Otherwise you're doing a check when adding to the registry and when the registry is used, and that comes with a performance hit because it involves filesystem I/O.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-12-11 14:28:11 |
| Closed_By | ⇒ | HLeithner |
Finally...
Thank you for your optimization.
Wow - submitted on 25 Feb 2018, and merged on 11 December 2019 only 23 months later. Is this a record?
P.S. It was only a small fix, but I am glad to be able to contribute.
I have tested this item✅ successfully on b57ddc3
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19781.