Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix JTable::addIncludePath - includes tests #1609

Merged
merged 1 commit into from

3 participants

@dongilbert
Collaborator

When passed paths that already exist in the $_includePaths array, JTable::addIncludePath mistakenly re-adds them. This is due to the fact that before !in_array($path, self::$_includePaths) runs, $path is converted to an array. This was to facilitate the ability to pass either an array of paths or a path string to the method. However, it also caused the in_array function to check $_includePaths as a multi-dimensional array, when it is only ever a single-dimensional array. This code fixes that behavior. I've included tests as well.

@dongilbert
Collaborator

As you can probably tell, this is the actual commit - dongilbert@0f9aefd

I'm not sure how my staging branch (which I branched from) get's behind. Should I git pull upstream staging into my staging branch before I checkout / create a new branch for editing?

@LouisLandry

So what I do in this sort of situation is:

$ git checkout origin jtablefix
$ git pull --rebase upstream staging
$ git push -f origin jtablefix

That makes sure you are on the correct branch, rebased your changes in the correct branch on top of the upstream staging branch, and then force pushes the result up to GitHub. Your mileage may vary, but try that.

Note: I'm closing this pull request for now until the requested changes are uploaded. Please leave a comment asking to reopen the pull request at that time so we can review it again. Thanks.

@dongilbert
Collaborator

@LouisLandry - since this was a closed pull request, I didn't see the changes take effect in my commit log above. I thought something might have been broke w/github, and submitted another PR instead to see if it would update this one, but it obviously didn't. I'll know next time to simply request the PR be reopened so that I can see the changes and make sure it updated correctly. Oh well.

That said, the new PR with the correct commit is here - #1610

@dongilbert
Collaborator

So, I think it would be good to document the new joomla-jenkins bot that reopens pull requests when a comment is made. I'll close PR #1610

@LouisLandry

So, I'm merging this, but the addIncludePath method really shouldn't be used any longer. It would be much better to use the autoloader for making sure that classes are available. The addIncludePath methods in JTable, JModel, etc, etc, were implemented before we had real autoloading available as an option.

@LouisLandry LouisLandry merged commit 21d12e1 into joomla:staging
@dongilbert
Collaborator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
7 libraries/joomla/table/table.php
@@ -228,7 +228,7 @@ public static function addIncludePath($path = null)
settype($path, 'array');
// If we have new paths to add, do so.
- if (!empty($path) && !in_array($path, self::$_includePaths))
+ if (!empty($path))
{
// Check and add each individual new path.
foreach ($path as $dir)
@@ -237,7 +237,10 @@ public static function addIncludePath($path = null)
$dir = trim($dir);
// Add to the front of the list so that custom paths are searched first.
- array_unshift(self::$_includePaths, $dir);
+ if (!in_array($dir, self::$_includePaths))
+ {
+ array_unshift(self::$_includePaths, $dir);
+ }
}
}
View
14 tests/suites/unit/joomla/table/JTableTest.php
@@ -92,6 +92,20 @@ public function testAddIncludePath()
$this->equalTo(realpath(JPATH_PLATFORM . '/joomla/table')),
'The default return from addIncludePath without additional parameters should be to "JPATH_PLATFORM . /joomla/table"'
);
+
+ // Test that adding paths that already exist don't get re-added
+ $expected = array(
+ '/dummy/',
+ 'dir/not/exist',
+ JPATH_PLATFORM . '/joomla/table'
+ );
+
+ // Add dummy paths
+ $paths = JTable::addIncludePath(array('dir/not/exist', '/dummy/'));
+ // Re-add the returned paths - these shouldn't get added again.
+ $paths = JTable::addIncludePath($paths);
+
+ $this->assertEquals($expected, $paths);
}
/**
Something went wrong with that request. Please try again.