Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Stop JTable from giving up too early #1846

Closed
wants to merge 1 commit into from

4 participants

@Paladin

JTable wasn't finding a class because it was in a file with the same name as a file encountered earlier in the include chain. This change enables JTable to keep looking, instead of giving up after the first matching file.

This one is worth discussing, I think. I can see an argument for keeping it as it was, but this is a case of "scratching my own itch." I have to work with an extension that creates just this situation -- it's in the includePath ahead of a Joomla class with the same file name (not the same class name).

The actual problem originates with JPath::find, but I think fixing it there would have much wider repercussions for b/c than this change will. Perhaps it would be the best idea to start moving towards that solution, putting this one in place to fix existing problems until there is time to smoothly integrate the other change into the system.

The change moves the loop into JTable, and just uses JPath for the proofing. (Maybe that should be pulled in to here to? I just wanted to minimize what I was changing, not arguing this is the ultimately best way.) Every successful result should be unaffected by this change, but some rare unsuccessful ones will now be successful.

Not sure if this change even causes a b/c issue, because it shouldn't break any successful code, and it's hard for me to understand why the older unsuccessful result should be preferred. It certainly causes a change, very minor but a change, in the behavior of the actual API while not affecting the API description at all.

@Paladin Paladin Stop JTable from giving up too early
JTable wasn't finding a class because it was in a file of the same name that was encountered earlier in the include chain. This change enables JTable to keep looking, instead of giving up after the first matching file.
c65e3e3
@mahagr

I would just figure out how to use autoloader instead... For now I'm just making sure that the filenames are unique.

@eddieajau

Arlen, if you register the class using the loader, does that help?

@dongilbert
Collaborator

I would also recommend using the autoloader, since JTable essentially tries to autoload the class via class_exists before it traverses the $_includePaths variable. You'll find that this approach is also faster than what you've proposed. However, 2-3 years ago, I would have accepted this in a heartbeat, but now that my understanding of the autoloader etc has increased, I find using it for all my class loading needs is really where it's at. It really is faster than our own "autoload" for table classes that utilizes JFile.

@dongilbert
Collaborator

Arlen, there are a couple check style errors, can you fix those up if you're still interested in getting this merged? http://developer.joomla.org/pulls/pulls/1846.html

It may not be worth the time to fix though, as the consensus seems to be it would be better to use the autoloader. But, I'm not opposed to the CMS picking this up instead, if @elinw or @mbabker could offer the CMS's take on this, that'd be good to know before fixing up the 2 issues.

Just leave a comment saying "reopen" if/when you're ready for it to be reconsidered.

@dongilbert dongilbert closed this
@Paladin

Sorry, guys, forgot it was here as well. Have to admit the CMS currently is far more relevant to my day-to-day than the Platform/Framework, so I'm spending more time there. If the bug is beneath the CMS, I try to post a PR in both places, to make sure whoever wants it, gets it.

To answer:

Re register and autoloader. The class that wasn't getting loaded properly was a core class. This was happening because a 3rd party component I had to use created a class named the same as the core class (but without the "J") and added it to the include paths process. (I'm away from the office at the moment, but IIRC the classes were JTableContent and TableContent. The system was looking for JTC and found TC first in the included files, so quit.)

Admittedly, the component wasn't written well at all, but the issue itself I thought was worth the raising, in case it might arise under better circumstances. I patched my own core, simply because I don't have time to fix the component at the moment. Essentially they're using TC to reference the same database table as JTC, for their own purposes. Job requires its function, so I did this to move on to more serious issues; it'll come back on my radar in a few months, and I'll rewrite the component then.

Apologies for the style errors, Don. Have to admit I never looked much beyond the issue at hand. My bad.

As for reopening, not sure if it's worth it. Like I said, the change was made necessary for me in large part because of a badly-written extension, this is simply an adjustment to compensate for the extension. It could very well be the best answer from your (meaning the platform/framework maintainer) point of view is to rewrite the component. That sounds like the consensus, anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 9, 2013
  1. @Paladin

    Stop JTable from giving up too early

    Paladin authored
    JTable wasn't finding a class because it was in a file of the same name that was encountered earlier in the include chain. This change enables JTable to keep looking, instead of giving up after the first matching file.
This page is out of date. Refresh to see the latest.
Showing with 8 additions and 11 deletions.
  1. +8 −11 libraries/joomla/table/table.php
View
19 libraries/joomla/table/table.php
@@ -218,22 +218,19 @@ public static function getInstance($type, $prefix = 'JTable', $config = array())
if (!class_exists($tableClass))
{
// Search for the class file in the JTable include paths.
- $path = JPath::find(self::addIncludePath(), strtolower($type) . '.php');
+ jimport('joomla.filesystem.path');
- if ($path)
+ $paths = JTable::addIncludePath();
+ $pathIndex = 0;
+ while (!class_exists($tableClass) && $pathIndex < count($paths))
{
- // Import the class file.
- include_once $path;
-
- // If we were unable to load the proper class, raise a warning and return false.
- if (!class_exists($tableClass))
+ if ($tryThis = JPath::find($paths[$pathIndex++], strtolower($type) . '.php'))
{
- JLog::add(JText::sprintf('JLIB_DATABASE_ERROR_CLASS_NOT_FOUND_IN_FILE', $tableClass), JLog::WARNING, 'jerror');
-
- return false;
+ // Import the class file.
+ include_once $tryThis;
}
}
- else
+ if (!class_exists($tableClass))
{
// If we were unable to find the class file in the JTable include paths, raise a warning and return false.
JLog::add(JText::sprintf('JLIB_DATABASE_ERROR_NOT_SUPPORTED_FILE_NOT_FOUND', $type), JLog::WARNING, 'jerror');
Something went wrong with that request. Please try again.