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

Throw exception NotSupported Exception for UuidGenerator with doctr… #8898

Merged

Conversation

scyzoryck
Copy link
Contributor

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:

  1. May it would be better to throw it in constructor?
  2. 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 :)

@greg0ire
Copy link
Member

greg0ire commented Aug 8, 2021

May it would be better to throw it in constructor?

Throwing as early as possible is indeed a good idea 👍

@scyzoryck 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
@scyzoryck
Copy link
Contributor Author

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 Show resolved Hide resolved
beberlei
beberlei previously approved these changes Aug 15, 2021
Copy link
Member

@beberlei beberlei left a comment

Choose a reason for hiding this comment

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

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

lib/Doctrine/ORM/Exception/NotSupported.php Outdated Show resolved Hide resolved
@scyzoryck 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
phpstan.neon Outdated Show resolved Hide resolved
@scyzoryck scyzoryck force-pushed the dbal-3-suport-for-uuid-generator branch from 2b2f99a to 90d6835 Compare August 15, 2021 12:24
phpstan-dbal3.neon Outdated Show resolved Hide resolved
@scyzoryck scyzoryck force-pushed the dbal-3-suport-for-uuid-generator branch from 90d6835 to d4363ea Compare August 15, 2021 14:25
greg0ire
greg0ire previously approved these changes Aug 15, 2021
greg0ire
greg0ire previously approved these changes Aug 16, 2021
@greg0ire greg0ire added this to the 2.10.0 milestone Aug 16, 2021
@@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

SenseException
SenseException previously approved these changes Aug 20, 2021
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

🤔 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 greg0ire dismissed stale reviews from SenseException and themself via 2b90aa3 August 21, 2021 10:21
@greg0ire greg0ire force-pushed the dbal-3-suport-for-uuid-generator branch from 2b90aa3 to 86b71d4 Compare August 21, 2021 10:22
@@ -20,18 +23,28 @@ class UUIDGeneratorTest extends OrmFunctionalTestCase
{
use VerifyDeprecations;

/**
* @group dbal2
Copy link
Member

@greg0ire greg0ire Aug 21, 2021

Choose a reason for hiding this comment

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

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

@greg0ire greg0ire marked this pull request as draft August 21, 2021 10:28
@greg0ire
Copy link
Member

greg0ire commented Aug 21, 2021

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 :)

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

if (! method_exists(Connection::class, 'getGuidExpression')) {
Copy link
Member

@greg0ire greg0ire Aug 21, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, that was it!

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

Generating `getGuidExpression` has been removed in doctrine/dbal:3.x.

Partially fixes doctrine#8884
@greg0ire greg0ire marked this pull request as ready for review August 21, 2021 13:40
@greg0ire greg0ire force-pushed the dbal-3-suport-for-uuid-generator branch from 426b426 to cf4a4f2 Compare August 21, 2021 13:40
@greg0ire
Copy link
Member

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

@greg0ire greg0ire merged commit 8bed630 into doctrine:2.10.x Aug 22, 2021
@greg0ire
Copy link
Member

Thanks @scyzoryck !

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