Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Normalize method signatures for fetch() and fetchAll(), ensuring compatibility with the PDOStatement signature #2527

Merged
merged 4 commits into from
Jun 6, 2017
Merged

Normalize method signatures for fetch() and fetchAll(), ensuring compatibility with the PDOStatement signature #2527

merged 4 commits into from
Jun 6, 2017

Conversation

phansys
Copy link
Contributor

@phansys phansys commented Oct 1, 2016 •
edited
Loading

Q A
Branch 2.5
Bug fix? yes
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets #2519
License MIT
Doc PR n/a

TODO

  • Check usage of func_get_args() at DB2Statement, SQLAnywhereStatement and SQLSrvStatement.

Close #2519.

@phansys phansys changed the base branch from master to 2.5 October 1, 2016 13:43
@Ocramius
Copy link
Member

Ocramius commented Oct 1, 2016

Please PR against master. Backporting is up to us.

On 1 Oct 2016 15:42, "Javier Spagnoletti" notifications@github.com wrote:

Q A
Branch 2.5
Bug fix? yes
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets #2519 #2519
License MIT
Doc PR n/a

Close #2519 #2519.

You can view, comment on, or merge this pull request online at:

#2527
Commit Summary

  • Changed the composer constraint to allow Common 2.5
  • add test for explicit positional parameter keys
  • Fixed expanding positional parameters which do not start from 0
  • Fixing default timezone for tests, as travis keeps failing due to
    php.ini in hhvm environment travis-ci/travis-ci#2523
  • fix column comments containing string literal chars on SQL Server
  • fix renaming column with default value on SQL Server
  • Add missing quotes at the end of literal strings
  • normalize asset names consistently
  • fix explicit case sensitive identifiers in Oracle
  • revert identity sequence naming strategy on Oracle to keep BC
  • use identity sequence name for generating SQL to drop an
    autoincrement from a column
  • fix NULL / NOT NULL clause for column alterations in Oracle
  • fix column comment lifecycle
  • fix table diff's new name if it is not set
  • false is not a valid type: replacing with boolean
  • Moving HHVM+mysql back to allowed failures: we need green builds to
    keep moving (for now)
  • fallback to default MySQL port if not given in mysqli connection
  • close statement cursor after test completion
  • add HHVM + mysqli driver to Travis build matrix
  • Using vendor PHPUnit version for travis builds
  • Reset date fields to UNIX epoch in TimeType.
  • Adds support for setting the charset in SQLSrv
  • fix explicitly quoted table identifiers in ALTER TABLE statements
  • Fixed closing a connection
  • Add test case for Fixed closing a connection #688
  • Fix test purpose
  • Use method setExpectedException instead annotation
  • Update MasterSlaveConnection.php #685 - as per @deeky666's review, exception should be expected only
    on the quoting call
  • doModifyLimitQuery() was missing an outer "ORDER BY doctrine_rownum"
  • fixed test cases
  • Enabled placeholders for "in" method in ExpressionBuilder
  • wrap PDOException for exec() method and cleanup unnecessary try
    catch blocks
  • fix quoted sequence name
  • add test to verify null cast in boolean type
  • resolves Add test to verify null cast in boolean type #632 Fix null cast in boolean type in Postgres platform
  • refs Add test to verify null cast in boolean type #632 Remove is_null to be consistent with doctrine coding
    standard
  • Update PostgreSqlPlatform.php
  • Test to check null conversion with boolean type
  • refs Add test to verify null cast in boolean type #632 check boolean conversion with and without PDO type value
  • Fix boolean conversion with php 5.6
  • Revert change to fix php 5.6
  • Change assertEquals to assertSame
  • Fix return type of Connection::project
  • Update PostgreSqlPlatform.php
  • fix table column alteration
  • Update PostgreSqlPlatform.php
  • Update PostgreSqlPlatform.php
  • Update PostgreSqlPlatform.php
  • Update PostgreSqlPlatform.php
  • only introspect namespaces and views accessible
  • allow connecting without database name for sqlanywhere driver
  • add test for connecting without database name on capable platforms
  • fix line separators
  • fix foreign key referential action SQL on SQL Server and SQL Anywhere
  • throw exception if exec() fails in mysqli driver connection
  • add more foreign key constraint violation error codes
  • fix fetching NULL values via fetchColumn()
  • fix CS
  • fix tear down foreign key constraint violation exception tests
  • fix foreign key referential action SQL on SQL Server and SQL Anywhere
  • fix generating COMMENT ON COLUMN statements for quoted identifiers
  • Merge branch 'master' into 2.5
  • sign release tags in ant build script
  • Prepare 2.5.0 release
  • Release 2.5.0
  • Bump version to 2.5.1
  • Update UPGRADE.md
  • Update index name quoting in MySQL table creation
  • Move index name quote to abstract
  • Only quote in get*SQL() methods
  • Use name rather than index object
  • fix quoting reserved keywords as unique constraint name in SQL
    Anywhere
  • throw exception for unsupported inline index declaration in DB2
  • apply reserved keyword as index name test case for all platforms
  • Removed non-phpdoc @internal tags
  • minor phpdoc fixes in the platform files
  • minor phpdoc fixes in the remaining files after pr minor phpdoc fixes in the platform files #746
  • Take care about MariaDB versioning
  • Check for changed fixed option added in PostgreSqlPlatform
  • unittest for fixed string alteration added
  • unittest group added to testcase for the fix
  • unittest adapted for all plattforms using AbstractPlatformTestCase
  • fix renaming indexes used by foreign key constraints
  • Merge branch 'hotfix/[DBAL-1062] Fix renaming indexes used by foreign key constraints #756-DBAL-1062-DBAL-1092-backport-to-2.5.x'
    into 2.5
  • fix creating and dropping database on PostgreSQL
  • Merge branch 'hotfix/backport-#757-DBAL-1093-to-2.5' into 2.5
  • fix index introspection on SQL Anywhere
  • Merge branch 'hotfix/[DBAL-1095] Fix index introspection on SQL Anywhere #759-DDC-1095-sql-anywhere-index-introspection-fk-fixes'
    into 2.5
  • Fix error handling in mysqli driver
  • char length fixed in PostgreSqlSchemaManager
  • add fixed string column introspection functional test case
  • fix foreign key constraint referential action NO ACTION and RESTRICT
    on Oracle
  • Merge branch 'hotfix/[DBAL-1097] Fix foreign key constraint referential action on Oracle #760-DBAL-1097-backport-to-2.5' into 2.5
  • Force connection close to allow further DROP DATABASE statements to
    work under PostgreSQL.
  • allow defining duplicate indexes based on columns and properties on
    a table
  • Merge branch 'hotfix/[DBAL-1063] Allow defining duplicate indexes on a table #764-DDC-1102-backport-to-2.5' into 2.5
  • Made the handling of NULL more consistent in the json_array type.
  • Added a unit test for the type JsonArray, to test empty string
    conversion.
  • Do not TRIM() parentheses around partial index's conditions
  • add group tag to test case
  • add more assertions for partial index test case
  • fix partial index test case
  • allow overriding implicit indexes
  • Merge branch 'hotfix/Allow overriding implicit indexes #769-allow-implicit-indexes-overrides' into 2.5
  • Fix unique index exception handling for an index on multiple columns
    in PHP 5.4
  • Merge branch 'hotfix/Fix unique index exception handling for an index on multiple columns in PHP 5.4 #771-backport-php-5-4-
    unique-index-exception-error-handling' into 2.5
  • Quote the name of an index in the create table statement.
  • Quote the name of an index in the create table statement. #723 - Added tests
  • fix database and namespace introspection for SQL Server
  • Merge branch 'hotfix/[DBAL-1058] Fix database and namespace introspection for SQL Server #736-DBAL-1058-fix-database-and-namespace-
    introspection-for-sql-server-backport-to-2.5' into 2.5
  • make host and server connection parameters optional for sqlanywhere
    driver
  • Merge branch 'hotfix/[DBAL-1121] Allow optional host and server connection parameters for sqlanywhere driver #777-allow-optional-host-and-server-connection-parameters-2.5'
    into 2.5
  • SQLite supports null limit via LIMIT -1
  • Hotfix: SQLite supports null limit via LIMIT -1
  • Merge branch 'hotfix/sqlite-offset-with-no-limit-support-2.5-backport'
    into 2.5
  • fix quoted identifiers for database creation SQL on SQL Anywhere
  • Merge branch 'hotfix/[DBAL-1115] Fix quoted identifiers for database creation SQL on SQL Anywhere #773-fix-quoted-identifiers-in-database-creation-for-sqlanywhere'
    into 2.5
  • Update QueryBuilder.php
  • Update QueryException.php
  • Update QueryBuilder.php
  • Update QueryBuilder.php
  • Update QueryBuilderTest.php
  • Update QueryBuilderTest.php
  • Update QueryBuilderTest.php
  • Update QueryBuilderTest.php
  • rename exception method
  • fix passing known aliases to QueryException
  • fix test case
  • simplify test case
  • use more declarative name for test case
  • fix removing autoincrement column from primary key in MySQL
  • Clean invalid primary option
  • Use getPrimaryKeyColumns api
  • Retain autoincrement
  • move pre alter table alter index SQL to private method
  • fix test group annotation
  • proper table naming
  • Fix broken functional test for SQL server
  • fix client_encoding setting to support pgbouncer
  • rehashed charset implementation to support old versions of postgresql
  • added additional comments explaining client_encoding support as
    requested
  • fixed typo in my comment
  • manual merge allow hhvm/mysqli failure so poor travis can feel better #831
  • Fix a bug where a failed transaction would cause bad profiling data,
    showing an open-ended query.
  • Add a unit-test to verify that when statements throw exceptions, the
    logger still has stopQuery called.
  • fix incorrect ordering of columns in clustered indexes on sql server
    when hydrated from schema info
  • Add test for correct index column ordering on clustered indexes
  • Change short array syntax to full array syntax
  • template1 as default database for Postgres
  • fix code style
  • add comment about using template1 database if dbname parameter is
    not set
  • add functional driver test case for connecting without dbname
    parameter
  • Merge pull request Fix call on non-object in ping() with PDO wrapper #878 from Virakal/master
  • Add test for MariaDB 5.5, 10.0 and 10.1 on Travis
  • use REGEXP operator for sqlite
  • fix dropping database with active connection on PostgreSQL
  • Override methods for sharding connection
  • fix retrieving the database name connected to for SQL Server
  • make driver property protected to be available for subclassing test
    cases
  • fix retrieving the database name connected to for SQL Anywhere
  • fix tear down of some test cases
  • allow failure for HHVM and MariaDB build
  • remove already excluded HHVM + pgsql builds from allowed failures
  • Merge pull request [2.5] Fix allowed failures for HHVM + MariaDB builds on Travis #906 from deeky666/fix-travis-allowed-failures
  • Fix current version
  • Release 2.5.2
  • Bump version to 2.5.3
  • Fix bug about get default params on PoolingShardConnection
  • Rebuild SQLServerPlatform::doModifyLimitQuery again to use a CTE and
    only 1 regex.
  • Change (?:DISTINCT|) to (?:DISTINCT)?
  • Add test for ORDER BY in TOP N subquery
  • Resolved @todo to allow ORDER BY in TOP N subquery with SQL Server
    2008 doModifyLimitQuery
  • Handle whitespace better
  • fix typo
  • Strict comparison for parens
  • Convert if/else to ternary
  • improve regex in isOrderByInTopNSubquery
  • Convert if to elseif
  • CS fixes
  • Loosen doctrine/common requirement #2260 - loosening doctrine/common requirement: allowing 2.6.x
  • Merge pull request #2260 - loosening doctrine/common requirement: allowing 2.6.x #2268 from Ocramius/hotfix/Loosen doctrine/common requirement #2260-loosen-
    doctrine-common-requirement
  • Release 2.5.3
  • Bumping version to 2.5.4-DEV
  • fix string column type declarations with whitespace on SQLite
  • fix usage of PDO::PGSQL_ATTR_DISABLE_PREPARES for pdo_pgsql
    extension setups not built from PHP core
  • Merge branch 'hotfix/Fix usage of PDO::PGSQL_ATTR_DISABLE_PREPARES for edge case pdo_pgsql setups #2273-undefined-pdo-constant-fix-backport-2-5'
    into 2.5
  • Release 2.5.4
  • Bumping version to 2.5.5-DEV
  • MySQL getListTableForeignKeysSQL: use current database if null passed
  • Remove SQL code duplication
  • Stop using template1 as default database for postgres drivers
  • When creating sqlite connections from a url with DriverManager, pass
    the 'path' key the driver expects instead of the 'dbname' key other drivers
    expect.
  • Update DriverManager unit tests to use 'path' instead of 'dbname'
    with sqlite driver.
  • use "memory" connection parameter instead of "path" for SQLite URLs
    if possible
  • Fixing the command when option is CURRENT_SCHEMA
  • add test for Oracle session init parameter value quotation
  • fix CS
  • Pass table object instead of table table name
  • fix schema diff expectation as we now pass table object instead of
    name
  • quote table and constraint names in drop foreign key / drop
    constraint SQL
  • Check for foreign table name on removed tables foreign key
  • cover an additional case of avoiding multi drop foreign key avoid in
    tests
  • fix CS
  • Fixed incorrect handling of single quotes in SQL-Strings escaped by
    repeated single-quote (DBAL-1205)
  • Error in Test fixed
  • preserve quotation of old column name in ColumnDiff
  • quote reserved keywords in TRUNCATE TABLE SQL
  • Change order of parameters in doModifyLimitQuery to work with
    Migrations
  • Fixes unit test DB2PlatformTest::testModifiesLimitQuery
  • keep references to bound parameter values in oci8 driver
  • Merge branch 'hotfix/Keep references to bound parameter values in oci8 driver #2434-OCI8 - bindValue overwrite previous values #2261-Issue #2261 - OCI8 Driver PHP 7 - Fix bindValue overwriting other params.  #2262-Fix oci driver bindValue overwrite with php7 #2386-keep-bound-oci8-parameters-from-gc-2.5'
    into 2.5
  • Fix Modify Limit Query with newline before ORDER BY
  • Merge branch 'fix/Modify Limit Query on SQLServer2012 with newline before ORDER BY #2428-sql-server-2012-newline-before-order-by-2.5'
    into 2.5
  • Track the Values & Types Passed to Statement::bindParam
  • Fix Up Some Tests to ::class & self::{once,equalTo}
  • Short Array Notation & Current PHPUnit Usage
  • Add a Newline Before return
  • make test compatible with 2.5 branch
  • escape identifiers in metadata SQL properly when used as string
    literal
  • Merge branch 'fix/Escape identifiers in metadata SQL properly when used as string literal #2442-DBAL-2436-escape-
    identifiers-in-metadata-sql-2.5' into 2.5
  • fix list foreign keys SQL database parameter evaluation
  • mark test as complete now
  • fix rebase mistake
  • Merge branch 'fix/Fix list foreign keys SQL database parameter evaluation #2286-foreign-keys-sql-database-parameter-evaluation'
    into 2.5
  • do not generate FROM clause in QueryBuilder if no tables specified
  • Merge branch 'fix/Do not generate FROM clause in QueryBuilder if no tables specified #2292-do-not-generate-sql-from-with-no-tables'
    into 2.5
  • Allow usage of symfony/console ^3.0
  • Merge pull request Allow usage of symfony/console ^3.0 in dev dependencies #2484 from stephandesouza/patch-1
  • added test to make sure that fetch returns false if nothing is found
  • changed return null to return false in mysqli driver if nothing was
    found
  • ResultStatement::fetch() returns null instead of false with mysqli #2497 Making sure that fetch returns false if nothing is found #2500 simplified test case scenario (inlined statement)
  • Merge branch 'fix/Making sure that fetch returns false if nothing is found #2500-ResultStatement::fetch() returns null instead of false with mysqli #2497-return-false-
    from-mysqli-statement-when-nothing-is-fetched-2.5' into 2.5
  • PHP 5.3 compatibility (broken test case array backets)
  • fix reverse engineering boolean type columns on DB2
  • Merge branch 'fix/[DBAL-939] Fix reverse engineering boolean type columns on DB2 #2277-DBAL-939: schema update doesnt detect boolean type correctly #2182-DBAL-939-fix-
    reverse-engineering-boolean-on-db2-2.5' into 2.5
  • trim leading slash from schemeless connection URL path with default
    driver connection parameters
  • fix parsing schemeless connection URLs with PHP < 5.4.8
  • fix mocking PDO
  • Fix parsing schemeless connection URLs #2287 minor docblock correction
  • Fix parsing schemeless connection URLs #2287 removing exception that was incorrectly cherry-picked into 2.5
  • Merge branch 'fix/Fix parsing schemeless connection URLs #2287-DBAL-1234: Additional slash in dbname when providing settings as URL without scheme #1183-parse-schemaless-connection-uris-2.5'
    into 2.5
  • fix another primary key alteration with autoincrement column case on
    MySQL
  • add test case for adding non autoincrement column to primary key
    with autoincrement column on MySQL
  • Merge branch 'fix/Fix another primary key alteration with autoincrement column case on MySQL #2303-when use \Doctrine\DBAL\Schema\Comparator to compare two table schema, has a problem #2302-primary-key-alteration-with-auto-increment-on-mysql-2.5'
    into 2.5
  • fix list table columns when using external or os authentication
  • add testReturnsGetListTableColumnsSQL test
  • [Oracle] Fix list table columns when using external or OS authentication with Oracle #2318 DBAL-831 adding note about possible fragile test comparing
    entire SQL query strings
  • Merge branch 'fix/[Oracle] Fix list table columns when using external or OS authentication with Oracle #2318-fix-listing-table-
    columns-when-using-external-or-os-authentication-on-oracledb-2.5' into
    2.5
  • Normalize signature for ResultStatement::fetchAll()
  • Normalize signature for ResultStatement::fetch()

