I'm still deeply opposed to commit wilsonge@80cb859 from this PR.
Saving a few extra-lines of code on one column and loosing proper software design abstraction is against our coding standards imho.
See hidden comments above for the discussion.

which abstraction need in this case?
we have a problem that json_encode work different on win or unix, or someone will compile this code to javascript?

Ok, to clarify some CS basics on this example: :)

• JRegistry is a class representing a registry and abstracting its private implementation
• json_encode is a json encoding implementation
• JRegistry happens today to use json. If tomorrow or on site xyz, it uses xml, only one place needs to change, and other places don't have to know details.

See the difference ?

More information on that CS subject (among many other good books): http://en.wikipedia.org/wiki/Abstraction_%28computer_science%29

that right, but in this case it unnecessary because:

if (!empty($this->jsonEncode))

alludes

Indeed wrong too! I didn't review rest of PR yet.
Then that added line needs to be fixed to follow proper CS (Computer Science) semantics too. ;)

So @beat would it be fair to say that in general the PR is fine - it's just whether to use json_encode or to keep JRegistry (and if so the change the corresponding class variable name)? But otherwise you are happy?

First of all we should test if this works. I have this already for my own extensions and I'm using JRegistry so maybe it's required because the format isn't right just with json_encode.

If both systems work I would use json_encode. We are performing a very specific action and JRegistry here only consumes resources and adds a dependency. In the future if we need to encode to a different format we will need to replace toString() with the new encoding anyway.

It definitely does work both with both json_encode and JRegistry as unit tests passed before and after (ignore travis saying it doesn't - it definitely did - it's because I tried rebasing and I'm having to try and do it with my home PC which isn't liking me)

Also I'd add if the format needs to change we can't do it before J4 anyhow in which case we can change the variable name and move to whatever JRegistry Object we want anyhow :)

@wilsonge for the general idea (except for "hardcoding" outside of JRegistery its implementation).

It makes sure sense to move the Registery-encoding down to JTable level, as it removes lots of duplicate code in every child class, and brings a consistency back.

I'm still thinking that there could be an even more elegant way to do this cleanly separated from JTable but invoked by JTable.

But more importantly remove the dependancy of JRegistry in JTable using either a DI pattern or an Observer pattern.

I think that a JTableObserver could implement that part, called by the JTable bind(). E.g. JTableObserverRegistry could listen to bind() and do the job.

function bind could have this at the begin:

    // Implement JObservableInterface: Pre-processing by observers
$this->_observers->update('onBeforeBind', array($src, $ignore));

(and similar to e.g. load() obserer at the end)

instead of your loops.

It would removes the dependancy, while implementing a much more extensible pattern for binding. Actually those observer calls in bind() seem to have been forgotten yet.

The observer itself would take as constructor parameter the list of columns which are JRegistry and thus could be a unique class for all JTables. Now the last thing to determine here is where to attach the JTableObserverRegistry to the JTableSomething object. That could be done using the JObserverMapper class, like for other observers. There is a part which is missing in Joomla right now to complete the proper place to invoke the JObserverMapper, but we could take this opportunity to add it for 3.3...

My gut says it shouldn't be part of the observer because we can't put an array into this field. It HAS to be json encoded (otherwise JTable::store just fails) and therefore should be directly done in the table class.

I'm open to an onBeforeBind method but I think that's outside the scope of this PR.

but we could take this opportunity to add it for 3.3...

I think for 3.3 it's to late. We're already close to the second beta and I'm not a huge fan of adding new classes at this time.

I'm also with Roberto on the json_encode question. As long as it produces the same result, I don't see why we need JRegistry just to convert an array to an json string. We're not using any specific JRegistry features here as far as I see.

for json_encode and I see that @wilsonge already have changed it :)

I guess this is a typical case of Software Architecture vs Coder implementation. For what it's worth:
from me for json_encode as it breaks the fundamental rule of encapsulating data handling, an important Software Architecture principle that is the basis of MVC that was embraced by Joomla!

@wilsonge Great work, thanks! Tested successfully.

Issues with test:
JDatabaseDriverMysqlTest.testGetCollation
JDatabaseDriverMysqliTest.testGetCollation

JDatabaseDriverMysqlTest::testGetCollation
Line:130 The getCollation method should return the collation of the database.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'utf8_general_ci'
+'latin1_swedish_ci'

/var/lib/jenkins/jobs/cms/workspace/tests/unit/suites/database/driver/mysql/JDatabaseDriverMysqlTest.php:131

@infograf768 That issue sounds more related to this PR: #3886. Or not?

Yeah that will be #3886 not this one only changes the bind method not the getCollation one

@Bakual and @wilsonge

Yep, indeed.