Throw exception NotSupported Exception for `UuidGenerator` with doctr… #8898

scyzoryck · 2021-08-08T11:06:30Z

Let's fix another issue:

�[0;31mERROR�[0m: UndefinedMethod - lib/Doctrine/ORM/Id/UuidGenerator.php:33:59 - Method Doctrine\DBAL\Platforms\AbstractPlatform::getGuidExpression does not exist (see https://psalm.dev/022)
        $sql  = 'SELECT ' . $conn->getDatabasePlatform()->�[97;41mgetGuidExpression�[0m();

Method has been removed with doctrine/dbal#3211.
Lets throw exception if method do not exists. Generator itself is already deprecated.

Questions:

May it would be better to throw it in constructor?
What about unit tests? Should we cover also this case?

It looks like there will be few similar issues, so let's start from this one to create a template for the next PRs :)

lib/Doctrine/ORM/Id/UuidGenerator.php

greg0ire · 2021-08-08T16:05:15Z

May it would be better to throw it in constructor?

Throwing as early as possible is indeed a good idea 👍

scyzoryck · 2021-08-10T18:09:45Z

Thanks @greg0ire! It was completly wrong. I've added Unit Tests to cover its logic.
In PHPUnit I've defined 2 groups - dbal3 and dbal2 to differentiate tests that could be run with doctrine/dbal 2 and 3. I've adjusted also CI configuration.

psalm.xml

beberlei

Small adjustment to the exception message, otherwise good to go for me.

lib/Doctrine/ORM/Exception/NotSupported.php

phpstan.neon

phpstan-dbal3.neon

SenseException · 2021-08-16T21:56:33Z

.github/workflows/continuous-integration.yml

@@ -40,14 +40,14 @@ jobs:

      - name: "Run PHPUnit"
        continue-on-error: true
-        run: "vendor/bin/phpunit -c ci/github/phpunit/sqlite.xml --coverage-clover=coverage-no-cache.xml"
+        run: "vendor/bin/phpunit -c ci/github/phpunit/sqlite.xml --coverage-clover=coverage-no-cache.xml --exclude-group performance,locking_functional,dbal2"


@greg0ire Are performance and locking_functional not needed for the regular "Run PHPUnit" step?

performance is run separately, that's for sure. As for locking_functional, it's about Gearman, so it might be great to run it someday. They are already excluded in sqlite.xml, but I think @scyzroyck had to respecify them in order to add dbal2 to the list.

We'll see how this will affects the code coverage in case some test won't be executed anymore.

looks like it does

greg0ire · 2021-08-20T20:46:53Z

lib/Doctrine/ORM/Id/UuidGenerator.php

    }

    /**
     * {@inheritDoc}
+     *
+     * @throws NotSupported
     */
    public function generate(EntityManager $em, $entity)
    {


Apparently this method is no longer covered: https://app.codecov.io/gh/doctrine/orm/compare/8898/changes
Why is that?

If the coverage is taken from tests with doctrine/dbal:3.0, this function will not be tested as the class itself will be never initialised.

@greg0ire - it looks like the possible reason might be the same name --coverage-clover=coverage-cache.xml used for the tests with dbal2 and dbal3.

It's close to that, but that's not it, I think you can have the same name as long as it differs when it's uploaded (so the upload step needs to be modified)

Or maybe I'm mistaken, looks like I already took care of setting different names when introducing the DBAL 3 build

The list can be seen here: https://github.com/doctrine/orm/pull/8898/checks?check_run_id=3343305825#step:3:21

I think some might be missing indeed.

Ah but the DBAL3 tests don't even run right now, so there will not be a report from them. The DBAL2 tests are running though, and are supposed to cover that path.

The job supposed to cover that path are the mysql ones, and they are present… not sure what to think 🤔

Independent from this PR: Shouldn't this also be covered by the other platforms?

🤔 good question, might be worth removing that call to self::markTestSkipped and see on which other platforms it passes. The situation might have evolved since this test was written in 2012.

greg0ire · 2021-08-21T10:28:19Z

tests/Doctrine/Tests/ORM/Functional/UUIDGeneratorTest.php

@@ -20,18 +23,28 @@ class UUIDGeneratorTest extends OrmFunctionalTestCase
 {
    use VerifyDeprecations;

+    /**
+     * @group dbal2


Why do we need these groups anyway? Can't the same be achieved with markTestSkipped?

greg0ire · 2021-08-21T10:29:36Z

The mysql jobs are passing despite me pushing a very wrong commit, so it's not a coverage upload issue, it's an execution issue. Marking as draft until this is fixed by you or me :)

greg0ire · 2021-08-21T13:28:09Z

tests/Doctrine/Tests/ORM/Functional/UUIDGeneratorTest.php

    public function testGenerateUUID(): void
    {
        if ($this->_em->getConnection()->getDatabasePlatform()->getName() !== 'mysql') {
            self::markTestSkipped('Currently restricted to MySQL platform.');
        }

+        if (! method_exists(Connection::class, 'getGuidExpression')) {


I don't think Connection is the right class for this, I think you meant AbstractPlatform. That's probably why the code was not executed.

Yup, that was it!

…ine/dbal:3.x. Generating `getGuidExpression` has been removed in doctrine/dbal:3.x. Partially fixes doctrine#8884

greg0ire · 2021-08-21T13:41:31Z

Note: there is still a drop in coverage, because the DBAL 3 tests are not actually running yet (see #8886)

greg0ire · 2021-08-22T14:05:44Z

Thanks @scyzoryck !

greg0ire reviewed Aug 8, 2021

View reviewed changes

lib/Doctrine/ORM/Id/UuidGenerator.php Outdated Show resolved Hide resolved

scyzoryck force-pushed the dbal-3-suport-for-uuid-generator branch 2 times, most recently from 5c4d182 to 29ee927 Compare August 10, 2021 18:07

greg0ire reviewed Aug 10, 2021

View reviewed changes

psalm.xml Show resolved Hide resolved

beberlei previously approved these changes Aug 15, 2021

View reviewed changes

lib/Doctrine/ORM/Exception/NotSupported.php Outdated Show resolved Hide resolved

scyzoryck dismissed beberlei’s stale review via 6920251 August 15, 2021 11:54

scyzoryck force-pushed the dbal-3-suport-for-uuid-generator branch 2 times, most recently from 6920251 to 2b2f99a Compare August 15, 2021 11:57

scyzoryck requested review from greg0ire and beberlei August 15, 2021 11:58

greg0ire reviewed Aug 15, 2021

View reviewed changes

phpstan.neon Outdated Show resolved Hide resolved

scyzoryck force-pushed the dbal-3-suport-for-uuid-generator branch from 2b2f99a to 90d6835 Compare August 15, 2021 12:24

greg0ire reviewed Aug 15, 2021

View reviewed changes

phpstan-dbal3.neon Outdated Show resolved Hide resolved

scyzoryck force-pushed the dbal-3-suport-for-uuid-generator branch from 90d6835 to d4363ea Compare August 15, 2021 14:25

greg0ire previously approved these changes Aug 15, 2021

View reviewed changes

scyzoryck dismissed greg0ire’s stale review via f717029 August 16, 2021 18:36

scyzoryck force-pushed the dbal-3-suport-for-uuid-generator branch from d4363ea to f717029 Compare August 16, 2021 18:36

greg0ire previously approved these changes Aug 16, 2021

View reviewed changes

greg0ire added this to the 2.10.0 milestone Aug 16, 2021

greg0ire requested a review from SenseException August 16, 2021 19:51

SenseException reviewed Aug 16, 2021

View reviewed changes

scyzoryck requested a review from SenseException August 20, 2021 18:43

SenseException previously approved these changes Aug 20, 2021

View reviewed changes

greg0ire reviewed Aug 20, 2021

View reviewed changes

greg0ire dismissed stale reviews from SenseException and themself via 2b90aa3 August 21, 2021 10:21

greg0ire force-pushed the dbal-3-suport-for-uuid-generator branch from 2b90aa3 to 86b71d4 Compare August 21, 2021 10:22

greg0ire reviewed Aug 21, 2021

View reviewed changes

greg0ire marked this pull request as draft August 21, 2021 10:28

greg0ire reviewed Aug 21, 2021

View reviewed changes

greg0ire force-pushed the dbal-3-suport-for-uuid-generator branch from 86b71d4 to 426b426 Compare August 21, 2021 13:30

Throw exception NotSupported Exception for UuidGenerator with doctr…

cf4a4f2

…ine/dbal:3.x. Generating `getGuidExpression` has been removed in doctrine/dbal:3.x. Partially fixes doctrine#8884

greg0ire marked this pull request as ready for review August 21, 2021 13:40

greg0ire force-pushed the dbal-3-suport-for-uuid-generator branch from 426b426 to cf4a4f2 Compare August 21, 2021 13:40

greg0ire requested a review from SenseException August 21, 2021 13:41

SenseException approved these changes Aug 21, 2021

View reviewed changes

greg0ire merged commit 8bed630 into doctrine:2.10.x Aug 22, 2021

github-actions bot mentioned this pull request Oct 3, 2021

Merge release 2.10.0 into 3.0.x #9066

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw exception NotSupported Exception for `UuidGenerator` with doctr… #8898

Throw exception NotSupported Exception for `UuidGenerator` with doctr… #8898

scyzoryck commented Aug 8, 2021

greg0ire commented Aug 8, 2021

scyzoryck commented Aug 10, 2021

beberlei left a comment

SenseException Aug 16, 2021

greg0ire Aug 17, 2021

SenseException Aug 20, 2021

greg0ire Aug 20, 2021

greg0ire Aug 20, 2021

scyzoryck Aug 21, 2021

scyzoryck Aug 21, 2021

greg0ire Aug 21, 2021

greg0ire Aug 21, 2021

greg0ire Aug 21, 2021

greg0ire Aug 21, 2021

greg0ire Aug 21, 2021

SenseException Aug 21, 2021

greg0ire Aug 22, 2021

greg0ire Aug 21, 2021 •

edited

Loading

greg0ire commented Aug 21, 2021 •

edited

Loading

greg0ire Aug 21, 2021 •

edited

Loading

greg0ire Aug 21, 2021

greg0ire commented Aug 21, 2021

greg0ire commented Aug 22, 2021

Throw exception NotSupported Exception for UuidGenerator with doctr… #8898

Throw exception NotSupported Exception for UuidGenerator with doctr… #8898

Conversation

scyzoryck commented Aug 8, 2021

greg0ire commented Aug 8, 2021

scyzoryck commented Aug 10, 2021

beberlei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Aug 21, 2021 • edited Loading

Choose a reason for hiding this comment

greg0ire commented Aug 21, 2021 • edited Loading

greg0ire Aug 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Aug 21, 2021

greg0ire commented Aug 22, 2021

Throw exception NotSupported Exception for `UuidGenerator` with doctr… #8898

Throw exception NotSupported Exception for `UuidGenerator` with doctr… #8898

greg0ire Aug 21, 2021 •

edited

Loading

greg0ire commented Aug 21, 2021 •

edited

Loading

greg0ire Aug 21, 2021 •

edited

Loading