TASK: Core and neos schemas for pgsql and mysql #5398
Conversation
kitsunet
requested review from
bwaidelich,
mhsdesign and
dlubitz
and removed request for
bwaidelich
There was a problem hiding this comment.
Nice ❤️ so does that mean well have postgresql after all for 9.0? Then we should also consider enabling the test to run on postgresql again otherwise we cant be sure it really works.
regarding the warnings, id supress them in the baseline via:
see https://phpstan.org/user-guide/ignoring-errors#generate-an-ignoreerrors-entry
-
message: "#^The internal method \"Neos\\\\ContentRepository\\\\Core\\\\Infrastructure\\\\SchemaServiceInterface\\:\\:.*\" is called\\.$#"
path: Neos.Neos/Classes/FrontendRouting/Projection/DocumentUriPathSchemaBuilder.php
|
Nope unfortunately that part is not uite as easy. The dbal adapter queries are still very much specific for my/maria. I might look into that too, but at least the schemas work now. |
|
so that pr fixes the part that the uri path projection and change projection work with posgresql: #3855 (comment) |
|
we could add a todo to the postgre adapter to use the |
|
ok WTF, the tests say:
My phpstan will not update past 1.10.13 resulting in a different baseline that fails here. |
|
Right because neos/rector pins it to 1.10.13... WHY |
|
jup because it needs phpstan php dependencies or sth. one of the reasons why you must not install rector in the same installation. I have it as a plain composer project because of that. |
|
That is... bad? |
|
But we know this since month an it's documented here: https://github.com/neos/rector#Installation |
|
tbh, I'm a bit skeptical about this PR because it introduces interfaces and factories over what is called a DB abstraction and with that removes colocation of common concepts (i.e. I would prefer to keep common parts together and differences explicit) e.g. via: public function columnForNodeAnchorPoint(string $columnName): Column
{
// on PostgreSQL we use a string type for anchor points because....
return (new Column($columnName, Type::getType($this->isPostgreSQL() ? Types::BIGINT : Types::STRING)))
->setNotnull(true);
}
public function columnForContentStreamId(string $columnName): Column
{
return (new Column($columnName, Type::getType(Types::STRING)))
->setLength(36);
}But then again, ofc it's great to have PG support fixed and we could always re-evaluate that part later |
|
Yep we could totally just do that in a single class, I was thinking maybe sqlite support might follow at some point, surely not for production use, but could be useful for some testing purposes. I would be happy to not spend too much time on this right now as it's all internal but I can refactor it back into a single class ofc. |
| /** | ||
| * The ContentStreamId is generally a UUID, therefore not a real "string" but at the moment a strified identifier, | ||
| * we can safely store it as binary string as the charset doesn't matter | ||
| * for the characters we use (they are all asscii). | ||
| * | ||
| * We should however reduce the allowed size to 36 like suggested | ||
| * here in the schema and as further improvement store the UUID in a more DB friendly format. | ||
| * | ||
| * @see ContentStreamId | ||
| */ |
There was a problem hiding this comment.
it seems they are the same comments like in the interface? i would just like to remove them all in this class then. Comments get outdated so easily and duplicating them is even worse (especially if its internal and no one reads them here ^^ )
| /** | |
| * The ContentStreamId is generally a UUID, therefore not a real "string" but at the moment a strified identifier, | |
| * we can safely store it as binary string as the charset doesn't matter | |
| * for the characters we use (they are all asscii). | |
| * | |
| * We should however reduce the allowed size to 36 like suggested | |
| * here in the schema and as further improvement store the UUID in a more DB friendly format. | |
| * | |
| * @see ContentStreamId | |
| */ | |
| /** | |
| * The ContentStreamId is generally a UUID, therefore not a real "string" but at the moment a strified identifier, | |
| * we can safely store it as binary string as the charset doesn't matter | |
| * for the characters we use (they are all asscii). | |
| * | |
| * We should however reduce the allowed size to 36 like suggested | |
| * here in the schema and as further improvement store the UUID in a more DB friendly format. | |
| * | |
| * @see ContentStreamId | |
| */ |
I agree here. But I think we need some abstraction for a real Postgres adapter later to use Postgres specific (graph) features. |
Related: #3855 Extends the ideas of the DbalSchemaFactory and provides separate implementations for MySql/MariaDB and PostgreSQL, allowing all core tables to be created on both platforms.
kitsunet
force-pushed
the
task/schema-postgresql
branch
from
49f23e4 to
405d66a
Compare
|
There we go, nice and simple, just a couple of ifs, checking platforms, but essentially doing the same thing as the change originally. |
|
Ok so the behavior test failure shows an unexpected dependency on the way the database schema is setup. I didn't quite understand why we would setup "properties" as text type (in dbal terms) and not as a json type. The reason is that we expect a case insensitive collation for searchTerm filters. The |
Ensures mysql gets a column with a case insensitive collation for searching.
|
Ok interesting, I didn't test this locally beforehand, but it seems JSON hard locks the column to utf8mb4_bin collaction, which I find correct, but breaks our search, so I guess is a non starter. Will split the schema between postgres JSON and TEXT with hand picked collaction for mysql... |
Related: #3855
Extends the ideas of the DbalSchemaFactory and provides separate implementations for MySql/MariaDB and PostgreSQL, allowing all core tables to be created on both platforms.