File Changes

Patch Links:

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#2527, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAJakBIocVY-v_90BzRyeaiAV2bmVoqcks5qvmM2gaJpZM4KLwbY
.

@phansys phansys changed the base branch from 2.5 to master October 1, 2016 14:22
@phansys phansys changed the title Normalize method signatures for fetch() and fetchAll() [WIP] Normalize method signatures for fetch() and fetchAll() Oct 1, 2016
@mikeSimonson
Copy link
Contributor

@phansys What about the usage of func_get_args ?

@deeky666
Copy link
Member

deeky666 commented Feb 9, 2017

@phansys yes please do as @mikeSimonson suggested. We already follow this approach here

@phansys
Copy link
Contributor Author

phansys commented Feb 14, 2017

@mikeSimonson, @deeky666; the reason for the comment in my TODO is that I'm trying to normalize the method signatures, making them as explicitly as possible. Using func_get_args() to unpack method arguments is precisely doing the opposite.

@mikeSimonson
Copy link
Contributor

@phansys If it's possible to make the signature more explicit in those cases go for it.
As long as it doesn't break backward compatibility.
If it does put it in another PR.

@phansys phansys force-pushed the ticket_2519 branch 2 times, most recently from b778a43 to 93b3848 Compare February 19, 2017 00:06
@phansys phansys changed the title [WIP] Normalize method signatures for fetch() and fetchAll() Normalize method signatures for fetch() and fetchAll() Feb 19, 2017
@phansys
Copy link
Contributor Author

