Bug #34152

stdWrap function numRows() fails in 4.6 due to wrong SELECT clause

Added by Alain over 7 years ago. Updated 12 months ago.

Status:
Closed
Priority:
Should have
Category:
Frontend
Target version:
-
Start date:
2012-02-21
Due date:
% Done:

100%

TYPO3 Version:
4.5
PHP Version:
5.3
Tags:
Complexity:
Is Regression:
Sprint Focus:

Description

Hello,

this piece of TS does not work anymore since an update from 4.5.11 to 4.6.4:

    ATagParams.cObject = TEXT
    ATagParams.cObject {
            value = class="hasSubpage" 
        if.isTrue.numRows {
                table = pages
            select {
                pidInList.field = uid
                where = doktype IN (1,199)
            }
        }
    }

While tracking tslib_cObj::getQuery(), I found out that the sanitizeSelectPart() is introcuced since 4.6. This uses the PHP function strpos() to check if wildcard is set in the SELECT clause, which leads to wrong SQL query in the special case of tslib_cObj::numRows(), where 'count(*)' is set in selectFields property.

The retrieved SQL query would be:

SELECT     count(*), pages.uid as uid, pages.pid as pid, pages.t3ver_state as t3ver_state
FROM     pages
WHERE     pages.pid IN (6) AND nav_hide!=1 AND doktype!=5 AND doktype!=6 AND pages.deleted=0 AND pages.hidden=0 AND pages.starttime<=1329827460 AND (pages.endtime=0 OR pages.endtime>1329827460) AND NOT pages.t3ver_state>0 AND pages.doktype<200 AND (pages.fe_group='' OR pages.fe_group IS NULL OR pages.fe_group='0' OR FIND_IN_SET('0',pages.fe_group) OR FIND_IN_SET('-1',pages.fe_group))

but the expected SQL query should be:

SELECT     count(*)
FROM     pages
WHERE     pages.pid IN (6) AND nav_hide!=1 AND doktype!=5 AND doktype!=6 AND pages.deleted=0 AND pages.hidden=0 AND pages.starttime<=1329827460 AND (pages.endtime=0 OR pages.endtime>1329827460) AND NOT pages.t3ver_state>0 AND pages.doktype<200 AND (pages.fe_group='' OR pages.fe_group IS NULL OR pages.fe_group='0' OR FIND_IN_SET('0',pages.fe_group) OR FIND_IN_SET('-1',pages.fe_group))

