Bug #29783

getQuery: fields explicilty added on select <table>.* : Error: ambigious parameter uid

Added by Björn Pedersen almost 8 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
Should have
Category:
Database API (Doctrine DBAL)
Target version:
Start date:
2011-10-11
Due date:
% Done:

100%

TYPO3 Version:
4.6
PHP Version:
Tags:
Complexity:
Is Regression:
Sprint Focus:

Description

tslib_content->getQuery add ui and pid to the selected fields if fields are set. It does not check, wether a tablename.* occurs.

That occurs e.g. using cal_ts_service to link seminars events to cal calenders. There the query needs to contain table.title as mytitle.

Solution: Check, if a * is part of the requested fields.


Subtasks

Task #30762: Add Unit tests for #29783Rejected


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 #34152: stdWrap function numRows() fails in 4.6 due to wrong SELECT clause Closed 2012-02-21
Duplicated by TYPO3 Core - Bug #30486: Typoscript select join broken Closed 2011-09-30

Associated revisions

Revision 1afc0014 (diff)
Added by Björn Pedersen almost 8 years ago

[BUGFIX] Don't add uid,pid if <table>.'*' is present in field list.

tx_cal_tsservice uses queries with select <tablename>.* and
complex joins. This breaks due to the unconditionl adding of uid,pid
if the query is set.

Do not add uid,pid,... to field list if the field list already contains a
'*' or the repective field is in the list.

To keep the code easier to read, the sanitizing has been moved
to a seperate function.

The regexp matching is necessary to detect fields like post_uid
(comments extension).

Change-Id: I50332c22e627ea452aaee233fdbbdf3dd426a1b6
Resolves: #29783,#30486
Releases: 4.6
Reviewed-on: http://review.typo3.org/4974
Reviewed-by: Benjamin Mack
Tested-by: Benjamin Mack
Reviewed-by: Christof Rodejohann
Tested-by: Christof Rodejohann
Reviewed-by: Tolleiv Nietsch
Tested-by: Tolleiv Nietsch

History

#1 Updated by Mr. Hudson almost 8 years ago

Patch set 1 of change I50332c22e627ea452aaee233fdbbdf3dd426a1b6 has been pushed to the review server.
It is available at http://review.typo3.org/4974

#2 Updated by Mr. Hudson almost 8 years ago

Patch set 2 of change I50332c22e627ea452aaee233fdbbdf3dd426a1b6 has been pushed to the review server.
It is available at http://review.typo3.org/4974

#3 Updated by Mr. Hudson almost 8 years ago

Patch set 3 of change I50332c22e627ea452aaee233fdbbdf3dd426a1b6 has been pushed to the review server.
It is available at http://review.typo3.org/4974

#4 Updated by Tobias Liebig almost 8 years ago

  • Category set to Database API (Doctrine DBAL)
  • Status changed from New to Under Review
  • Assignee set to Björn Pedersen

#5 Updated by Björn Pedersen almost 8 years ago

The TS from cal_ts_service:

plugin.tx_cal_controller {
        display {
                seminars {

                        # @description  This is the heart of the connection SQL. It will be used in conjunction with 
                        #                               the view-where (findallWithinWhere, findAll and findWhere) to retrieve the according records.
                        event_select {
                                selectFields = tx_seminars_seminars.*, tx_seminars_timeslots.*, tx_seminars_timeslots.begin_date as bgndt, tx_seminars_timeslots.end_date as nddt, tx_seminars_seminars.title as mytitle, tx_seminars_timeslots.begin_date as my_startdate, tx_seminars_timeslots.end_date as my_enddate
                                leftjoin = tx_seminars_timeslots ON (tx_seminars_seminars.uid=tx_seminars_timeslots.seminar)
                        }

}

#6 Updated by Mr. Hudson almost 8 years ago

Patch set 4 of change I50332c22e627ea452aaee233fdbbdf3dd426a1b6 has been pushed to the review server.
It is available at http://review.typo3.org/4974

#7 Updated by Helmut Hummel almost 8 years ago

  • Status changed from Under Review to Accepted
  • Priority changed from Should have to Must have
  • Target version set to 4.6.0-RC1

This needs to be resolved in RC1 or #17284 must be reverted.

#8 Updated by Mr. Hudson almost 8 years ago

Patch set 5 of change I50332c22e627ea452aaee233fdbbdf3dd426a1b6 has been pushed to the review server.
It is available at http://review.typo3.org/4974

#9 Updated by Christof Rodejohann almost 8 years ago

While fixing this issue please take into account problems arising from http://forge.typo3.org/issues/30486

Especially uid, pid and t3ver_state should never be used without especially defining the table. This is because when using a join you will always have uid, pid and t3ver in both tables. Therefore resulting in a "Error: ambigious parameter uid".

Furthermore we should take some time to think about how workspace's should be handled for the second part of the join.

#10 Updated by Steffen Gebert almost 8 years ago

  • Status changed from Accepted to Under Review

#11 Updated by Björn Pedersen almost 8 years ago

@Christof Rodejohann:

That#s what this patch is about.

#12 Updated by Mr. Hudson almost 8 years ago

Patch set 6 of change I50332c22e627ea452aaee233fdbbdf3dd426a1b6 has been pushed to the review server.
It is available at http://review.typo3.org/4974

#13 Updated by Mr. Hudson almost 8 years ago

Patch set 7 of change I50332c22e627ea452aaee233fdbbdf3dd426a1b6 has been pushed to the review server.
It is available at http://review.typo3.org/4974

#14 Updated by Christof Rodejohann almost 8 years ago

@Björn Pedersen

Kind of. The latest patch still fails on a basic join. I am quoting my test setup from http://forge.typo3.org/issues/30486

page = PAGE
page.10 = COA
page.10 {

  10 = CONTENT
  10 {
    table = tt_content
    select {            
      selectFields = tt_content.header,be_users.username
      join = be_users ON tt_content.cruser_id = be_users.uid
    }
    renderObj = COA
    renderObj {
      10 = TEXT
      10.field = header

      20 = TEXT
      20.field = username
    }  
  }
}

This setup fails with the latest patch and error "Column 'uid' in field list is ambiguous".

That is because you add the raw uid,pid and t3ver_state without the explicit database table name. You focus on the selectFields and table part. But the join setup adds another table and contains a different set of uid,pid and t3ver_state.

Adding the table name to uid,pid and t3ver_state from $table and aliasing them back to uid,pid and t3ver_state would be an easy fix. But how should workspace's be handled for the second part of the join?

#15 Updated by Björn Pedersen almost 8 years ago

@Christof:

The workspace part for the join probably is one of the border cases that I can not fix at the moment. In such a case it is still the responibility of the TS writer to ensure all the right fields are selected.

#16 Updated by Christof Rodejohann almost 8 years ago

@Björn:

IMHO this is fine. I am trying to push a patch for the $table part, so you won't have to do all the work. But i am having a lot of trouble with gerrit.

#17 Updated by Mr. Hudson almost 8 years ago

Patch set 8 of change I50332c22e627ea452aaee233fdbbdf3dd426a1b6 has been pushed to the review server.
It is available at http://review.typo3.org/4974

#18 Updated by Christof Rodejohann almost 8 years ago

@Björn:

Patch set 8 resolves the join issue from http://forge.typo3.org/issues/30486. I believe this bug is resolved. Good work.

#19 Updated by Mr. Hudson almost 8 years ago

Patch set 9 of change I50332c22e627ea452aaee233fdbbdf3dd426a1b6 has been pushed to the review server.
It is available at http://review.typo3.org/4974

#20 Updated by Mr. Hudson almost 8 years ago

Patch set 10 of change I50332c22e627ea452aaee233fdbbdf3dd426a1b6 has been pushed to the review server.
It is available at http://review.typo3.org/4974

#21 Updated by Mr. Hudson almost 8 years ago

Patch set 11 of change I50332c22e627ea452aaee233fdbbdf3dd426a1b6 has been pushed to the review server.
It is available at http://review.typo3.org/4974

#22 Updated by Björn Pedersen almost 8 years ago

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

#23 Updated by Riccardo De Contardi over 1 year ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF