Adding context information when requesting invalid column type #7013
Conversation
mamazu
force-pushed
the
better_error_messages
branch
from
ed91f05 to
6c06126
Compare
src/Schema/AbstractSchemaManager.php
Outdated
| try { | ||
| $column = $this->_getPortableTableColumnDefinition($tableColumn); | ||
| } catch (UnknownColumnType $unknownTypeException) { | ||
| throw UnknownColumnType::withContext($unknownTypeException->getType(), sprintf('table %s', $table)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sounds like a reasonable idea but what is this UnsupportedTableDefinition class? I can't find it in the source code or the dependencies.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
mamazu
force-pushed
the
better_error_messages
branch
from
6c06126 to
4a10788
Compare
| private function __construct( | ||
| private string $requestedType, | ||
| ?string $context, | ||
| int $code = 0, | ||
| ?Throwable $throwable = null, | ||
| ) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
What about SchemaManagerFunctionalTestCase?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
mamazu
force-pushed
the
better_error_messages
branch
from
30d5dee to
9caaba3
Compare
src/Schema/AbstractSchemaManager.php
Outdated
| } catch (UnknownColumnType $unknownTypeException) { | ||
| throw UnknownColumnType::newWithContext($unknownTypeException->getRequestedType(), $table); |
There was a problem hiding this comment.
$unknownTypeException and its stack trace are destroyed here. Please always attach the an exception as $previous when upcasting it.
mamazu
force-pushed
the
better_error_messages
branch
from
f85c621 to
252b19a
Compare
mamazu
force-pushed
the
better_error_messages
branch
from
252b19a to
92d7814
Compare
Summary
Adding a way to set context on the
UnkownColumnType. However in the place where I've added it there is no easy way to also get the name of the column, so I'm just providing the table, which should also already help.