-
Notifications
You must be signed in to change notification settings - Fork 29
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
IBX-6173: Added indexes to ezcontentobject_attribute and ezurl_object_link tables #376
Conversation
f2a5fee
to
bd732e3
Compare
bd732e3
to
3b6786e
Compare
eZ/Publish/Core/MVC/Symfony/Security/Authentication/RepositoryAuthenticationProvider.php
Outdated
Show resolved
Hide resolved
@@ -445,10 +445,10 @@ public function testLoadFieldType($content) | |||
{ | |||
$this->assertSame( | |||
$this->getTypeName(), | |||
$content->fields[1]->type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extensive changes to data dumps and changes here suggest that adding this index makes Content Gateway very unstable. It uncovers the known issue that comes back from time to time - gateway doesn't order fields by their position (ezcontentclass_attribute.placement
).
The issue of ordering fields is resolved on API level via ContentMapper, however Handler undergoes consumption BC promise, so it should return data in the same order as well. Whether this is important issue or something we can fix as a follow up, depends on if the issue appears when using MySQL and PostgreSQL. Unfortunately on CI those tests are executed on SQLIte.
Could you please check if 776052f change causes the same issue on MySQL and/or PostgreSQL? If it does then to fix it we need to improve \eZ\Publish\Core\Persistence\Legacy\Content\Gateway\DoctrineDatabase::internalLoadContent
by adding
use eZ\Publish\Core\Persistence\Legacy\Content\Type\Gateway as TypeGateway;
use eZ\Publish\SPI\Persistence\Content\Type;
// (...)
->innerJoin(
'a',
TypeGateway::FIELD_DEFINITION_TABLE,
'f_def',
$expr->and(
$expr->eq('a.contentclassattribute_id', 'f_def.id'),
$expr->eq('f_def.version', $queryBuilder->createNamedParameter(Type::STATUS_DEFINED))
)
)
// (...)
->orderBy('f_def.placement', 'ASC');
I see further updates to legacy persistence tests would be needed, so it's not a quick fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on PostgreSQL and MySQL tests pass without changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on PostgreSQL and MySQL tests pass without changes
Ok, let's fix SQLite as a follow-up. Alternatively as that follow-up you can make sure that use cases for failing SQLite tests are already covered by the proper integration tests (they should) and drop SQLite tests, because they will be dropped eventually, once I have some time to analyze every single one of them ™️ 😉
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
v3.3
Upgrade script: https://github.com/ibexa/installer/pull/112
Checklist:
$ composer fix-cs
).@ezsystems/engineering-team
).