Allow names with more than two dots in the name#4708
Allow names with more than two dots in the name#4708JoshuaBehrens wants to merge 1 commit intodoctrine:3.1.xfrom
Conversation
|
|
||
| if (strpos($name, '.') !== false) { | ||
| $parts = explode('.', $name); | ||
| $parts = explode('.', $name, 2); |
There was a problem hiding this comment.
What if both the schema name and the object name contain dots?
There was a problem hiding this comment.
Then we probably need to implement this exact part in each child from AbstractAsset and evaluate per case :/
There was a problem hiding this comment.
There's currently no API in the DBAL that will allow the support of schemas in SQL Server and PostgreSQL as a first-class citizen. This code is a hack, changing it will likely break some code that relies on it. I don't think we should be making any changes like this. A proper API should be introduced instead.
There was a problem hiding this comment.
I work with SQL Server, SQLite, Psql, MySQL, MariaSQL on a daily basis so I can test them. I can come up with an implementation. I am not sure whether I will design a correct solution for it though, as I just have the knowledge of a few database systems. I assume those who call the _setName shall be able to already give in the separated value? Or do we want to split up the name setter into two?
There was a problem hiding this comment.
I am not sure whether I will design a correct solution for it though [...]
I don't think there's a correct solution here per se. There may be multiple options with certain pros and cons.
I assume those who call the _setName shall be able to already give in the separated value? Or do we want to split up the name setter into two?
In both cases, the challenge is that this method is invoked internally from all schema assets like Table, Column, etc. which accept the name as a single argument in the constructor. How do you plan to change this logic?
In any case, please feel free to experiment. I cannot promise that any of your proposals will be accepted, but we can explore the options.
So far, I'm leaning towards deprecating the support working with multiple schemas at all, since they are supported only by a couple of supported platforms.
There was a problem hiding this comment.
Thank you for the heads-up! There are systems out there that do not support multiple schemas? Are we talking about sth like SQLite?
There was a problem hiding this comment.
SQLite doesn't have a notion of schema at all, AFAIK. But only in PostgreSQL and SQL Server, a schema is a first-class citizen of the data topology. In MySQL, a schema is a synonym of the database, in Oracle and IBM DB2, a schema is a synonym of the user/owner.
Currently, the discrepancy between the platforms in the DBAL is that for PostgreSQL and SQL Server, all schemas within a database are visible to the DBAL consumer but for the rest of the platforms, only the objects within the current user/database are visible.
It should be possible to design the API to support schemas on all platforms that de-facto support them but we'll need to think of an API that would allow preserving the current behavior (e.g. use only the current database/schema on MySQL and use all schemas within the current database on PostgreSQL).
|
Closing as this isn't getting us anywhere. |
|
Unfortunately you are right :/ |
Summary
I stumbled upon an issue using the database of
shopware/platformwith$connection->getSchemaManager()->listTables(). This throws an exception stating that a duplicate index has been added to the table class. That should not be the case as this is straight from the database which would've complained first about it. Debugging into it reveiled the issue. Shopware names their indices with multiple dots:table order
When the index objects set themselves their names they have the following names:
One can already spot the duplicates which cause the error.
I have no idea how to test properly within your framework yet. This is a breaking change which does not just affect indices as this is the base class for basically everything. Guidance needed here.