From 0433ba1642efdd32c5667c12cc3ef7f2f5e7db04 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Tue, 15 Jul 2014 09:31:03 +1200 Subject: [PATCH] BUG Revert some changes to ManyManyList BUG Fix incompatibility in Member_GroupList Fix regressions in merges from 3.1 BUG Fix Security failing on test classes BUG Fix postgresql compatibility Clarify sql encoding of table names --- control/RequestProcessor.php | 4 +- dev/FixtureFactory.php | 4 +- docs/en/changelogs/3.2.0.md | 2 + model/ManyManyList.php | 108 +++++++++++++-------- model/connect/DBSchemaManager.php | 5 +- model/queries/SQLConditionalExpression.php | 15 +-- model/queries/SQLDelete.php | 2 + model/queries/SQLInsert.php | 6 +- model/queries/SQLSelect.php | 2 + model/queries/SQLUpdate.php | 4 +- security/Member.php | 14 +-- security/Security.php | 3 + tests/model/ManyManyListTest.php | 6 +- 13 files changed, 103 insertions(+), 72 deletions(-) diff --git a/control/RequestProcessor.php b/control/RequestProcessor.php index e391be0a5..aebdd4d50 100644 --- a/control/RequestProcessor.php +++ b/control/RequestProcessor.php @@ -2,7 +2,7 @@ /** * Represents a request processer that delegates pre and post request handling to nested request filters - * + * * @package framework * @subpackage control */ @@ -21,7 +21,7 @@ class RequestProcessor implements RequestFilter { /** * Assign a list of request filters - * + * * @param array $filters */ public function setFilters($filters) { diff --git a/dev/FixtureFactory.php b/dev/FixtureFactory.php index 1b01535b2..0c49ce533 100644 --- a/dev/FixtureFactory.php +++ b/dev/FixtureFactory.php @@ -91,9 +91,9 @@ class FixtureFactory { public function createRaw($table, $identifier, $data) { $fields = array(); foreach($data as $fieldName => $fieldVal) { - $fields["\"$fieldName\""] = $this->parseValue($fieldVal); + $fields["\"{$fieldName}\""] = $this->parseValue($fieldVal); } - $insert = new SQLInsert($table, $fields); + $insert = new SQLInsert("\"{$table}\"", $fields); $insert->execute(); $id = DB::get_generated_id($table); $this->fixtures[$table][$identifier] = $id; diff --git a/docs/en/changelogs/3.2.0.md b/docs/en/changelogs/3.2.0.md index ed2d53839..d45a2a154 100644 --- a/docs/en/changelogs/3.2.0.md +++ b/docs/en/changelogs/3.2.0.md @@ -9,6 +9,8 @@ * UploadField "Select from files" shows files in all folders by default * UploadField won't display an overwrite warning unless Upload:replaceFile is true * HtmlEditorField no longer substitutes `
` for indented text + * ClassInfo::dataClassesFor now returns classes which should have tables, regardless of whether those + tables actually exist. ### CMS diff --git a/model/ManyManyList.php b/model/ManyManyList.php index a50c60d32..a06af47f8 100644 --- a/model/ManyManyList.php +++ b/model/ManyManyList.php @@ -59,10 +59,16 @@ class ManyManyList extends RelationList { $this->foreignKey = $foreignKey; $this->extraFields = $extraFields; - $baseClass = ClassInfo::baseDataClass($dataClass); + $this->linkJoinTable(); + } + /** + * Setup the join between this dataobject and the necessary mapping table + */ + protected function linkJoinTable() { // Join to the many-many join table - $this->dataQuery->innerJoin($joinTable, "\"$joinTable\".\"$this->localKey\" = \"$baseClass\".\"ID\""); + $baseClass = ClassInfo::baseDataClass($this->dataClass); + $this->dataQuery->innerJoin($this->joinTable, "\"{$this->joinTable}\".\"{$this->localKey}\" = \"{$baseClass}\".\"ID\""); // Add the extra fields to the query if($this->extraFields) { @@ -86,12 +92,12 @@ class ManyManyList extends RelationList { $this->_compositeExtraFields[$field] = array(); // append the composite field names to the select - foreach($obj->compositeDatabaseFields() as $k => $f) { - $col = $field . $k; + foreach($obj->compositeDatabaseFields() as $subField => $subSpec) { + $col = $field . $subField; $finalized[] = $col; // cache - $this->_compositeExtraFields[$field][] = $k; + $this->_compositeExtraFields[$field][] = $subField; } } else { $finalized[] = $field; @@ -113,16 +119,15 @@ class ManyManyList extends RelationList { if($this->_compositeExtraFields) { foreach($this->_compositeExtraFields as $fieldName => $composed) { - // convert joined extra fields into their composite field - // types. + // convert joined extra fields into their composite field types. $value = array(); - foreach($composed as $i => $k) { - if(isset($row[$fieldName . $k])) { - $value[$k] = $row[$fieldName . $k]; + foreach($composed as $subField => $subSpec) { + if(isset($row[$fieldName . $subSpec])) { + $value[$subSpec] = $row[$fieldName . $subSpec]; // don't duplicate data in the record - unset($row[$fieldName . $k]); + unset($row[$fieldName . $subSpec]); } } @@ -156,7 +161,7 @@ class ManyManyList extends RelationList { } // Apply relation filter - $key = "\"$this->joinTable\".\"$this->foreignKey\""; + $key = "\"{$this->joinTable}\".\"{$this->foreignKey}\""; if(is_array($id)) { return array("$key IN (".DB::placeholders($id).")" => $id); } else if($id !== null){ @@ -184,7 +189,8 @@ class ManyManyList extends RelationList { * Does so by adding an entry to the joinTable. * * @param mixed $item - * @param $extraFields A map of additional columns to insert into the joinTable + * @param array $extraFields A map of additional columns to insert into the joinTable. + * Column names should be ANSI quoted. */ public function add($item, $extraFields = array()) { // Ensure nulls or empty strings are correctly treated as empty arrays @@ -208,43 +214,61 @@ class ManyManyList extends RelationList { // Apply this item to each given foreign ID record if(!is_array($foreignIDs)) $foreignIDs = array($foreignIDs); - $baseClass = ClassInfo::baseDataClass($this->dataClass); foreach($foreignIDs as $foreignID) { - // Check for existing records for this item - if($foreignFilter = $this->foreignIDFilter($foreignID)) { + if($foreignFilter = $this->foreignIDWriteFilter($foreignID)) { // With the current query, simply add the foreign and local conditions // The query can be a bit odd, especially if custom relation classes // don't join expected tables (@see Member_GroupSet for example). - $query = $this->dataQuery->query()->toSelect(); + $query = new SQLSelect("*", "\"{$this->joinTable}\""); $query->addWhere($foreignFilter); $query->addWhere(array( - "\"$baseClass\".\"ID\"" => $itemID + "\"{$this->joinTable}\".\"{$this->localKey}\"" => $itemID )); $hasExisting = ($query->count() > 0); } else { $hasExisting = false; } - // Determine entry type - if(!$hasExisting) { - // Field values for new record - $fieldValues = array_merge($extraFields, array( - "\"{$this->foreignKey}\"" => $foreignID, - "\"{$this->localKey}\"" => $itemID - )); - // Create new record - $insert = new SQLInsert("\"{$this->joinTable}\"", $fieldValues); - $insert->execute(); - } elseif(!empty($extraFields)) { - // For existing records, simply update any extra data supplied - $foreignWriteFilter = $this->foreignIDWriteFilter($foreignID); - $update = new SQLUpdate("\"{$this->joinTable}\"", $extraFields, $foreignWriteFilter); - $update->addWhere(array( + $manipulation = array(); + if($hasExisting) { + $manipulation[$this->joinTable]['command'] = 'update'; + $manipulation[$this->joinTable]['where'] = array( + "\"{$this->joinTable}\".\"{$this->foreignKey}\"" => $foreignID, "\"{$this->joinTable}\".\"{$this->localKey}\"" => $itemID - )); - $update->execute(); + ); + } else { + $manipulation[$this->joinTable]['command'] = 'insert'; } + + if($extraFields) { + foreach($extraFields as $fieldName => $fieldValue) { + if(is_null($fieldValue)) { + $manipulation[$this->joinTable]['fields'][$fieldName] = null; + } elseif($fieldValue instanceof DBField) { + // rely on writeToManipulation to manage the changes + // required for this field. + $working = array('fields' => array()); + + // create a new instance of the field so we can + // modify the field name to the correct version. + $field = DBField::create_field(get_class($fieldValue), $fieldValue); + $field->setName($fieldName); + $field->writeToManipulation($working); + + foreach($working['fields'] as $extraName => $extraValue) { + $manipulation[$this->joinTable]['fields'][$extraName] = $extraValue; + } + } else { + $manipulation[$this->joinTable]['fields'][$fieldName] = $fieldValue; + } + } + } + + $manipulation[$this->joinTable]['fields'][$this->localKey] = $itemID; + $manipulation[$this->joinTable]['fields'][$this->foreignKey] = $foreignID; + + DB::manipulate($manipulation); } } @@ -275,7 +299,7 @@ class ManyManyList extends RelationList { public function removeByID($itemID) { if(!is_numeric($itemID)) throw new InvalidArgumentException("ManyManyList::removeById() expecting an ID"); - $query = new SQLDelete("\"$this->joinTable\""); + $query = new SQLDelete("\"{$this->joinTable}\""); if($filter = $this->foreignIDWriteFilter($this->getForeignID())) { $query->setWhere($filter); @@ -283,7 +307,7 @@ class ManyManyList extends RelationList { user_error("Can't call ManyManyList::remove() until a foreign ID is set", E_USER_WARNING); } - $query->addWhere(array("\"$this->localKey\"" => $itemID)); + $query->addWhere(array("\"{$this->localKey}\"" => $itemID)); $query->execute(); } @@ -302,7 +326,7 @@ class ManyManyList extends RelationList { $query->removeFilterOn($foreignFilter); $selectQuery = $query->query(); - $selectQuery->setSelect("\"$base\".\"ID\""); + $selectQuery->setSelect("\"{$base}\".\"ID\""); $from = $selectQuery->getFrom(); unset($from[$this->joinTable]); @@ -313,11 +337,11 @@ class ManyManyList extends RelationList { // Use a sub-query as SQLite does not support setting delete targets in // joined queries. $delete = new SQLDelete(); - $delete->setFrom("\"$this->joinTable\""); + $delete->setFrom("\"{$this->joinTable}\""); $delete->addWhere($this->foreignIDFilter()); $subSelect = $selectQuery->sql($parameters); $delete->addWhere(array( - "\"$this->joinTable\".\"$this->localKey\" IN ($subSelect)" => $parameters + "\"{$this->joinTable}\".\"{$this->localKey}\" IN ($subSelect)" => $parameters )); $delete->execute(); } @@ -341,13 +365,13 @@ class ManyManyList extends RelationList { // @todo Optimize into a single query instead of one per extra field if($this->extraFields) { foreach($this->extraFields as $fieldName => $dbFieldSpec) { - $query = new SQLSelect("\"$fieldName\"", "\"$this->joinTable\""); + $query = new SQLSelect("\"{$fieldName}\"", "\"{$this->joinTable}\""); if($filter = $this->foreignIDWriteFilter($this->getForeignID())) { $query->setWhere($filter); } else { user_error("Can't call ManyManyList::getExtraData() until a foreign ID is set", E_USER_WARNING); } - $query->addWhere("\"$this->localKey\" = {$itemID}"); + $query->addWhere("\"{$this->localKey}\" = {$itemID}"); $result[$fieldName] = $query->execute()->value(); } } diff --git a/model/connect/DBSchemaManager.php b/model/connect/DBSchemaManager.php index 760b43ec9..1836f84a1 100644 --- a/model/connect/DBSchemaManager.php +++ b/model/connect/DBSchemaManager.php @@ -130,16 +130,17 @@ abstract class DBSchemaManager { // End schema update foreach ($this->schemaUpdateTransaction as $tableName => $changes) { + $advancedOptions = isset($changes['advancedOptions']) ? $changes['advancedOptions'] : null; switch ($changes['command']) { case 'create': $this->createTable($tableName, $changes['newFields'], $changes['newIndexes'], - $changes['options'], @$changes['advancedOptions']); + $changes['options'], $advancedOptions); break; case 'alter': $this->alterTable($tableName, $changes['newFields'], $changes['newIndexes'], $changes['alteredFields'], $changes['alteredIndexes'], - $changes['alteredOptions'], @$changes['advancedOptions']); + $changes['alteredOptions'], $advancedOptions); break; } } diff --git a/model/queries/SQLConditionalExpression.php b/model/queries/SQLConditionalExpression.php index 0a68e664f..521a6c599 100644 --- a/model/queries/SQLConditionalExpression.php +++ b/model/queries/SQLConditionalExpression.php @@ -54,10 +54,10 @@ abstract class SQLConditionalExpression extends SQLExpression { /** * Sets the list of tables to query from or update * - * @example $query->setFrom("MyTable"); // SELECT * FROM MyTable + * @example $query->setFrom('"MyTable"'); // SELECT * FROM "MyTable" * - * @param string|array $from Escaped SQL statement, usually an unquoted table name - * @return SQLSelect + * @param string|array $from Single, or list of, ANSI quoted table names + * @return self */ public function setFrom($from) { $this->from = array(); @@ -75,9 +75,9 @@ abstract class SQLConditionalExpression extends SQLExpression { /** * Add a table to include in the query or update * - * @example $query->addFrom("MyTable"); // UPDATE MyTable + * @example $query->addFrom('"MyTable"'); // SELECT * FROM "MyTable" * - * @param string|array $from Escaped SQL statement, usually an unquoted table name + * @param string|array $from Single, or list of, ANSI quoted table names * @return self Self reference */ public function addFrom($from) { @@ -189,7 +189,7 @@ abstract class SQLConditionalExpression extends SQLExpression { /** * Add an additional filter (part of the ON clause) on a join. * - * @param string $table Table to join on from the original join + * @param string $table Table to join on from the original join (unquoted) * @param string $filter The "ON" SQL fragment (escaped) * @return self Self reference */ @@ -201,7 +201,7 @@ abstract class SQLConditionalExpression extends SQLExpression { /** * Set the filter (part of the ON clause) on a join. * - * @param string $table Table to join on from the original join + * @param string $table Table to join on from the original join (unquoted) * @param string $filter The "ON" SQL fragment (escaped) * @return self Self reference */ @@ -213,6 +213,7 @@ abstract class SQLConditionalExpression extends SQLExpression { /** * Returns true if we are already joining to the given table alias * + * @param string $tableAlias Table name * @return boolean */ public function isJoinedTo($tableAlias) { diff --git a/model/queries/SQLDelete.php b/model/queries/SQLDelete.php index 212365977..66c1499cf 100644 --- a/model/queries/SQLDelete.php +++ b/model/queries/SQLDelete.php @@ -23,6 +23,7 @@ class SQLDelete extends SQLConditionalExpression { * Construct a new SQLDelete. * * @param array|string $from An array of Tables (FROM clauses). The first one should be just the table name. + * Each should be ANSI quoted. * @param array $where An array of WHERE clauses. * @param array|string $delete The table(s) to delete, if multiple tables are queried from * @return self Self reference @@ -35,6 +36,7 @@ class SQLDelete extends SQLConditionalExpression { * Construct a new SQLDelete. * * @param array|string $from An array of Tables (FROM clauses). The first one should be just the table name. + * Each should be ANSI quoted. * @param array $where An array of WHERE clauses. * @param array|string $delete The table(s) to delete, if multiple tables are queried from */ diff --git a/model/queries/SQLInsert.php b/model/queries/SQLInsert.php index 73cd1b340..02c85aa08 100644 --- a/model/queries/SQLInsert.php +++ b/model/queries/SQLInsert.php @@ -26,7 +26,7 @@ class SQLInsert extends SQLExpression implements SQLWriteExpression { /** * Construct a new SQLInsert object * - * @param string $into Table name to insert into + * @param string $into Table name to insert into (ANSI quoted) * @param array $assignments List of column assignments * @return static */ @@ -37,7 +37,7 @@ class SQLInsert extends SQLExpression implements SQLWriteExpression { /** * Construct a new SQLInsert object * - * @param string $into Table name to insert into + * @param string $into Table name to insert into (ANSI quoted) * @param array $assignments List of column assignments */ function __construct($into = null, $assignments = array()) { @@ -50,7 +50,7 @@ class SQLInsert extends SQLExpression implements SQLWriteExpression { /** * Sets the table name to insert into * - * @param string $into Single table name + * @param string $into Single table name (ANSI quoted) * @return self The self reference to this query */ public function setInto($into) { diff --git a/model/queries/SQLSelect.php b/model/queries/SQLSelect.php index e3f2799e8..2a9272642 100644 --- a/model/queries/SQLSelect.php +++ b/model/queries/SQLSelect.php @@ -63,6 +63,7 @@ class SQLSelect extends SQLConditionalExpression { * * @param array $select An array of SELECT fields. * @param array|string $from An array of FROM clauses. The first one should be just the table name. + * Each should be ANSI quoted. * @param array $where An array of WHERE clauses. * @param array $orderby An array ORDER BY clause. * @param array $groupby An array of GROUP BY clauses. @@ -80,6 +81,7 @@ class SQLSelect extends SQLConditionalExpression { * * @param array $select An array of SELECT fields. * @param array|string $from An array of FROM clauses. The first one should be just the table name. + * Each should be ANSI quoted. * @param array $where An array of WHERE clauses. * @param array $orderby An array ORDER BY clause. * @param array $groupby An array of GROUP BY clauses. diff --git a/model/queries/SQLUpdate.php b/model/queries/SQLUpdate.php index e38fcbaec..ab62b8996 100644 --- a/model/queries/SQLUpdate.php +++ b/model/queries/SQLUpdate.php @@ -19,7 +19,7 @@ class SQLUpdate extends SQLConditionalExpression implements SQLWriteExpression { /** * Construct a new SQLUpdate object * - * @param string $table Table name to update + * @param string $table Table name to update (ANSI quoted) * @param array $assignment List of column assignments * @param array $where List of where clauses * @return static @@ -31,7 +31,7 @@ class SQLUpdate extends SQLConditionalExpression implements SQLWriteExpression { /** * Construct a new SQLUpdate object * - * @param string $table Table name to update + * @param string $table Table name to update (ANSI quoted) * @param array $assignment List of column assignments * @param array $where List of where clauses */ diff --git a/security/Member.php b/security/Member.php index 9de869f1f..731a65dfe 100644 --- a/security/Member.php +++ b/security/Member.php @@ -1497,15 +1497,11 @@ class Member extends DataObject implements TemplateGlobalProvider { */ class Member_GroupSet extends ManyManyList { - public function __construct($dataClass, $joinTable, $localKey, $foreignKey, $extraFields = array()) { - - // Bypass the many-many constructor - DataList::__construct($dataClass); - - $this->joinTable = $joinTable; - $this->localKey = $localKey; - $this->foreignKey = $foreignKey; - $this->extraFields = $extraFields; + protected function linkJoinTable() { + // Do not join the table directly + if($this->extraFields) { + user_error('Member_GroupSet does not support many_many_extraFields', E_USER_ERROR); + } } /** diff --git a/security/Security.php b/security/Security.php index daf152b59..72e4fed59 100644 --- a/security/Security.php +++ b/security/Security.php @@ -937,6 +937,9 @@ class Security extends Controller implements TemplateGlobalProvider { $requiredTables[] = 'Permission'; foreach($requiredTables as $table) { + // Skip test classes, as not all test classes are scaffolded at once + if(is_subclass_of($table, 'TestOnly')) continue; + // if any of the tables aren't created in the database if(!ClassInfo::hasTable($table)) return false; diff --git a/tests/model/ManyManyListTest.php b/tests/model/ManyManyListTest.php index 40622e05b..d19bba9be 100644 --- a/tests/model/ManyManyListTest.php +++ b/tests/model/ManyManyListTest.php @@ -253,8 +253,8 @@ class ManyManyListTest extends SapphireTest { $db = DB::getConn(); $expected = 'SELECT DISTINCT "ManyManyListTest_ExtraFields_Clients"."WorthCurrency",' .' "ManyManyListTest_ExtraFields_Clients"."WorthAmount", "ManyManyListTest_ExtraFields_Clients"."Reference",' - .' "ManyManyListTest_ExtraFields"."ClassName", "ManyManyListTest_ExtraFields"."Created",' - .' "ManyManyListTest_ExtraFields"."LastEdited", "ManyManyListTest_ExtraFields"."ID",' + .' "ManyManyListTest_ExtraFields"."ClassName", "ManyManyListTest_ExtraFields"."LastEdited",' + .' "ManyManyListTest_ExtraFields"."Created", "ManyManyListTest_ExtraFields"."ID",' .' CASE WHEN "ManyManyListTest_ExtraFields"."ClassName" IS NOT NULL THEN' .' "ManyManyListTest_ExtraFields"."ClassName" ELSE '. $db->prepStringForDB('ManyManyListTest_ExtraFields') .' END AS "RecordClassName" FROM "ManyManyListTest_ExtraFields" INNER JOIN' @@ -262,7 +262,7 @@ class ManyManyListTest extends SapphireTest { .' "ManyManyListTest_ExtraFields_Clients"."ManyManyListTest_ExtraFieldsID" =' .' "ManyManyListTest_ExtraFields"."ID"'; - $this->assertEquals($expected, $list->sql()); + $this->assertSQLEquals($expected, $list->sql($parameters)); }