? Success

User tests: Successful: Unsuccessful:

avatar okonomiyaki3000
okonomiyaki3000
22 Sep 2014

There are several cases of table classes which internally use instances of themselves or other table classes but get these instances without specifying a dbo. In normal Joomla operation, this will never be a problem but, when you try to do something funny, like manipulate tables on some other db, you may have problems.

FOFTable may have the same issue but I don't understand the purpose of that class well enough to touch it right now.

avatar okonomiyaki3000 okonomiyaki3000 - open - 22 Sep 2014
avatar jissues-bot jissues-bot - change - 22 Sep 2014
Labels Added: ?
avatar okonomiyaki3000
okonomiyaki3000 - comment - 22 Sep 2014

By the way, do I need to do something with the new tracker now? I'm not so familiar with the new system.

avatar Bakual
Bakual - comment - 22 Sep 2014

By the way, do I need to do something with the new tracker now? I'm not so familiar with the new system.

No special actions are needed. issues.joomla.org automatically syncs with GitHub.

avatar brianteeman brianteeman - change - 22 Sep 2014
Category Libraries
avatar okonomiyaki3000
okonomiyaki3000 - comment - 20 Nov 2014

:+1:

avatar okonomiyaki3000
okonomiyaki3000 - comment - 1 Dec 2014

Testing this change is basically impossible without a very special setup involving at least two databases and some custom code. I would hope that the nature of these five, identical, single-line changes would be simple and obvious enough to skip that but I can explain what kind of ill effects you can expect.

As an example, let's say you are using a JTableContent to add or update a row on a #__content table in an external database (meaning, not the db you get when calling JFactory::getDBO(). Why would you do this? Who cares, that's not the point.). This table will do a check to make sure you are not saving two items with the same alias in the same category but it does this using another instance of JTableContent. If you are working with an external database, this other instance should also connect to the same database. Currently it doesn't, it connects to the local one. So the check that it performs is totally meaningless. It might prevent you from saving for no reason at all, it might allow you to save when it really shouldn't.

The case for other tables might be slightly different but the general idea is the same. If the table class needs to do some check using another instance of a JTable class, it needs to make sure it is configured for the same db.

avatar rdeutz
rdeutz - comment - 1 Dec 2014

Seems ok for me. Not sure if we need tests here, it is only a usage of the existing API and it makes sense to use the same db. I would say we can merge it.

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4318.

avatar Bakual
Bakual - comment - 1 Dec 2014

Just test that existing functionality still works.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 1 Dec 2014

How about FOFTable? I didn't touch it in this PR but what is it and how is it used? It might need some similar fix.

avatar Bakual
Bakual - comment - 1 Dec 2014

You would have to submit that PR to the FOF repo as it's a 3rd party library.

avatar okonomiyaki3000
okonomiyaki3000 - comment - 1 Dec 2014

But does Joomla use it in any way? Or is there some plan to use it in the future?

avatar Bakual
Bakual - comment - 1 Dec 2014

Joomla does only use FOF for the postinstall messages and the TFA.
But 3rd party extensions may use FOF also.

avatar Hackwar
Hackwar - comment - 9 Dec 2014

Looked over the code and can only give it a thumbs up :+1: Looks good and will make testing easier.

Regarding FOF: I guess the longterm plan is to remove it from Joomla again, right? Especially since FOF does not get any support anymore and rather F0F has a community that supports it. Or did I misunderstand that?

avatar wilsonge wilsonge - change - 27 Dec 2014
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2014-12-27 15:56:14
avatar wilsonge wilsonge - close - 27 Dec 2014
avatar wilsonge wilsonge - reference | 8ae91c6 - 27 Dec 14
avatar wilsonge wilsonge - merge - 27 Dec 2014
avatar wilsonge wilsonge - close - 27 Dec 2014
avatar wilsonge
wilsonge - comment - 27 Dec 2014

Merging on review.

avatar wilsonge wilsonge - change - 27 Dec 2014
Milestone Added:
avatar okonomiyaki3000 okonomiyaki3000 - head_ref_deleted - 29 Dec 2014

Add a Comment

Login with GitHub to post a comment