I would suggest to exclude "count(*)" in the if statement of tslib_cObj::sanitizeSelectPart():

    protected function sanitizeSelectPart($selectPart, $table) {
        // pattern matching parts
        $matchStart = '/(^\s*|,\s*|' . $table . '\.)';
        $matchEnd = '(\s*,|\s*$)/';

        $necessaryFields = array('uid', 'pid');
        $wsFields = array('t3ver_state');

-        if (isset($GLOBALS['TCA'][$table]) && strpos($selectPart, '*')!== FALSE) {
+        if (isset($GLOBALS['TCA'][$table]) && strpos($selectPart, '*')!== FALSE && $selectPart != 'count(*)') {
            foreach ($necessaryFields as $field) {
                $match = $matchStart . $field . $matchEnd;
                if (!preg_match($match, $selectPart)) {
                    $selectPart .= ', ' . $table . '.' . $field . ' as ' . $field;
                }
            }

            if ($GLOBALS['TCA'][$table]['ctrl']['versioningWS']) {
                foreach ($wsFields as $field) {
                    $match = $matchStart . $field . $matchEnd;
                    if (!preg_match($match, $selectPart)) {
                        $selectPart .= ', ' . $table . '.' . $field . ' as ' . $field;
                    }
                }
            }
        }
        return $selectPart;
    }

Thank you and best regards,

Alain


Related issues

Related to TYPO3 Core - Bug #17284: no content in workspace preview when using select.selectFields Closed 2007-05-07
Related to TYPO3 Core - Bug #29783: getQuery: fields explicilty added on select <table>.* : Error: ambigious parameter uid Closed 2011-10-11
Related to TYPO3 Core - Bug #39634: Solved Bug #34152 results in a new interpretation of my TypoScript Rejected 2012-08-09
Related to TYPO3 Core - Bug #43114: sanitizeSelectPart - select CONTENT using DISTINCT New 2012-11-19

Associated revisions

Revision 481d1ffa (diff)
Added by Ernesto Baschny about 7 years ago

[BUGFIX] stdWrap numRows fails due to wrong SELECT clause

Exclude aggregate functions count(), sum(), max(),
min(), avg() in if statement inside
tslib_cObj::sanitizeSelectPart().

Change-Id: I5d8cd5f00472b417dad3c8790b1cc75f3cfd473a
Fixes: #34152
Releases: 4.5, 4.6, 4.7, 6.0
Reviewed-on: http://review.typo3.org/9158
Reviewed-by: Christian Kuhn
Tested-by: Christian Kuhn
Reviewed-by: Alain
Tested-by: Alain
Reviewed-by: Wouter Wolters
Tested-by: Wouter Wolters
Reviewed-by: Stefan Neufeind
Reviewed-by: Ernesto Baschny
Tested-by: Ernesto Baschny

Revision ac4f234e (diff)
Added by Ernesto Baschny about 7 years ago

[BUGFIX] stdWrap numRows fails due to wrong SELECT clause

Exclude aggregate functions count(), sum(), max(),
min(), avg() in if statement inside
tslib_cObj::sanitizeSelectPart().

Change-Id: Ie2efa1446b2b3a6fed4e31b110b46afbcd178c88
Fixes: #34152
Releases: 4.5, 4.6, 4.7, 6.0
Reviewed-on: http://review.typo3.org/12997
Reviewed-by: Ernesto Baschny
Tested-by: Ernesto Baschny

Revision 8621c145 (diff)
Added by Ernesto Baschny about 7 years ago

[BUGFIX] stdWrap numRows fails due to wrong SELECT clause

Exclude aggregate functions count(), sum(), max(),
min(), avg() in if statement inside
tslib_cObj::sanitizeSelectPart().

Change-Id: I0e28e382cbfaaafa8d715044322a2c8ce1364ba4
Fixes: #34152
Releases: 4.5, 4.6, 4.7, 6.0
Reviewed-on: http://review.typo3.org/12998
Reviewed-by: Ernesto Baschny
Tested-by: Ernesto Baschny

Revision 8e944f09 (diff)
Added by Ernesto Baschny about 7 years ago

[BUGFIX] stdWrap numRows fails due to wrong SELECT clause

Exclude aggregate functions count(), sum(), max(),
min(), avg() in if statement inside
tslib_cObj::sanitizeSelectPart().

Change-Id: I1fc1360ffc239695f0c04ca2322870a2129133ec
Fixes: #34152
Releases: 4.5, 4.6, 4.7, 6.0
Reviewed-on: http://review.typo3.org/12999
Reviewed-by: Ernesto Baschny
Tested-by: Ernesto Baschny

Revision b846f3d2 (diff)
Added by Ernesto Baschny about 7 years ago

[BUGFIX] stdWrap numRows fails due to wrong SELECT clause

Exclude aggregate functions count(), sum(), max(),
min(), avg() in if statement inside
tslib_cObj::sanitizeSelectPart().

Change-Id: I5d8cd5f00472b417dad3c8790b1cc75f3cfd473a
Fixes: #34152
Releases: 4.5, 4.6, 4.7, 6.0
Reviewed-on: http://review.typo3.org/9158
Reviewed-by: Christian Kuhn
Tested-by: Christian Kuhn
Reviewed-by: Alain
Tested-by: Alain
Reviewed-by: Wouter Wolters
Tested-by: Wouter Wolters
Reviewed-by: Stefan Neufeind
Reviewed-by: Ernesto Baschny
Tested-by: Ernesto Baschny

History

#1 Updated by Stefan Galinski over 7 years ago

Sounds like you already solved the bug. Can you create a patch against the master branch and open a new review request please? I also added Helmut as watcher as this seems to be security related!?

http://wiki.typo3.org/Contribution_Walkthrough_Tutorials

#2 Updated by Alain over 7 years ago

Hello,

thanks for your feedback. I am not yet familiar with git & gerrit so I need time to learn more about, sorry. At the moment, I would appreciate if someone else can create a patch for this bug.

Best regards,

Alain

#3 Updated by Gerrit Code Review over 7 years ago

  • Status changed from New to Under Review

Patch set 1 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9158

#4 Updated by Georg Ringer over 7 years ago

hi,

i tested this with current master

page.20 = TEXT
page.20 {
          value = class="hasSubpage" 
    if.isTrue.numRows {
            table = pages
      select {
        pidInList.field = uid
        where = doktype IN (1,199)
      }
    }
}

and it puts out class="hasSubpage" if there is a subpage, IMO this works. can you check the master?

#5 Updated by Gerrit Code Review over 7 years ago

Patch set 2 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9158

#6 Updated by Wouter Wolters over 7 years ago

Tested in master and 4.6 installation and the given example is working.

#7 Updated by Alain over 7 years ago

I have cloned the master branch from repository git://git.typo3.org/TYPO3v4/Core.git to my local machine and I can reproduce it in 4.7-dev. I'm now not sure if this issue is system specific. I'm running Debian Etch, MySQL 5.0.32, PHP 5.3.4. I had never problem with this constellation since 4.2 until 4.5.

#8 Updated by Markus Klein over 7 years ago

Alain, in the ticket you say TYPO3-Version 4.6.
Now you say "..never had problem ... until 4.5"

To clarify things: Affected versions 4.5+ according to you?

#9 Updated by Alain over 7 years ago

the affected version is 4.6.4.

To clarify what I meant by "I had never problem with this constellation since 4.2 until 4.5", let me say in german:

Seit 4.2 bis einschließlich 4.5 hatte ich nie Probleme mit meiner System-Konstellation. Das o.g. TS-Snippet funktioniert bei mir in 4.5.11 tadellos.
Ich hatte in meinem englischen Satz das Wort inclusively vergessen, sorry.

The TS snippet that I've mentionned above, works great in 4.5.11. But since I updated to 4.6.4, I am confronted with this issue.

Meanwhile, I have tested the retrieved SQL query on shell command and I get an error:

mysql> use typo3db;
Database changed

mysql> SELECT count(*), pages.uid as uid, pages.pid as pid, pages.t3ver_state as t3ver_state FROM pages WHERE pages.pid IN (6) AND nav_hide!=1 AND doktype!=5 AND doktype!=6 AND pages.deleted=0 AND pages.hidden=0 AND pages.starttime<=1329827460 AND (pages.endtime=0 OR pages.endtime>1329827460) AND NOT pages.t3ver_state>0 AND pages.doktype<200 AND (pages.fe_group='' OR pages.fe_group IS NULL OR pages.fe_group='0' OR FIND_IN_SET('0',pages.fe_group) OR FIND_IN_SET('-1',pages.fe_group));

ERROR 1140 (42000): Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause

#10 Updated by Alain over 7 years ago

Patch Set 2 available at http://review.typo3.org/9158 solves this issue. Tested on my current 4.6 installation and on master 4.7-dev.

#11 Updated by Maik Matthias over 7 years ago

Same problem with TYPO3 version 4.5.13. The provided patch works fine with 4.5.13 too. IMHO fixing this bug is a "must have" because the numRows function is very common.

#12 Updated by TO_Webmaster no-lastname-given over 7 years ago

Yeah, please fix this in 4.5.

I also suggest to check for other aggregate functions, too.

#13 Updated by Marcus Krause over 7 years ago

We're also affected, using 4-5 branch (LTS).

#14 Updated by Gerrit Code Review over 7 years ago

Patch set 3 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9158

#15 Updated by Ernesto Baschny over 7 years ago

  • Category set to Frontend
  • TYPO3 Version changed from 4.6 to 4.5

This problem also affects TYPO3_4-5, since the fix #17284 was also backported to the 4.5 branch and released with 4.5.13. So it needs to be fixed there as well (regression).

Please note that the numRows problem only seem to affect MySQL 5.0, as with MySQL 5.1 and later this is not considered wrong SQL anymore, which is why the problem is probably not that "wide-spread".

MySQL 5.0:

mysql> select count(*),uid from pages;
ERROR 1140 (42000): Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause

MySQL 5.1:

mysql> select count(*),uid from pages;
-------------+ | count(*) | uid |
-------------+ | 769 | 1 |
-------------+
1 row in set (0.00 sec)

#16 Updated by Manfred Egger over 7 years ago

Please consider to make

$selectPart != 'count(*)'
case insensitive because
count(*)
is often written as
COUNT(*)

#17 Updated by Gerrit Code Review over 7 years ago

Patch set 4 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9158

#18 Updated by Gerrit Code Review over 7 years ago

Patch set 5 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9158

#19 Updated by Gerrit Code Review over 7 years ago

Patch set 6 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9158

#20 Updated by Gerrit Code Review over 7 years ago

Patch set 7 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9158

#21 Updated by Gerrit Code Review over 7 years ago

Patch set 8 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9158

#22 Updated by Gerrit Code Review over 7 years ago

Patch set 9 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9158

#23 Updated by Fedir RYKHTIK about 7 years ago

4.5 +1

patch works fine

#24 Updated by Jan Simbera about 7 years ago

4.5 (4.5.17) +1

patch working

#25 Updated by BenBaghira no-lastname-given about 7 years ago

4.5 (4.5.17) +1

patch working

#26 Updated by Gerrit Code Review about 7 years ago

Patch set 10 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9158

#27 Updated by Ernesto Baschny about 7 years ago

  • Assignee set to Ernesto Baschny
  • % Done changed from 0 to 50

Ok, the latest patch 10 now adds a unit test for the "getQuery" method which has been broken and fixed over and over. It includes test cases for the relevant issues (#17284, #29783, #34152).

Please review so that we can get that into the whole "chain" of releases (4.5 .. 6.0) soon.

Thanks!

#28 Updated by Gerrit Code Review about 7 years ago

Patch set 11 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9158

#29 Updated by Gerrit Code Review about 7 years ago

Patch set 12 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9158

#30 Updated by Gerrit Code Review about 7 years ago

Patch set 13 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/9158

#31 Updated by Ernesto Baschny about 7 years ago

Please note that the "rowNums" problem due to wrong SQL only pops up with MySQL 5.0 (e.g. debian lenny), so you won't be able to reproduce the bug in MySQL 5.1 (debian squeeze).

Test with this snippet:

page.1 = TEXT
page.1.value = TEST
page.1.if.isTrue.numRows {
table = pages
select {
pidInList.field = uid
where = doktype IN (1,199)
}
}

You should see "TEST" on top of your page if the current page has subpages. If you never see TEST, it means the SQL failed.

#32 Updated by Gerrit Code Review about 7 years ago

Patch set 1 for branch TYPO3_4-7 has been pushed to the review server.
It is available at http://review.typo3.org/12997

#33 Updated by Gerrit Code Review about 7 years ago

Patch set 1 for branch TYPO3_4-6 has been pushed to the review server.
It is available at http://review.typo3.org/12998

#34 Updated by Gerrit Code Review about 7 years ago

Patch set 1 for branch TYPO3_4-5 has been pushed to the review server.
It is available at http://review.typo3.org/12999

#35 Updated by Ernesto Baschny about 7 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 50 to 100

#36 Updated by Gerrit Code Review about 7 years ago

  • Status changed from Resolved to Under Review

Patch set 1 for branch TYPO3_4-7 has been pushed to the review server.
It is available at http://review.typo3.org/13202

#37 Updated by Ernesto Baschny about 7 years ago

  • Status changed from Under Review to Resolved

#38 Updated by Gerrit Code Review about 7 years ago

  • Status changed from Resolved to Under Review

Patch set 1 for branch TYPO3_4-6 has been pushed to the review server.
It is available at http://review.typo3.org/13309

#39 Updated by Stefan Froemken about 7 years ago

Since you have pushed the solution for this report to current TYPO3 4.6.11 there are problems with TypoScript-parts using "select". Here is my problem which I have posted into the core list, too:

The field "uid" is not the "uid" out of the select-part anymore. My TS returns following records:

224 Tceforms // not linked
225 Ctrl // not linked
69 fluid // not linked
14 Warum jQuery? // linked
2 Home // linked
255 Form // not linked
252 Buttons // not linked

But the Link "Home" shows to Record "index.php?id=40" instead of "2"
and the link "Warum jQuery?" shows to record index.php?id=8" instead of "14"

Here is my TypoScript:

10 = CONTENT
10 {
table = tt_content
select {
orderBy = MAX DESC
groupBy = tt_content.pid
pidInList = 1
recursive = 10
max = 7
where = nav_hide = 0
selectFields = pages.uid, pages.title
leftjoin = pages ON (tt_content.pid = pages.uid)
}

renderObj = TEXT
renderObj {
field = title
required = 1
typolink.parameter.field = uid // maybe here is the problem
typolink.ATagParams = class=internal-link
wrap = &lt;li&gt;|&lt;/li&gt;
}
wrap = &lt;h2&gt;Meine zuletzt bearbeiteten Inhalte&lt;/h2&gt;&lt;ul&gt;|&lt;/ul&gt;
}

And here is the resulting Query:

SELECT pages.uid, pages.title
FROM tt_content
LEFT JOIN pages ON ( tt_content.pid = pages.uid )
WHERE nav_hide =0
GROUP BY tt_content.pid
ORDER BY MAX DESC
LIMIT 7

#40 Updated by Markus Klein about 7 years ago

@Stefan: Not a bug! See #39634.

#41 Updated by Oliver Hader about 7 years ago

  • Status changed from Under Review to Resolved

Closing issue due to notes in issue #39634 and accordant discussion in the core mailing list (thread "Announcing TYPO3 4.5.18, 4.6.11 and 4.7.3")

#42 Updated by Benni Mack 12 months ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF