Skip to content
This repository was archived by the owner on Jan 29, 2020. It is now read-only.

TableIdentifier schema parameter to allow arrays for use in quoteIdentifierChain to express full object path.#232

Open
alextech wants to merge 4 commits into
zendframework:masterfrom
alextech:bugfix/sqlserver_tableidentifier
Open

TableIdentifier schema parameter to allow arrays for use in quoteIdentifierChain to express full object path.#232
alextech wants to merge 4 commits into
zendframework:masterfrom
alextech:bugfix/sqlserver_tableidentifier

Conversation

@alextech

@alextech alextech commented Mar 11, 2017

Copy link
Copy Markdown
Contributor

#206 Allow expression of full path towards table by allowing array as parameter for $schema in TableIdentifier and use quoteIdentifierChain in Sql table resolver. Reasoning explained in the issue.

While working on this noticed that quoteIdentifierChain tries to preform near identical operation as identifierChain, hence the change in the AbstractSQL class, but in less safer way. The result is the same most of the time but it uses hardcoded strings, instead of tokens specified in the specification that quoteIdentifier is using, such as quoteIdentiferTo or quoteIdentifer[0] and getIdentifierSeparator().

I think this is an oversight since databases with more complex database object locator rules (PostgreSQL and SQL Server) are only now becoming more popular. Cleaning that up possibly in another PR.

…wing array as parameter for $schema in TableIdentifier
Comment thread src/Sql/AbstractSql.php Outdated
*
* @link http://github.com/zendframework/zf2 for the canonical source repository
* @copyright Copyright (c) 2005-2016 Zend Technologies USA Inc. (http://www.zend.com)
* @copyright Copyright (c) 2005-2017 Zend Technologies USA Inc. (http://www.zend.com)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Comment thread src/Sql/AbstractSql.php Outdated

if ($schema && $table) {
$table = $platform->quoteIdentifier($schema) . $platform->getIdentifierSeparator() . $table;
$table = $platform->quoteIdentifierChain($schema).$platform->getIdentifierSeparator() . $table;

@froschdesign froschdesign Mar 11, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Spaces are missing before and after the dot.

Comment thread src/Sql/TableIdentifier.php Outdated
@@ -1,9 +1,11 @@
<?php

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Comment thread src/Sql/TableIdentifier.php Outdated
public function __construct($table, $schema = null)
{
if (! (is_string($table) || is_callable([$table, '__toString']))) {
if (!(is_string($table) || is_callable([$table, '__toString']))) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Space is missing after !.

Comment thread src/Sql/TableIdentifier.php Outdated
$this->schema = null;
} else {
if (! (is_string($schema) || is_callable([$schema, '__toString']))) {
if (!(is_string($schema) || is_array($schema) || is_callable([$schema, '__toString']))) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Space is missing after !.

Comment thread src/Sql/TableIdentifier.php Outdated
}

$this->schema = (string) $schema;
if (!is_array($schema)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Space is missing after !.

Comment thread test/Sql/TableIdentifierTest.php Outdated
@@ -1,19 +1,23 @@
<?php

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See above.

Comment thread test/Sql/TableIdentifierTest.php Outdated

/**
* Tests for {@see \Zend\Db\Sql\TableIdentifier}
* Tests for {@see \Zend\Db\Sql\TableIdentifier}.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Revert

Comment thread test/Sql/TableIdentifierTest.php Outdated

/**
* Data provider
* Data provider.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Revert

Comment thread test/Sql/TableIdentifierTest.php Outdated

/**
* Data provider
* Data provider.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Revert

@froschdesign

Copy link
Copy Markdown
Member

@alextech
I think you should deactivate your automated CS corrections.

}

$this->schema = (string) $schema;
if (! is_array($schema)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

probably check for callable [$schema, '__toString'] only to avoid re-cast $schema that already string ?

if (is_callable([$schema, '__toString'])) {

@alextech alextech mentioned this pull request Sep 26, 2017
@froschdesign froschdesign modified the milestones: 2.9.0, 2.10.0 Dec 7, 2017
@weierophinney

Copy link
Copy Markdown
Member

This repository has been moved to laminas/laminas-db. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-db to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-db.
  • In your clone of laminas/laminas-db, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

@michalbundyra

Copy link
Copy Markdown
Member

This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at laminas/laminas-db#78.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants