Problem with multiple user.php files and JTableUser definition

[Radek Suski]

Hi everybody,

I found small problem to instantiate the JUser class if we are inside a component which has a user.php table definition inside the administrator/components/com_something/tables directory.

Here is what happen:

We have a small administrator module in which we need the some data of the currently logged user.

So we are trying to get the user with JFactory::getUser()

It is going through the normal way:

• JUser->__construct( )
  
• JUser->load( )
  
• JUser->getTable( )
  
• JTable::getInstance( 'user', 'JTable' )
  

Now when the user is currently visiting "com_something" component, after the JTable::addIncludePath() has been called the possible path array looks like this:

Array ( 
[0] => /root/administrator/components/com_something/tables 
[1] => /root/administrator/components/com_something/tables 
[2] => /root/libraries/joomla/database/table 
)

Because in the /root/administrator/components/com_something/tables there is indeed a file called user.php, JPath::find() returns as the class path for the "JTable":

/root/administrator/components/com_something/tables/user.php

 

And at the end the whole procedure fails.

I think that is is not the base idea to have a file called "user.php" inside the tables directory but some extensions have it.

What would be a good solution here?

Either we should not allow do have conflicting file names, which probably would not even really work, or maybe re-write the "JTable::getInstance" method that way that it travels the whole path array until the right class name has been found or the array exhausted.

What do you think?

Regards,

Radek

[Andrew Eddie]

This is a long standing issue. There are two options to workaround the issue.

A) Include the component user.php file
B) Register the component's user table class with the loader.

B is my preferred way and you'd just do something like this in your
main component file before you execute the component controller:

JLoader::register('SomethingTableUser', __DIR__ . '/tables/user.php');

Adjust the path to suit (and note __DIR__ is PHP 5.3+).

The "addIncludePath" methods are a legacy of PHP 4 so it's better to
go down the path of registering classes or relying on the autoloader
when 3.0 comes out (better as opposed to trying to "fix"
JTable::getInstance).

Hope that helps.

Regards,
Andrew Eddie
http://learn.theartofjoomla.com - training videos for Joomla developers

[Andrew Eddie]

Oh, sorry, I got that backwards.

I think the only way I've got around that situation is to rename the
file and the class in the component. For example, for com_something,
have SomethingTableSuser in /tables/suser.php

Regards,
Andrew Eddie
http://learn.theartofjoomla.com - training videos for Joomla developers

[Radek Suski]

Hi Andrew,

so you mean we (Joomla!) should restrict component developer to not use certain file names?

Honestly I don't think that this is such a good idea because I don't believe we can really execute this. 

Developers will use it anyway because they may not even be aware of this restriction.

And I think as this is the method of JTable, which is one of the most important classes in the platform, the potential for further conflicts may be quite high.

I guess sooner or later this problem will come back with JUser or any other class.

Or am I wrong?

Regards,

Radek

[Andrew Eddie]

On 16 May 2012 16:42, Radek Suski <suski...@googlemail.com> wrote:
> Hi Andrew,
>
> so you mean we (Joomla!) should restrict component developer to not use
> certain file names?
> Honestly I don't think that this is such a good idea because I don't believe
> we can really execute this.
> Developers will use it anyway because they may not even be aware of this
> restriction.
> And I think as this is the method of JTable, which is one of the most
> important classes in the platform, the potential for further conflicts may
> be quite high.
> I guess sooner or later this problem will come back with JUser or any other
> class.
>
> Or am I wrong?

Dunno, but that's how I have worked around the issue in the past.

That said, I actually think my first reply to you does work so I would
try that first.

[Phil Brown]

This is something I hit fairly often.

Only recently with a html class called SSFormsHtmlForm in a file in JPATH_COMPONENT/html/form.php.

This stopped JHtmlForm::token from loading due to this issue.
Personaly it is ugly to have other files named correctly then have an odd one out due to conflicts.

If the paths could take the namespace into account?

Regards,

Phill Brown
M  04 2481 9754
Bathurst Software Solutions
-------------------------------------------------------------------------------------------------------------------

[Andrew Eddie]

On 16 May 2012 17:01, Phil Brown <pbr...@softwaresolutions.com.au> wrote:
> This is something I hit fairly often.
>
> Only recently with a html class called SSFormsHtmlForm in a file in
> JPATH_COMPONENT/html/form.php.
>
> This stopped JHtmlForm::token from loading due to this issue.
> Personaly it is ugly to have other files named correctly then have an odd
> one out due to conflicts.
>
> If the paths could take the namespace into account?

The whole point of the problem is that all the "include path" methods
and their variants are a hangover from supporting PHP 4. This is no
longer a problem with new autoloaders and such, but we aren't
completely there yet. The workaround is to use JLoader::register to
force the behaviour that you want and this works in Joomla 1.5 and
2.5. That's far easier than trying to "fix" the path order which I'm
sure will end up breaking something else in an unexpected way.

