Module lint and clean up. Implemented fixes from testing on Azure #57
Conversation
| @@ -143,18 +161,34 @@ public function preparedQuery($sql, $parameters, $errorLevel = E_USER_ERROR) | |||
|
|
|||
| // Check for error | |||
| if (!$handle) { | |||
| $error = $this->getLastError(); | |||
|
|
|||
| if (preg_match("/Cannot insert explicit value for identity column in table '(.*)'/", $error, $matches)) { | |||
There was a problem hiding this comment.
This seems risky - if the database has SET_IDENTITY_INSERT off then we shouldn't be turning it on to force the query. The application should assume the that DB Server is correctly configured, rather than running these implicit queries.
There was a problem hiding this comment.
I believe that IDENTITY INSERT can only be overridden one table at a time, per connection and there isn't a global method configuration option for it?
There was a problem hiding this comment.
hmm - inserting with ID values is pretty central to SS - it's required for almost all our ORM inserts where there's MTI in play. Having logic that runs a query, checks the error, runs another query, re-runs the original and then runs a fourth final query is going to have a massive impact on performance.
Not only that, SET_IDENTITY_INSERT requires ALTER permissions on the table, so anyone with appropriately locked down permissions for their app won't even be able to run the query anyway.
However, it does seem to be somewhat impossible to have an autoincrement value in MSSQL without using the IDENTITY type. This just provides more reason why we shouldn't rely on the DB layer to produce PK values in SS.
In conclusion, it looks like we have pretty much no choice about this and we should probably accept that MSSQL is not a good DBMS to be using with SS. :(
| @@ -43,27 +47,10 @@ public function __destruct() | |||
| } | |||
| } | |||
|
|
|||
| public function getIterator() | |||
There was a problem hiding this comment.
Why has this generator been replaced with the old style API? #49
There was a problem hiding this comment.
@dhensby I take it getIterator is for SS5 not SS4 though? I was trying to get compatibility with SS 4.6
There was a problem hiding this comment.
Hmm - ok, then we need to do some branch management on this repo, it seems.
| * @param string $tableName | ||
| * @return string|null | ||
| */ | ||
| public function getIdentityColumn($tableName) |
There was a problem hiding this comment.
Why has this function been removed?
There was a problem hiding this comment.
As far as I could see it was no longer used anywhere?
Deployed to Azure MSSQL and confirmed dev/build