phansys commented Feb 19, 2017

The BC is respected internally, since ResultStatement::fetch() and ResultStatement::fetchAll() have default values for the new arguments. But this could lead to issues if somebody was using ResultStatement interface for their own classes.

@phansys
Copy link
Contributor Author

phansys commented Jun 6, 2017

Which is the status of this PR? Ping @Ocramius, @deeky666, @mikeSimonson.

Ocramius
Ocramius suggested changes Jun 6, 2017
View reviewed changes
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phansys excellent work, but we need to document the migration approach for existing implementations in UPGRADE.md

lib/Doctrine/DBAL/Driver/ResultStatement.php
*
* @return mixed The return value of this method on success depends on the fetch mode. In all cases, FALSE is
* returned on failure.
*
* @see PDO::FETCH_* constants.
*/
public function fetch($fetchMode = null);
public function fetch($fetchMode = null, $cursorOrientation = \PDO::FETCH_ORI_NEXT, $cursorOffset = 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This signature change is a BC break and needs upgrade/migration path in UPGRADE.md

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @Ocramius, I've added a note in the upgrade path for 2.6 regarding these changes.

lib/Doctrine/DBAL/Driver/ResultStatement.php
*/
public function fetchAll($fetchMode = null);
public function fetchAll($fetchMode = null, $fetchArgument = null, $ctorArgs = null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This signature change is a BC break and needs upgrade/migration path in UPGRADE.md

@Ocramius Ocramius added this to the 2.6 milestone Jun 6, 2017
@Ocramius Ocramius self-assigned this Jun 6, 2017
@Ocramius
Copy link
Member

Ocramius commented Jun 6, 2017

Thanks @phansys!

@Ocramius Ocramius merged commit 66fa7f8 into doctrine:master Jun 6, 2017
@phansys phansys deleted the ticket_2519 branch June 6, 2017 16:01
@Ocramius Ocramius changed the title Normalize method signatures for fetch() and fetchAll() Normalize method signatures for fetch() and fetchAll(), ensuring compatibility with the PDOStatement signature Jul 22, 2017
@Majkl578 Majkl578 mentioned this pull request Dec 18, 2017
Closed
morozov
morozov reviewed Jan 3, 2018
View reviewed changes
UPGRADE.md

After:

Doctrine\DBAL\Driver\ResultStatement::fetch($fetchMode, $fetchArgument, $ctorArgs);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably should've been Doctrine\DBAL\Driver\ResultStatement::fetchAll($fetchMode, $fetchArgument, $ctorArgs);.

@morozov
Copy link
Member

morozov commented Jan 3, 2018

After #2953 gets resolved, we may want to revert the API changes made here. Or maybe we could revert fetchAll() as part of #2953 (it already reverts the fetch() part)?

/cc @Ocramius

@Ocramius
Copy link
Member

Ocramius commented Jan 3, 2018

As part of #2953 would be best, since it already deals with moving away from PDO

@morozov
Copy link
Member

morozov commented Jan 3, 2018

Apparently, some drivers like IBM DB2, SQLServer and others implement handling additional arguments in fetch() and fetchAll() using func_get_args(), so removing all arguments except $fetchMode from the fetch*() methods is incorrect. I thought those arguments were introduced solely for compatibility with PDOStatement. Thanks to DataAccessTest::testFetchAllSupportFetchClass().

Instead, we can change their signatures to fetch*($fetchMode = null, ...$args) to make the optional arguments explicit and at the same time independent on the PDO constants.

@morozov morozov mentioned this pull request Jan 4, 2018
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@morozov morozov morozov left review comments

@Ocramius Ocramius Ocramius approved these changes

Assignees

@Ocramius Ocramius

Labels
BC Break Compatibility Drivers Improvement PDO
Projects
None yet
Milestone
2.6.0
Development

Successfully merging this pull request may close these issues.

ResultStatement#fetchAll() must define 3 arguments in order to be compatible with PDOStatement#fetchAll()
5 participants
@phansys @Ocramius @mikeSimonson @deeky666 @morozov