Skip to content
This repository has been archived by the owner on Jan 24, 2024. It is now read-only.

Provide a default prefix to compile in sqlStatement #63

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sylbru
Copy link

@sylbru sylbru commented Mar 10, 2021

When creating an ActiveQuery without a prefix (which is possible since the prefix property is nullable), the sqlStatement method fails since it passes the prefix property to $this->driver->getQueryCompiler()->compile which expects a string. I suggest to fallback to an empty string when the prefix property is null.

…ctiveQuery

The prefix property in ActiveQuery is a nullable string, but the `compile` method in
CompilerInterface expects a string. We provide an empty string if the prefix property is null.
@sylbru
Copy link
Author

sylbru commented Mar 10, 2021

I wasn’t able to run the test suite, but it seems like the CI setup itself is broken, so I guess it’s alright. Plus it’s really a small and safe change I think.

@codecov
Copy link

codecov bot commented Mar 10, 2021

Codecov Report

Merging #63 (9f0a620) into master (5ae0a62) will decrease coverage by 0.34%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #63      +/-   ##
============================================
- Coverage     92.90%   92.55%   -0.35%     
- Complexity     1476     1488      +12     
============================================
  Files            67       67              
  Lines          3804     3842      +38     
============================================
+ Hits           3534     3556      +22     
- Misses          270      286      +16     
Impacted Files Coverage Δ
src/Query/ActiveQuery.php 91.66% <100.00%> (+0.23%) ⬆️
src/Driver/Driver.php 76.44% <0.00%> (-6.17%) ⬇️
src/Driver/Postgres/PostgresDriver.php 81.25% <0.00%> (-1.29%) ⬇️
src/Schema/AbstractColumn.php 94.41% <0.00%> (-0.09%) ⬇️
src/Driver/MySQL/MySQLCompiler.php 100.00% <0.00%> (ø)
src/Driver/Compiler.php 95.13% <0.00%> (+0.08%) ⬆️
src/DatabaseManager.php 98.48% <0.00%> (+0.12%) ⬆️
src/Query/SelectQuery.php 95.49% <0.00%> (+0.25%) ⬆️
src/Driver/SQLite/SQLiteCompiler.php 95.55% <0.00%> (+0.43%) ⬆️
src/Query/InsertQuery.php 100.00% <0.00%> (+5.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a729662...9f0a620. Read the comment docs.

@@ -105,7 +105,7 @@ public function sqlStatement(QueryParameters $parameters = null): string

return $this->driver->getQueryCompiler()->compile(
$parameters ?? new QueryParameters(),
$this->prefix,
$this->prefix ?? "",
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest using single quotes instead of double quotes. It’ll be more consistent

Copy link
Author

Choose a reason for hiding this comment

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

Done!

@sylbru
Copy link
Author

sylbru commented Jul 20, 2021

Should we re-run the checks for this PR? I think the CI setup was kind of broken at the time.

@wolfy-j wolfy-j removed the request for review from SerafimArts July 26, 2021 12:57
@wolfy-j
Copy link
Member

wolfy-j commented Jul 26, 2021

Sounds like we can't do it anymore...

@wolfy-j
Copy link
Member

wolfy-j commented Jul 26, 2021

@SerafimArts do you have any idea how to rerun it?

@SerafimArts
Copy link
Contributor

Maybe add some unimportant commit? Something like that:

$compiler = $this->driver->getQueryCompiler();

return $compiler->compile(
    $parameters ?? new QueryParameters(),
    $this->prefix,
    $this
);

@sylbru
Copy link
Author

sylbru commented Jul 30, 2021

@SerafimArts Done!

@SerafimArts
Copy link
Contributor

@wolfy-j it seems it worked =)

@wolfy-j
Copy link
Member

wolfy-j commented Jul 30, 2021

@roxblnfk we have to copy it to the cycle/database as well.

@sylbru
Copy link
Author

sylbru commented Jul 30, 2021

So the test suite fails under MySQL 5.7 but apparently the same test was already failing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants