-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Deprecate extension via Doctrine Event Manager #5784
Comments
I don't want to sound too negative, but that's a bit of a drive-by deprecation, no? Ticket was opened 7 days ago and 6 days ago it was merged. Would be nice to have a bit of a heads-up on stuff that requires consumers to rewrite their stuff. I am currently using the |
Isn't that exactly what a deprecation does? What would you do?
I don't think that's the right thing to do, see https://doctrine.slack.com/archives/CA9600CLC/p1666886106048959 |
I just mean the plan to deprecate was announced (i.e. this ticket opened) and one day later it was already implemented. It just seems like it would have been nicer to keep it open for a while to see if there is any user feedback (Also, some migration guide would really be appreciated).
That wants me to log into something. Is there a public version of it maybe? |
The deprecation of the event system happened in an effort to consolidate our extension points. But if we discover that this deprecation was premature, we can talk about it. Can you please open a new issue where you explain for what purpose you use the event system and why extending the platform would make that more difficult for you? |
Sure, I can look into that. To be clear: I don't mind you deprecating stuff as long as there's an alternative way to implement the functionality I need. Just to make sure: @derrabus you think extending Platform (as in BTW, one scenario I use I described here: |
According to https://doctrine.slack.com/archives/CA9600CLC/p1666886106048959, it isn't: |
@greg0ire thanks for the info! I just tried to figure this out, but somehow I'm stuck: AFAICT the dbal/src/Platforms/SqlitePlatform.php Lines 1481 to 1484 in d3b8e80
So to use a custom dbal/src/Driver/AbstractSQLiteDriver.php Lines 24 to 27 in d3b8e80
And to override the Lines 245 to 253 in d3b8e80
Which means I'd have to copy the dbal/src/Driver/SQLite3/Driver.php Line 9 in d3b8e80
Is there something I'm missing? |
… if you don't pass one via the |
@derrabus thanks, that helped! So for $db_config = [/* config as before, driver set to pdo_mysql or pdo_sqlite */];
if ($db_config['driver'] == 'pdo_mysql') {
$db_config['platform'] = my_mysql_platform::class;
} elseif ($db_config['driver'] == 'pdo_sqlite') {
$db_config['platform'] = my_sqlite_platform::class;
} else {
throw new Exception('Oh noes!!!');
}
/* startup connection as before */
class my_mysql_platform extends Doctrine\DBAL\Platforms\MySQLPlatform
{
public function createSchemaManager(Doctrine\DBAL\Connection $connection): Doctrine\DBAL\Schema\MySQLSchemaManager
{
return new my_mysql_schemamanager($connection, $this);
}
}
class my_sqlite_platform extends Doctrine\DBAL\Platforms\SqlitePlatform
{
public function createSchemaManager(Doctrine\DBAL\Connection $connection): Doctrine\DBAL\Schema\SqliteSchemaManager
{
return new my_sqlite_schemamanager($connection, $this);
}
}
class my_mysql_schemamanager extends Doctrine\DBAL\Schema\MySQLSchemaManager
{
protected function _getPortableTableColumnList($table, $database, $tableColumns)
{
$tableColumns = onSchemaColumnDefinition($tableColumns);
return parent::_getPortableTableColumnList($table, $database, $tableColumns);
}
}
class my_sqlite_schemamanager extends Doctrine\DBAL\Schema\SqliteSchemaManager
{
protected function _getPortableTableColumnList($table, $database, $tableColumns)
{
$tableColumns = onSchemaColumnDefinition($tableColumns);
return parent::_getPortableTableColumnList($table, $database, $tableColumns);
}
}
function onSchemaColumnDefinition($tableColumns)
{
/* event listener code goes here */
return $tableColumns;
} Not exactly pretty, but I guess somewhat manageable, if those protected functions stay stable at least. The |
Yes, that looks about right. |
Hello, I am the author of the Slack question mentioned above. Thank you for your discussion and suggestions. I am trying to implement the solution proposed by @flack , but I am running into the fact that I will probably have to extend the Driver as well. The reason is that although I pass the platform in the connection configuration, the original SchemaManager is created in the Driver's getSchemaManager() method and my createSchemaManager() method is never called. This brings me back to the fact that Doctrine\DBAL\Driver\PDO\PgSQL\Driver is final. Did I understand that correctly, please? Thanks! |
@jaroslavlibal use Doctrine\DBAL\Connection;
use Doctrine\DBAL\Driver;
use Doctrine\DBAL\Driver\API\ExceptionConverter;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Platforms\PostgreSQLPlatform;
class YourCustomDriver implements Driver
{
public function __construct(
private readonly Driver $underlyingDriver
) {
}
public function getSchemaManager(Connection $conn, AbstractPlatform $platform)
{
return $platform->createSchemaManager($conn);
}
public function connect(array $params): Driver\Connection
{
return $this->underlyingDriver->connect($params);
}
public function getDatabasePlatform(): AbstractPlatform
{
return $this->underlyingDriver->getDatabasePlatform();
}
public function getExceptionConverter(): ExceptionConverter
{
return $this->underlyingDriver->getExceptionConverter();
}
} use Doctrine\DBAL\Driver;
class Middleware implements Driver\Middleware
{
public function wrap(Driver $driver): Driver
{
return new YourCustomDriver($driver);
}
} if you configured everything else correctly this code will make sure your custom schema manager is used in my case, i've extended doctrine:
dbal:
platform_service: <yourCustomPlatform> |
The effort and goal is clear, the way this has been done unfortunately is very rough. API is inconsistent and could be done better: ok, we can agree on that. But deprecate an extension system in full without a single day of discussion is unpleasant, especially if done in a such popular project like this one. On technical aspects:
IMHO the way this thing has been done is not very nice to the community, and lacked a wider vision on how this project is used in the wild. What makes me sad is the low consideration of the community feedback: 24 hours issue is barely a communication and left no space for discussion. To me this is not acceptable in a such important project: it is widely used, production apps are written onto it and should take this responsibility seriously giving the community the time to evaluate such an important change and express an opinion. |
I don't know if you realize that or if you just haven't read the previous comments, but we're actually having the discussion you're asking for. A deprecation is not an immediate removal. And we we can surely take it back if you give us good reasons to.
Do you think, an event handler is the best way to expose this kind of extension point? If you designed this feature from scratch, how would you build the extension point that you need for your scenario?
Why would you need to hook into the DBAL to add a column? Why can't you add it to the schema directly? Sorry if my question sounds stupid, but I'd like to understand you problem before we decide on an action here.
Can you give us examples? Is this something the ORM should address instead?
The event manager is a trivial library. Sunsetting it is not our intention and certainly was not the motivation here. |
From my POV, a discussion is needed before to take action, not after. This is a communication of what's have been already done, and doesn't matter if it can be un-done. A discussion can take place on a proposal, which this is not. This is a fact: events have been deprecated in the current public release and removed in the first beta of the next major version.
Yes, why not? An entire framework (Symfony) has been written around an event manager, where handlers continuously modify and process information and structures, for example enhancing the request object, transforming the request body, etc.
I'll answer two questions here because they are connected. I'll give you some examples:
So those listed under "Problems with the EventManager API as such" are not valid reasons to deprecate and remove the event system? Because a PSR-compatible event dispatcher will solve both problems. And lately, will increase the integration level with the frameworks in general. |
And you would've participated in that discussion? Would you have noticed that it even happens? During the last months of DBAL development, I have barely seen any real discussion going on any PR. Users of the DBAL don't monitor this repository and that's absolutely okay. Trust me: We could've kept that PR open for two more weeks and the only difference would've been that you would have one thing less to complain about. Really, I value every constructive feedback that we get from the community. But please think about how you address people who invest their free-time to build libraries that you can use for free. A phrase like "this is not acceptable" feels a bit inappropriate here, don't you think?
I think, this is the issue we have to solve. We need to enable you to modify the schema after the ORM has generted it from the metadata and before it's being used for comparisons with the actual database.
Do you think, we should enable you configure vendor-specific tweaks like that in the ORM?
You should be abler to configure patterns of table names that should be ignored during schema comparisons, shouldn't you? I have to look that up though, not entirely sure about how to configure that.
No, if those were our only problems with the event system, we wouldn't have deprecated it. You are right, a PSR-4 dispatcher would be the solution here.
I agree. If you want to work on switching the ORM to PSR-14, don't hesitate to investigate a possible migration path. |
Really, I don't want to open a flame, so just two things on these and then I want to discuss only the technical aspects: presuming that the discussion would pass unnoticed is wrong in my opinion, and no, to me is not inappropriate because is my point-of-view.
That's the problem the event system resolved. If we can find an alternative way, then the migration path would be clear and sound and will not be a feature removal.
IMHO no, it's not a thing the ORM should handle. It is a detail that has no space in the ORM, but the underlying library (this one) should do.
I try to explain it better: I don't want to simply ignore some assets, I need to generate them alongside the tables and columns of the schema generated by the ORM. Solving the problem of modifying the schema generated by the ORM before passing it to DBAL will probably work for these two.
I did not write that they are the only problems, but that we shouldn't consider them as valid reasons for this change, so we can remove them from the discussion.
Ok, I'll do. |
Well then I don't know why I waste any more of my free-time on having a this conversation with you. If we're not the first open-source project to tell you that, maybe your attitude is the problem?
The way you lecture us on how our development process should look like is really disrespectful. "This is my opinion" is really a weak argument for any conversation. If you just want to state your opinion, keep it for yourself next time.
No, the event system healed a missing extension point. You hooked into the DBAL because you could not hook into the ORMs schema generation.
All right.
Okay, so we should really try to build that extension point in the ORM. Thank you for those answers, that was really helpful. |
I cannot hook into the ORM schema generation, but even if I could, it will not resolve the point of setting vendor-specific details on how the table/view/... should be created. To be more clear, I'll give you an example: the Also, appending custom SQL to the generated asset (schema/table/view/index/etc) is not possible with the current objects. I did not meant to hurt or offend anyone, but you're going down on a personal level in the last message.
Sorry, but on if, when and how to deprecate something, every decision is opinion based. So "this is my opinion" is the only thing that counts, because there are no technical reasons to remove something that works and worked well for years.
I really have to answer these statements? And I am the one who's disrespectful? |
Well, but you have and you've made it very clear that you don't care.
You'd be surprised.
I don't.
No.
Yes. |
To anyone who had to witness this unfortunate exchange of messages: The Doctrine project values constructive feedback from users of our libraries. If a change that we've shipped caused trouble in one of your projects, get in touch with us. We are very careful not to break downstream code, but if we do, it certainly does not happen on purpose. Back to topic: I understand that if you rely heavily on DBAL's events, the deprecation of the event system might appear a bit premature. But as of today, DBAL 3 is still maintained and the event system is still working. We've release DBAL 4 as a first beta and don't have a release date for a final version yet. Right now, you can help us smoothen the upgrade path and shape the 4.0 release with us. The Doctrine project is maintained by very few people in their free-time. Any additional help is appreciated. I have created issues for the 3.6.0 milestone from the feedback I've gathered after the 3.5 release. Comment on those or open new issues if you think, we've missed something. This issue will be locked now. |
Recently, I tried to enable checking exceptions in PHPStan and, besides a few other issues with the DBAL API, stumbled upon the one that shows what an awfully overcomplicated piece of work the Events API is:
AbstractPlatform::getDropTableSQL()
dispatches anonSchemaDropTable
event which can override the SQL used to drop the table.SchemaDropTableEventArgs::getSql()
is?string
. If it returnsNULL
, the platform will throw anUnexpectedValueException
.on
are event handlers. Those methods are not invoked from anywhere in the code explicitly.Here's a chain of questions that leads to the above conclusion:
SchemaDropTableEventArgs
allowed to returnNULL
if it's not even a valid value?string
but then what would it return if the handler of the event didn't callsetSql()
?SchemaDropTableEventArgs
class do if it was instantiated and then got itssetSql()
invoked? Maybe throw a logical exception?preventDefault()
first?Problems with the API design and implementation
What this API does, most likely could be solved just by extending the platform class and overriding the corresponding methods. Besides this awfully implicitly stateful logic, a few more concerns about DDL events:
parent
class).parent
method would.DROP TABLE
but cannot overrideDROP FOREIGN KEY
).onSchemaDropTable
event represent? Let's assume it's a fact that somebody intended to drop a table. Why is it mutable? Why does it carry any information about processing the event (prevent default) and even implementation of that processing (SQL)?Problems with the EventManager API as such
This API is unsafe from the standpoint of the types:
onSomething
) is dispatched only with a specific type of arguments (SomethingArgs
).The mistakes in the code that implements the handling of events will lead to a type error at runtime.
Types of events
dbal/src/Events.php
Line 21 in a85d913
dbal/src/Events.php
Lines 33 to 35 in a85d913
dbal/src/Events.php
Lines 23 to 30 in a85d913
dbal/src/Events.php
Lines 31 to 32 in a85d913
The text was updated successfully, but these errors were encountered: