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.

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

Fix for PHQL replaces field names in where #12971 #13124

Closed
wants to merge 2 commits into from

Conversation

felixDlh
Copy link

Hello!

This PR concern issue #12971. While using your framework in a project, we encoutered the same problem as described in the issue. This solution worked for us.
Thanks

@sergeyklay
Copy link
Member

@felixDlh Could you please cover your changes by small unit test?

@felixDlh
Copy link
Author

@sergeyklay Is it right for you now ?

@sergeyklay sergeyklay changed the base branch from 3.3.x to 3.2.x October 21, 2017 09:47
@sergeyklay sergeyklay changed the base branch from 3.2.x to 3.3.x October 21, 2017 09:47
sergeyklay added a commit that referenced this pull request Oct 21, 2017
@sergeyklay
Copy link
Member

Merged by hand to the 3.2.x branch. Thank you

@sergeyklay sergeyklay closed this Oct 21, 2017
sergeyklay added a commit that referenced this pull request Oct 21, 2017
@sergeyklay
Copy link
Member

@felixDlh Actually your patch is totally wrong

--- mvc/model/query.zep 2017-10-21 15:43:41.000000000 +0300
+++ mvc/model/query.zep 2017-10-21 15:43:58.000000000 +0300
         * Check if the qualified name is a column alias
         */
        let sqlColumnAliases = this->_sqlColumnAliases;
        if isset sqlColumnAliases[columnName] {
            var domain;

-		if isset sqlColumnAliases[columnName] && empty expr["domain"] {
  			return [
  				"type": "qualified",
  				"name": columnName
 			];
 		}

+		if isset sqlColumnAliases[columnName] && (!isset expr["domain"] || empty expr["domain"]) {
  			return [
  				"type": "qualified",
  				"name": columnName
 			];
 		}

        let metaData = this->_metaData;

as well as test

--- wrong test   2017-10-21 15:43:41.000000000 +0300
+++ correct test 2017-10-21 15:43:58.000000000 +0300
***************
*** 2,10 ****

  return [
      // PR #13124, ISSUE #12971
!     Missed space before WHERE and Robotina should be escaped 
-     "phql"     => 'SELECT UPPER('. Robots::class . '.name) AS name FROM ' . Robots::class . 'WHERE ' . Robots::class .'.name = Robotina',
      "expected" => [
-         'distinct' => 0,
          'models' => [
              Robots::class,
          ],
--- 2,9 ----

  return [
      // PR #13124, ISSUE #12971
+     "phql"     => 'SELECT UPPER('. Robots::class . '.name) AS name FROM ' . Robots::class . ' WHERE ' . Robots::class .'.name = "Robotina"',
      "expected" => [
          'models' => [
              Robots::class,
          ],
***************
*** 12,18 ****
              'robots'
          ],
          'columns' => [
!             Why??? 
-             'id' => [
                  'type' => 'scalar',
                  'balias' => 'name',
                  'sqlAlias' => 'name',
--- 11,17 ----
              'robots'
          ],
          'columns' => [
+             'name' => [
                  'type' => 'scalar',
                  'balias' => 'name',
                  'sqlAlias' => 'name',
***************
*** 41,47 ****
              ],
              'right' => [
                  'type' => 'literal',
-                 'value' => 'Robotina'
              ]
          ]
      ]
--- 40,46 ----
              ],
              'right' => [
                  'type' => 'literal',
+                 'value' => '\'Robotina\''
              ]
          ]
      ]

chilimatic pushed a commit to chilimatic/cphalcon that referenced this pull request Nov 2, 2017
chilimatic pushed a commit to chilimatic/cphalcon that referenced this pull request Nov 2, 2017
@niden niden added bug A bug report status: medium Medium and removed Bug - Medium labels Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug report status: medium Medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants