[TYPO3-core] RFC #12351: Clean-up SQL parser and SQL engine

[Ernesto Baschny [cron IT]]

Xavier Perseguers schrieb:

>>> Problem:
>>> This patch adds visibility to methods of t3lib_sqlengine and
>>> t3lib_sqlparser and applies CGL on modified lines.
>>>
>>> Purpose is to ease creation of unit tests.
>>
>> You choose to make these functions private:
>>
>> t3lib_sqlparser:
>> parseSELECT
>> parseUPDATE
>> parseINSERT
>> parseDELETE
>> parseEXPLAIN
>> parseCREATETABLE
>> parseALTERTABLE
>> parseDROPTABLE
>> parseCREATEDATABASE
>> nextPart
>> getValue
>> getValueInQuotes
>> parseStripslashes
>> parseError
>> trimSQL
>> compileSELECT
>> compileUPDATE
>> compileDELETE
>>
>> t3lib_sqlengine:
>> processAccordingToConfig
>>
>> How can you be sure that noone else is accessing / using these function
>> as they have always been public before?
> 
> I cannot of course. I checked that DBAL - which is the only extension I
> know that is extending those classes - was still able to work. And I
> tried to restrict as much as possible the visibility.

I understand. I know at least extdeveval which makes direct use of the
SQL-parsing methods of t3lib_sqlparse. It uses it like this:

  $sqlParser = t3lib_div::makeInstance('t3lib_sqlparser');
  $result = $sqlParser->parseSQL($this->inputCalc['sql']['input']);
  ...

As far as I can see none of your suggested private methods are being used.

And those look more like of "internal use" anyway. So I would tend to +1
on that decision.

> It's a bit tricky in all cases as we have this in Core:
> 
> class t3lib_sqlengine extends t3lib_sqlparser
> 
> and in DBAL:
> 
> class ux_t3lib_sqlengine extends t3lib_sqlengine
> class ux_t3lib_sqlparser extends t3lib_sqlparser
> 
> Meaning we loose in ux_t3lib_sqlengine what was overriden in
> ux_t3lib_sqlparser, which is why we have to copy lots of code from
> ux_t3lib_sqlparser.

Urgh, yea that looks ugly. Correct would be that sqlengine doesn't
extend sqlparser, but instead internally make use of sqlparser. Probably
"too late" to change that.

> Anyway, this is an attempt to restrict access and make visibility more
> "official" and this is an RFC, meaning if you don't agree to some
> visibility or find that something breaks, you may provide another patch.
> Of course it would be easier to make all methods public arguing that
> they were implicitly public before but I prefer trying to make a
> "breaking" change that should not be one at all as I pretend that
> extension authors use exec_ methods if they did their code the way they
> should and many simply used mysql_* calls directly.

True, I don't worry about general extensions, they should not break. But
in particular extensions where the author needed some "SQL parsing" and
found out "cool, TYPO3 provides a nice class to do just that" and makes
use of the "API" which is documented.

But I agree that they most probably haven't used that "internal
methods", so I +1 by reading your patch.

Cheers,
Ernesto