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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/workflows/static-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ jobs:
- name: "Run a static analysis with phpstan/phpstan"
continue-on-error: "${{ matrix.status == 'experimental' }}"
run: "vendor/bin/phpstan analyse"
if: "${{ matrix.dbal-version == 'default' }}"

- name: "Run a static analysis with phpstan/phpstan"
continue-on-error: "${{ matrix.status == 'experimental' }}"
run: "vendor/bin/phpstan analyse -c phpstan-dbal3.neon"
if: "${{ matrix.dbal-version != 'default' }}"

static-analysis-psalm:
name: "Static Analysis with Psalm"
Expand Down
5 changes: 5 additions & 0 deletions lib/Doctrine/ORM/Exception/NotSupported.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,9 @@ public static function create(): self
{
return new self('This behaviour is (currently) not supported by Doctrine 2');
}

public static function createForDbal3(): self
{
return new self('Feature was deprecated in doctrine/dbal 2.x and is not supported by installed doctrine/dbal:3.x, please see the doctrine/deprecations logs for new alternative approaches.');
}
}
10 changes: 10 additions & 0 deletions lib/Doctrine/ORM/Id/UuidGenerator.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,12 @@

namespace Doctrine\ORM\Id;

use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\Deprecations\Deprecation;
use Doctrine\ORM\EntityManager;
use Doctrine\ORM\Exception\NotSupported;

use function method_exists;

/**
* Represents an ID generator that uses the database UUID expression
Expand All @@ -22,10 +26,16 @@ public function __construct()
'%s is deprecated with no replacement, use an application-side generator instead',
self::class
);

if (! method_exists(AbstractPlatform::class, 'getGuidExpression')) {
throw NotSupported::createForDbal3();
}
}

/**
* {@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.

Expand Down
8 changes: 8 additions & 0 deletions phpstan-dbal3.neon
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
includes:
- phpstan-baseline.neon
- phpstan-params.neon

parameters:
ignoreErrors:
# deprecations from doctrine/dbal:3.x
- '/^Call to an undefined method Doctrine\\DBAL\\Platforms\\AbstractPlatform::getGuidExpression\(\).$/'
14 changes: 14 additions & 0 deletions phpstan-params.neon
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
parameters:
level: 5
paths:
- lib
- tests/Doctrine/StaticAnalysis
excludePaths:
- lib/Doctrine/ORM/Mapping/Driver/AttributeReader.php
earlyTerminatingMethodCalls:
Doctrine\ORM\Query\Parser:
- syntaxError
phpVersion: 70100
ignoreErrors:
# The class was added in PHP 8.1
- '/^Attribute class ReturnTypeWillChange does not exist.$/'
15 changes: 1 addition & 14 deletions phpstan.neon
Original file line number Diff line number Diff line change
@@ -1,21 +1,8 @@
includes:
- phpstan-baseline.neon
- phpstan-params.neon

parameters:
level: 5
paths:
- lib
- tests/Doctrine/StaticAnalysis
excludePaths:
- lib/Doctrine/ORM/Mapping/Driver/AttributeReader.php
earlyTerminatingMethodCalls:
Doctrine\ORM\Query\Parser:
- syntaxError
phpVersion: 70100

ignoreErrors:
# The class was added in PHP 8.1
- '/^Attribute class ReturnTypeWillChange does not exist.$/'

# https://github.com/doctrine/collections/pull/282
- '/Variable \$offset in isset\(\) always exists and is not nullable\./'
6 changes: 6 additions & 0 deletions psalm.xml
Original file line number Diff line number Diff line change
Expand Up @@ -47,5 +47,11 @@
<referencedClass name="Doctrine\Common\Cache\XcacheCache"/>
</errorLevel>
</UndefinedClass>
<UndefinedMethod>
<errorLevel type="suppress">
<!-- See https://github.com/doctrine/orm/issues/8884 -->
<referencedMethod name="Doctrine\DBAL\Platforms\AbstractPlatform::getGuidExpression"/>
</errorLevel>
</UndefinedMethod>
</issueHandlers>
</psalm>
22 changes: 22 additions & 0 deletions tests/Doctrine/Tests/ORM/Functional/UUIDGeneratorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,17 @@

namespace Doctrine\Tests\ORM\Functional;

use Doctrine\DBAL\Connection;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\Deprecations\PHPUnit\VerifyDeprecations;
use Doctrine\ORM\Exception\NotSupported;
use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Entity;
use Doctrine\ORM\Mapping\GeneratedValue;
use Doctrine\ORM\Mapping\Id;
use Doctrine\Tests\OrmFunctionalTestCase;

use function method_exists;
use function strlen;

/**
Expand All @@ -22,6 +26,10 @@ class UUIDGeneratorTest extends OrmFunctionalTestCase

public function testItIsDeprecated(): void
{
if (! method_exists(AbstractPlatform::class, 'getGuidExpression')) {
self::markTestSkipped('Test valid for doctrine/dbal:2.x only.');
}

$this->expectDeprecationWithIdentifier('https://github.com/doctrine/orm/issues/7312');
$this->_em->getClassMetadata(UUIDEntity::class);
}
Expand All @@ -32,6 +40,10 @@ public function testGenerateUUID(): void
self::markTestSkipped('Currently restricted to MySQL platform.');
}

if (! method_exists(AbstractPlatform::class, 'getGuidExpression')) {
self::markTestSkipped('Test valid for doctrine/dbal:2.x only.');
}

$this->_schemaTool->createSchema([
$this->_em->getClassMetadata(UUIDEntity::class),
]);
Expand All @@ -41,6 +53,16 @@ public function testGenerateUUID(): void
self::assertNotNull($entity->getId());
self::assertGreaterThan(0, strlen($entity->getId()));
}

public function testItCannotBeInitialised(): void
{
if (method_exists(AbstractPlatform::class, 'getGuidExpression')) {
self::markTestSkipped('Test valid for doctrine/dbal:3.x only.');
}

$this->expectException(NotSupported::class);
$this->_em->getClassMetadata(UUIDEntity::class);
}
}

/**
Expand Down