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

PHP Copy-Paste Detector #5031

Merged
merged 2 commits into from
Aug 30, 2021
Merged

PHP Copy-Paste Detector #5031

merged 2 commits into from
Aug 30, 2021

Conversation

MGatner
Copy link
Member

@MGatner MGatner commented Aug 26, 2021

Description
This is a new tool I'm using at work from Sebastian Bergmann (PHPUnit author) that I thought would make a nice addition to the CS Fixer changes were are rolling out soon. The only remaining violation (ignoring tests/ for now) is in Database\\SQLSRV\\Builder::coompileSelect() but refactoring that is beyond the current scope of develop so I have ignored the file for now.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • n/a User guide updated
  • Conforms to style guide

@MGatner
Copy link
Member Author

MGatner commented Aug 26, 2021

Something is disturbingly wrong that this has effected non-SQLSRV tests. Anyone have guesses? @michalsn?

@kenjis
Copy link
Member

kenjis commented Aug 27, 2021

I don't know why, but the test (testJoinWithAlias) mocks SQLSRV.

$this->db = new MockConnection(['DBDriver' => 'SQLSRV', 'database' => 'test', 'schema' => 'dbo']);

@sfadschm
Copy link
Contributor

sfadschm commented Aug 27, 2021

When calling the parent::join method, this already adds the (wrong) join to the QBJoin array:

$this->QBJoin[] = $type . 'JOIN ' . $table . $cond;

Then in the rest of the changed method, the same (now correct) join (using getFullName) is appended to QBJoin as well:
$this->QBJoin[] = $type . 'JOIN ' . $this->getFullName($table) . $cond;

Also, currently, this part is executed twice (in the parent and in the child):

// Do we want to escape the table name?
if ($escape === true) {
$table = $this->db->protectIdentifiers($table, true, null, false);
}

I think if you want to do this properly, you will have to split the duplicate part out into a helper method of the BaseBuilder.

Edit: This would then need to return the $type, $table and $cond parameters to the respective join methods, which only parse them into the string.

@MGatner
Copy link
Member Author

MGatner commented Aug 27, 2021

Thanks all. As @kenjis pointed out, the code in join() is not an immutable block. Since this falls under the same category as compileSelect() I have restored it for now so this can remain on the develop branch.

@MGatner MGatner merged commit 3d6b3bd into codeigniter4:develop Aug 30, 2021
@MGatner MGatner deleted the phpcbd branch August 30, 2021 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants