Skip to content

Adding context information when requesting invalid column type #7013

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: 4.4.x
Choose a base branch
from
Open

Adding context information when requesting invalid column type #7013

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4a10788
Adding context information when requesting invalid column type
mamazu Jul 1, 2025
9caaba3
Removing the bc break
mamazu Aug 17, 2025
92d7814
Adding tests for unknown colum type
mamazu Aug 17, 2025
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
8 changes: 7 additions & 1 deletion src/Schema/AbstractSchemaManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
use Doctrine\DBAL\Connection;
use Doctrine\DBAL\Exception;
use Doctrine\DBAL\Exception\DatabaseRequired;
use Doctrine\DBAL\Types\Exception\UnknownColumnType;
use Doctrine\DBAL\Platforms\AbstractPlatform;

Check failure on line 11 in src/Schema/AbstractSchemaManager.php

View workflow job for this annotation

GitHub Actions / Coding Standards / Coding Standards (PHP: 8.4) 

Use statements should be sorted alphabetically. The first wrong one is Doctrine\DBAL\Platforms\AbstractPlatform.
use Doctrine\DBAL\Platforms\Exception\NotSupported;
use Doctrine\DBAL\Result;
use Doctrine\DBAL\Schema\Exception\TableDoesNotExist;
Expand All @@ -23,6 +24,7 @@
use function count;
use function func_get_arg;
use function func_num_args;
use function sprintf;
use function strtolower;

/**
Expand Down Expand Up @@ -812,7 +814,11 @@
{
$list = [];
foreach ($rows as $row) {
$column = $this->_getPortableTableColumnDefinition($row);
try {
$column = $this->_getPortableTableColumnDefinition($row);
} catch (UnknownColumnType $unknownTypeException) {
throw UnknownColumnType::withContext($unknownTypeException->getType(), sprintf('table %s', $table));

Check warning on line 820 in src/Schema/AbstractSchemaManager.php

View check run for this annotation

Codecov / codecov/patch

src/Schema/AbstractSchemaManager.php#L819-L820 

Added lines #L819 - L820 were not covered by tests
Copy link
Member

@morozov morozov Jul 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is overcomplication. Instead, we could try reusing UnsupportedTableDefinition and adding another static constructor (e.g. fromIntrospectionException(). It would accept the table name and reference the exception caught from _getPortableTableColumnDefinition() as the previous exception.

This way, we will be able to supply any introspection exception with the context (table name) w/o introducing new classes.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds like a reasonable idea but what is this UnsupportedTableDefinition class? I can't find it in the source code or the dependencies.

Copy link
Member

@morozov morozov Jul 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for the confusion. This class is available only in 5.0.x (introduced in #6853).

Please feel free to re-introduce it in your PR (w/o the methods that exist in 5.0.x, only with yours).

Note that the 4.2.x branch accepts only bug fixes. An improvement like this needs to be targeted at 4.3.x.

Copy link
Author

Choose a reason for hiding this comment

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

Hm thinking about this, this change might be better suited for 5.0 anyway then seeing that using a different exception would also change the type of exception thrown in the method. However, I think it still would be cool to have it in 4.3.x without any bc breaks.

How do other parts of the application handle those kinds of things. Like adding context to an exception sounds like a common enough use-case.

}

$name = strtolower($column->getQuotedName($this->platform));
$list[$name] = $column;
Expand Down
35 changes: 28 additions & 7 deletions src/Types/Exception/UnknownColumnType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,45 @@
namespace Doctrine\DBAL\Types\Exception;

use Exception;
use Throwable;

use function sprintf;

final class UnknownColumnType extends Exception implements TypesException
{
public static function new(string $name): self
{
return new self(
sprintf(
'Unknown column type "%s" requested. Any Doctrine type that you use has '
private function __construct(
private string $requestedType,
?string $context,
int $code = 0,
?Throwable $throwable = null,
) {
Copy link
Member

Choose a reason for hiding this comment

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

Changing the constructor signature or the constructor's visibility is a breaking change because we did not declare the constructor as internal. The named constructor new() took care of rendering the message which was fine. Please keep it that way. If you need a different message, feel free to add a second named constructor.

Copy link
Author

Choose a reason for hiding this comment

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

That sounds like a good approach. I've refactored it to a new method. I am not sure if the ensuing code duplication is something that could be easily factored out or if it is okay.

Copy link
Author

Choose a reason for hiding this comment

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

And a question about the tests. I couldn't find any tests for the abstract schema manager. Could you give me a lead on where I would need to add this?

Copy link
Member

Choose a reason for hiding this comment

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

What about SchemaManagerFunctionalTestCase?

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I tried adding a test case but the problem is that it already fails on creation of the schema and it doesn't get to listing it. So when creating a column with an invalid type it throws an exception (but without context, as it doesn't know the table context yet).

I've added a test case in the column editor that it should throw an "Unknown column type" exception, is that enough or do I need to dig in how to create an invalid schema without the Table::editor()? Just to have it crash later when listing the schema.

Copy link
Member

Choose a reason for hiding this comment

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

I've added a test case in the column editor that it should throw an "Unknown column type" exception

That test would pass without your modifications already, wouldn't it? This basically means that your changes to the schema manager are still not covered.

Copy link
Author

Choose a reason for hiding this comment

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

Correct, this test already would have worked without my changes.

The problem I see is the test would fail when creating the column. Before I can assign it to any table to trigger the extended error message.

$message = sprintf(
'Unknown column type "%s" requested%s. Any Doctrine type that you use has '
. 'to be registered with \Doctrine\DBAL\Types\Type::addType(). You can get a list of all the '
. 'known types with \Doctrine\DBAL\Types\Type::getTypesMap(). If this error occurs during database '
. 'introspection then you might have forgotten to register all database types for a Doctrine Type. '
. 'Use AbstractPlatform#registerDoctrineTypeMapping() or have your custom types implement '
. 'Type#getMappedDatabaseTypes(). If the type name is empty you might '
. 'have a problem with the cache or forgot some mapping information.',
$name,
),
$this->requestedType,
$context !== null ? ' for ' . $context : '',
);

parent::__construct($message, $code, $throwable);
}

public function getType(): string
{
return $this->requestedType;
}

public static function new(string $name): self
{
return new self($name, null);
}

public static function withContext(string $name, string $context): self
{
return new self($name, $context);
}
}
33 changes: 33 additions & 0 deletions tests/Types/Exception/UnknownColumnTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?php

declare(strict_types=1);

namespace Doctrine\DBAL\Tests\Types\Exception;

use Doctrine\DBAL\Types\Exception\UnknownColumnType;
use PHPUnit\Framework\TestCase;

class UnknownColumnTypeTest extends TestCase
{
public function testNew(): void
{
$exception = UnknownColumnType::new('custom_type');

self::assertSame('custom_type', $exception->getType());
self::assertStringContainsString(
'Unknown column type "custom_type" requested.',
$exception->getMessage(),
);
}

public function testWithContext(): void
{
$exception = UnknownColumnType::withContext('custom_type', 'table "some_table"');

self::assertSame('custom_type', $exception->getType());
self::assertStringContainsString(
'Unknown column type "custom_type" requested for table "some_table".',
$exception->getMessage(),
);
}
}
Loading