Platform from params are not correctly passed to a `QueryCacheProfile` #2821

kimhemsoe · 2017-08-22T18:16:49Z

If you pass in the platform from connection params it will be used to generate the cache key, which in most cases is not serializable.

dbal/lib/Doctrine/DBAL/Cache/QueryCacheProfile.php

Line 105 in db1395b

'&connectionParams=' . serialize($connectionParams);

I can create a better test case where it will fail from the serialize() call if needed. But i was lazy, copy pasted one there fitted my case :D

…Profile::generateCacheKeys()

kimhemsoe · 2017-08-22T18:29:44Z

Any suggestions for how i fix this?

One way could be to remove it from params in detectDatabasePlatform, but that only happens if getPlatform have been called.

We could also check for it in the the constructor, or remove it from the passed connection params to generateCacheKeys()

And would there be any BC concerns with removing it from params, after we have assign it to $this->platform?

…om there.

Ocramius · 2017-08-24T10:41:19Z

lib/Doctrine/DBAL/Connection.php

@@ -211,6 +211,15 @@ public function __construct(array $params, Driver $driver, Configuration $config
            unset($this->_params['pdo']);
        }

+        if (isset($params["platform"])) {
+            if ( ! $params['platform'] instanceof Platforms\AbstractPlatform) {
+                throw DBALException::invalidPlatformSpecified();


Should at least pass in the $params['platform'] and let the exception produce a more detailed message about what was given instead.

I've added more detailed exception message if an object is given. If another type is given it fails horrible and the bottom of your coffee mug will go out. Should i try to add support for any type? Or are we ok with that? :D

Ocramius · 2017-08-24T10:43:43Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+            ->with('cacheKey')
+            ->will($this->returnValue(['realKey' => []]));
+
+        $query  = 'SELECT * FROM foo WHERE bar = ?';


Use a simpler SELECT 1 query - remove all unnecessary params/types

Ocramius · 2017-08-24T10:44:12Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+        $connectionParams = $this->params;
+        $expectedParams = $connectionParams;
+
+        $connectionParams["platform"] = $this->createMock(AbstractPlatform::class);


Single quotes

Ocramius · 2017-08-24T10:44:23Z

lib/Doctrine/DBAL/Connection.php

@@ -211,6 +211,15 @@ public function __construct(array $params, Driver $driver, Configuration $config
            unset($this->_params['pdo']);
        }

+        if (isset($params["platform"])) {
+            if ( ! $params['platform'] instanceof Platforms\AbstractPlatform) {


This branch isn't tested

Ocramius · 2017-08-24T10:45:07Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+            ->will($this->returnValue($resultCacheDriverMock));
+
+        $connectionParams = $this->params;
+        $expectedParams = $connectionParams;


Remove this variable - just rely on $connectionParams below

Ocramius · 2017-08-24T10:45:37Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+
+        $this->assertInstanceOf(
+            ArrayStatement::class,
+            (new Connection($connectionParams, $driver))->executeCacheQuery($query, $params, $types, $queryCacheProfileMock)


Add a message to assertInstanceOf

Removing the assert as it is tested by "testConnectionParamsArePassedToTheQueryCacheProfileInExecuteCacheQuery"

Ocramius · 2017-08-24T10:45:45Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+        /* @var $driver Driver */
+        $driver = $this->createMock(Driver::class);
+
+        $this->assertInstanceOf(


self::assertInstanceOf()

Ocramius · 2017-08-24T10:46:08Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

@@ -777,6 +778,49 @@ public function testConnectionParamsArePassedToTheQueryCacheProfileInExecuteCach
        );
    }

+    public function testShouldNotPassPlatformInParamsToTheQueryCacheProfileInExecuteCacheQuery()


Add an @group annotation to the test

Mark the method as void

Fixed cs issues

…e detailed about what was given.

kimhemsoe · 2017-08-24T21:46:41Z

Can someone give me a hint why Scrutinizer is suddenly throwing up?

Ocramius · 2017-08-25T03:38:52Z

lib/Doctrine/DBAL/DBALException.php

@@ -36,13 +37,19 @@ public static function notSupported($method)
    }

    /**
-     * @return \Doctrine\DBAL\DBALException
+     * @param $invalidPlatform


Needs to be documented as BC break or moved to a new method

Ocramius · 2017-08-25T03:39:42Z

lib/Doctrine/DBAL/DBALException.php

-            "\Doctrine\DBAL\Platforms\AbstractPlatform.");
+            sprintf(
+            "Given 'platform' option '%s' must be a subtype of '%s'",
+            get_class($invalidPlatform),


Could be a non-object

Ocramius · 2017-08-25T03:40:27Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+
+        $connectionParams = $this->params;
+
+        // This is our main expectation


Comment can be dropped: if there were no expectations, the test would be marked as risky

Ocramius · 2017-08-25T03:41:18Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+    public function testThrowsExceptionWhenInValidPlatformSpecified(): void
+    {
+        self::expectException(DBALException::class);
+        self::expectExceptionMessage(


This needed to be a $this-> call (IIRac, it's an instance method)

Ocramius · 2017-08-25T03:41:26Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+     */
+    public function testThrowsExceptionWhenInValidPlatformSpecified(): void
+    {
+        self::expectException(DBALException::class);


This needed to be a $this-> call (IIRac, it's an instance method)

Move the exception expectations right before the method call that raises them

Ocramius · 2017-08-25T03:41:57Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

+    {
+        self::expectException(DBALException::class);
+        self::expectExceptionMessage(
+            "Given 'platform' option 'stdClass' must be a subtype of 'Doctrine\DBAL\Platforms\AbstractPlatform'"


Thos should be a test on the exception test itself

Ocramius · 2017-08-25T03:44:04Z

Scrutinizer fails on the mocks union types: ignore

…

On 24 Aug 2017 11:46 PM, "Kim Hemsø Rasmussen" ***@***.***> wrote: Can someone give me a hint why Scrutinizer is suddenly throwing up? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#2821 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakNJDWjAYfYQMswRTNhHW4SFmp8E2ks5sbe9CgaJpZM4O-_04> .

…d' to DBALDBALExceptionTest. Added upgrade notice about BC break. Cleanups

…w method to avoid API changes

…-query-cache' Close #2821

…w method to avoid API changes

…-query-cache-2.6' into 2.6 Backport #2821

Ocramius · 2017-08-28T10:53:58Z

@kimhemsoe merged and backported, thanks!

master: ce7560f
2.6: 367b731

…AL-* issues are from JIRA, not Github

Added failure test where platform from params is passed to QueryCache…

d1eca40

…Profile::generateCacheKeys()

Set platform instance if passed in connection params and remove it fr…

c545ef7

…om there.

kimhemsoe requested a review from Ocramius August 22, 2017 18:43

kimhemsoe added BC Fix Bug labels Aug 23, 2017

Ocramius suggested changes Aug 24, 2017

View reviewed changes

kimhemsoe added 2 commits August 24, 2017 23:01

Removed unnecessary assertions and method arguments.

776d2e8

Fixed cs issues

Changed DBALException::invalidPlatformSpecified() exception to be mor…

cc9dcc3

…e detailed about what was given.

Ocramius suggested changes Aug 25, 2017

View reviewed changes

Ocramius assigned kimhemsoe Aug 25, 2017

kimhemsoe added the BC Break label Aug 25, 2017

kimhemsoe added 2 commits August 27, 2017 15:04

Moved testing of 'Doctrine\DBALDBALException::invalidPlatformSpecifie…

da2356e

…d' to DBALDBALExceptionTest. Added upgrade notice about BC break. Cleanups

Removed a few linebreaks

2d57c68

Ocramius assigned Ocramius and unassigned kimhemsoe Aug 28, 2017

Ocramius removed the BC Break label Aug 28, 2017

Ocramius merged commit 2d57c68 into doctrine:master Aug 28, 2017

Ocramius added this to the 2.6.2 milestone Aug 28, 2017

Ocramius added a commit that referenced this pull request Aug 28, 2017

#2821 reverting BC break - moved static exception constructor to a ne…

1119a3d

…w method to avoid API changes

Ocramius added a commit that referenced this pull request Aug 28, 2017

#2821 reverting upgrade notes that are no longer valid

ad91a9f

Ocramius added a commit that referenced this pull request Aug 28, 2017

Merge branch 'fix/#2821-verify-that-platform-params-are-passed-to-the…

ce7560f

…-query-cache' Close #2821

Ocramius added a commit that referenced this pull request Aug 28, 2017

#2821 reverting BC break - moved static exception constructor to a ne…

a94e26a

…w method to avoid API changes

Ocramius added a commit that referenced this pull request Aug 28, 2017

#2821 reverting upgrade notes that are no longer valid

45c022d

Ocramius added a commit that referenced this pull request Aug 28, 2017

Merge branch 'fix/#2821-verify-that-platform-params-are-passed-to-the…

367b731

…-query-cache-2.6' into 2.6 Backport #2821

Ocramius changed the title ~~Added failure test where platform from params is passed to QueryCache…~~ Platform from params are not correctly passed to a QueryCacheProfile Aug 28, 2017

Ocramius added a commit that referenced this pull request Aug 28, 2017

#2821 replacing @group annotations with proper references, since DB…

835423b

…AL-* issues are from JIRA, not Github

github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Platform from params are not correctly passed to a `QueryCacheProfile` #2821

Platform from params are not correctly passed to a `QueryCacheProfile` #2821

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!


		$connectionParams = $this->params;

		// This is our main expectation

Uh oh!

Platform from params are not correctly passed to a QueryCacheProfile #2821

Platform from params are not correctly passed to a QueryCacheProfile #2821

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Platform from params are not correctly passed to a `QueryCacheProfile` #2821

Platform from params are not correctly passed to a `QueryCacheProfile` #2821