Name spaces are only going to help when everything is name spaced and
I can't see that happening in the Platform until next year.

[Radek Suski]

Andrew,

yes but this is not my component which causes the problem, this is the component I have problems with.

In particular there is a component called "Job Board". This component has a file in /administrator/components/com_jobboard/tables/user.php. This file is NOT an override of the JTable User class.

When I'm trying to create the JUser object in our module, JPath::find() means that this table definition from the "Job Board" component is the right definition for the JTableUser class, which is not the case, and that's the whole problem. 

Phil, namespaces in PHP is not something we can rely on yet I think.

[Andrew Eddie]

On 16 May 2012 17:15, Radek Suski <suski...@googlemail.com> wrote:
> Andrew,
>
> yes but this is not my component which causes the problem, this is the
> component I have problems with.
>
> In particular there is a component called "Job Board". This component has a
> file in /administrator/components/com_jobboard/tables/user.php. This file is
> NOT an override of the JTable User class.
> When I'm trying to create the JUser object in our module, JPath::find()
> means that this table definition from the "Job Board" component is the right
> definition for the JTableUser class, which is not the case, and that's the
> whole problem.

Gotcha.

Try adding this to your module:

JLoader::register('JTableUser', JPATH_LIBRARIES . '/joomla/database/table.php');

Let me know if that fixes it.

Regards,
Andrew Eddie

[Radek Suski]

Andrew,

no, it would not because JTable::addIncludePath() still returns:

Array ( 
[0] => ../administrator/components/com_jobboard/tables 
[1] => ../administrator/components/com_jobboard/tables 
[2] => ../libraries/joomla/database/table 
)

and as /administrator/components/com_jobboard/tables is the first element of this array JPath::find() is trying to pick the /administrator/components/com_jobboard/tables/user.php as the right definition for the JTableUser class.

[Andrew Eddie]

On 16 May 2012 17:38, Radek Suski <suski...@googlemail.com> wrote:
> Andrew,
>

> no, it would not because JTable::addIncludePath() still returns:
>>
>> Array (
>> [0] => ../administrator/components/com_jobboard/tables
>> [1] => ../administrator/components/com_jobboard/tables
>> [2] => ../libraries/joomla/database/table
>> )
>
>
> and as /administrator/components/com_jobboard/tables is the first element of
> this array JPath::find() is trying to pick the
> /administrator/components/com_jobboard/tables/user.php as the right
> definition for the JTableUser class.

Did you try it?

[Radek Suski]

I did.

Even: JLoader::register('JTableUser', JPATH_LIBRARIES . '/joomla/database/table/user.php'); 

[Andrew Eddie]

On 17 May 2012 00:51, Radek Suski <suski...@googlemail.com> wrote:
> I did.
>
> Even: JLoader::register('JTableUser', JPATH_LIBRARIES .
> '/joomla/database/table/user.php');

Hrm, ok. Try this in your module then:

require_once JPATH_LIBRARIES . '/joomla/database/table/user.php';

I'll try and find some time to dig deeper into why the loader isn't'
working. What Joomla and PHP versions are you working with?

Regards,
Andrew Eddie

[Sam Moffatt]

Possibly because if it's a module it's being rendered after the
component and if the component has already loaded the file then the
autoloader isn't ever going to get hit because the class has been
loaded by JTable already.

Cheers,

Sam Moffatt
http://pasamio.id.au

[Elin Waring]

https://github.com/joomla/joomla-cms/blob/master/libraries/joomla/database/table.php#L166

Right, so if I understand it correctly (and there's a good chance I don't), it's the same fundamental issue as mixing and matching new mvc modules on pages with old mvc components isn't it?  So we've been way too focused on workarounds for the specific use cases of JModel JView and JController and not thinking abstractly about the general issue of autoloading clashes in more complex use cases  than simple platform applications (i.e. we need to think about platform applications that are themselves extensible).     What if two extensions to a platform app both extend the JTableUser and you want output from them to render from both on the same page? It's like we need context for each element.  I think going down the route of using namespacing to solve every problem is going to end up being self-defeating and horrible to manage. 

From what Andrew  has said before handling multiple items on a rendered page (i.e. lists is one example) is a missing but coming piece of UCM,  I haven't tried anything as complicated as this, but I think total buy in to JApplicationWeb+ UCM+new MVC will handle most of that  because you are pushing rendered content for everything on the page starting fresh with a new JContent object  for each.   But I don't think that the platform should require buy in to that structure just because you are using JApplicationWeb especially since it isn't in the platform yet. 

Elin

[Andrew Eddie]

Wouldn't that mean that com_jobboard is redefining JTableUser?

Regards,
Andrew Eddie
http://learn.theartofjoomla.com - training videos for Joomla developers

[Elin Waring]

Yes

class TableUser extends JTable 

Elin

[Radek Suski]

But if I understand correctly the TableUser in com_jobboard is not a replacement for Joomla! JTableUser.

It's just happen to have the same file name.

Nevertheless I still think that the main issue is that JPath::find() stops searching for the right class definition after first occurrence of a user.php file.

I think it would be better if this method, or the above, could ensure that the found file contains indeed the class definition we are currently looking for.

Regards,

Radek

[Sam Moffatt]

Likely it's actually called JTableUser if they are directly grabbing
the table from JTable::getInstance instead of using JModel::getTable.
JTable defaults to a prefix of JTable and JModel defaults to a prefix
of Table. This would also explain why explicitly registering the class
with the autoloader doesn't solve anything because the class has
already been loaded. Short of looking at the definition in the file (I
guess that's Radek's department) there isn't an easy way to confirm
but this would explain why manually registering the class name didn't
work properly.

This is also presuming you're not hitting JTable::getInstance's "class
not found in file" error which would happen if it was called TableUser
instead of JTableUser.

Regardless swapping that to a JFolder::files until we get everything
autoloading neatly wouldn't probably be a good idea for anyone
interested in picking it up. Presuming someone doesn't label the class
identically it would at least solve the class not found in file error.

Cheers,

Sam Moffatt
http://pasamio.id.au

[Elin Waring]

 I was quoting from the file directly.

From looking at the file given that it does not have a password field so  I would say it is not a functional replacement for the actual user table but we have no idea maybe they do have some other unknown replacement authentication system that doesn't use the Joomla user table. 

The install creates a table CREATE TABLE IF NOT EXISTS `#__jobboard_users` 

Now it would make sense for the code to say to be JobboardTableUsers

Still, Theoretically shouldn't they be able to replace the user table? What if this was some other application?  I'm really asking that, it's not rhetorical. 

Elin

[Sam Moffatt]

Near as I can tell you didn't do a pull request against Joomla!'s
staging branch but against Joomla!'s master branch (two as far as I
can see). This would have subverted the unit testing and code style
infrastructure designed to ensure we don't introduce new style issues
or break unit tests. What you needed to do is make the pull request
against the Joomla! staging branch not master.

GitHub have done a rather good job of documentation though not always
the most up to date. It does however cover this situation though these
days it's a bit more obvious what the destination repository is:
https://help.github.com/articles/using-pull-requests/

Cheers,

Sam Moffatt
http://pasamio.id.au

On Tue, Jul 24, 2012 at 8:15 AM, Thomas PAPIN <thomas...@gmail.com> wrote:
> Hi Radek,
>
> I just discovered this thread. I had the same problem with
> JTable::getInstance( 'user', 'JTable' ), 1 year ago and I raised a ticket
> for that with no success
> Link : https://github.com/joomla/joomla-platform/pull/268
>
> My solution was quite "easy".
> In the getInstance function of JTable, instead of using JPath::find() and I
> used a new function JPath::findAll().
> JPath::findAll() returns the list of all instances that match when
> JPath::find() returns the first instance that match.
> So in the getInstance all "User Table" classes will be tested for the test
> class_exists.
>
> In your example:
> [0] => /root/administrator/components/com_something/tables
> [1] => /root/libraries/joomla/database/table
> with JPath::find() only [0] is tested => Returns ERROR
> with Jpath::findAll() [0] and [1] are tested => getinstance returns the
> correct class.
>
> As my patch was not accepted to avoid renaming User to User2 or just another
> loading solution,
> I found this little workaround :
> Just at the end of my component code:
> $path = JPATH_ROOT.DS.'libraries'.DS.'joomla'.DS.'database.DS.'table';
> JTable::addIncludePath($path);
> Like this the Joomla Core Table are at the first position in the include
> path array. so other modules or plugins will not be impacted by the
> includepath of my component.

[Thomas PAPIN]

Oh thanks for this information I am newbie with github. Will read all that.
Is there some document that explain the joomla branches on github (staging, master,), validation process , unit test, etc..

Checking old mail, I saw that Christophe Demko create a pull request for that:
https://github.com/joomla/joomla-platform/pull/397
But the final status: "Closing this one. The MVC will be reworked after 11.4"

So finally it's seems that this "error" will not be fixed in 2.5.X ?

Anyway I will try to post a fix for 2.5.X :-) in the staging branch. This will be a good traning :)
But how to submit a request on the framework version used for 2.5 versus submit a request on the framework for 3.0 ?
If my version is "stupid" don't answer it, I will search by myself

Thomas

2012/7/24 Sam Moffatt <pas...@gmail.com>

[Elin Waring]

[Sam Moffatt]

I expect some of that is collateral damage that occurred. JTable isn't
part of the MVC rework so while the code was in there the majority of
the code in the pull request related to the MVC. I don't see any major
problem with extracting the JTable changes into a new pull request.

Cheers,

Sam Moffatt
http://pasamio.id.au