jackalope / jackalope-doctrine-dbal

Doctrine DBAL transport implementation for Jackalope
http://jackalope.github.io
Other
143 stars 60 forks source link

Add support for doctrine/dbal 4 version #436

Closed alexander-schranz closed 8 months ago

alexander-schranz commented 8 months ago

This adds a CI run against @dev which we should I think have to get earlier when a dependency would break us.

Also it adds doctrine/dbal: ^4 to check what we require todo there so we can keep eventually BC breaks in mind for this jackaleop-doctrine-dbal 2.0 release.

TODO

dbu commented 8 months ago

thanks for the work! good progress already!

oh no mysql and its encoding mess :( can we be more explicit about encoding to not rely on defaults? but we would need to avoid messing up with existing legacy setups, which can be tricky. as this is 2.x we could do a BC break to require an explicit encoding configuration with mysql.

i am worried about the deprecation warning spammed over the test output, we seem to use the ObjectManager in a weird way.

alexander-schranz commented 8 months ago

Yeah mysql encoding part is very stricky as we don't know what is correct encoding for our case sensitive field now. I also did not yet found out why dbal skipping set a default charset in the new version. Maybe @derrabus know more here. I'm also not familiar enough with the code.

The current getParams we use to get the charset is already marked as @internal, but think also it was when we implemented the handling.

alexander-schranz commented 8 months ago

I did rethink about the charset issue as it looks like symfony already defines for most connections already the charset.

https://github.com/symfony/recipes/blob/ca74983cffe05433f8fef460c72250662acb695b/doctrine/doctrine-bundle/2.10/manifest.json#L14-L16

We could maybe go the way throw a error if no charset instead fallback to utf8 here: https://github.com/jackalope/jackalope-doctrine-dbal/blob/59a7b05f7a4bebd6ce3bdf8155d5f20a72485e57/src/Jackalope/Transport/DoctrineDBAL/Client.php#L384

Just with some friendly error message jackalope-doctrine-dbal requires a defined charset definition, please configure it in your dbal connection via charset option.

dbu commented 8 months ago

that is indeed an idea, yeah. we should validate in the setup already - afaik the dbal connection is injected to the client with the params already in it, but we could check the params there. that would provide a better stack trace than doing it in the middle of everything.

and we also need to update the documentation and examples, also for the bundle configuration.

dbu commented 8 months ago

wrapped up in #439 :sweat_smile:

alexander-schranz commented 7 months ago

@dbu Thx