Closed (fixed)
Project:
Group
Version:
8.x-1.x-dev
Component:
Group (group)
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
31 Jan 2019 at 20:53 UTC
Updated:
2 Dec 2019 at 15:54 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
kristiaanvandeneyndeI would definitely be up for committing any patch that adds said index to the database. Currently a bit busy with the query access handler work, so a head-start would be appreciated :)
Comment #3
berdirWill see if I can provide something, not high on my priority list either as we don't have enough data (yet) where this results in a measurable performance improvement. Also applying patches with update functions is always tricky.
Comment #4
lobsterr commentedI have added the storage_schema, which provides the required index. I added an update hook, which add storage schema class to group content, but I couldn't find the way how to run update of the entity in order to apply the schema changes.
I do know how to update the field, when we add an index (addSharedTableFieldIndex) for specific field ()
In our case it is a composite index.
If somebody knows, how to do it, please let me know I will update the patch
Comment #5
berdirTry to use updateFieldableEntityType() instead, maybe that will work better.
If that doesn't help, \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::onEntityTypeUpdate() is the method you need to debug.
Basically, requiresEntityStorageSchemaChanges() needs to return TRUE and hasSharedTableStructureChange() needs to return FALSE, so it goes into the "sweet spot" of just re-creating the indexes.
Comment #6
lobsterr commented@Berdir, thank you for all your help. Debug helped me to understand, that I just missed use statement for GroupContentStorageSchema class and it was never used, but you are right no manual adding of the index is required.
Comment #7
kristiaanvandeneyndeWhy TRUE instead of passing on $reset?
I've changed this to $reset for now, rerolled the patch and moved a few files/comments around.
Ideally, this would have a test proving that the update works. But if anyone could point me to a core test showing that the used functionality indeed updates just the indexes, then I'm fine committing this without an update hook test.
Comment #8
lobsterr commented@kristiaanvandeneynde. Yes, I have forgotten to remove TRUE, it was done for a test purpose.
I have checked other cases in the core and I didn't any tests for indexes
Some examples:
https://www.drupal.org/project/drupal/issues/2183231
https://www.drupal.org/project/drupal/issues/3007669
Comment #9
berdirThe update looks good and I just tested it on our site.
> But if anyone could point me to a core test showing that the used functionality indeed updates just the indexes, then I'm fine committing this without an update hook test.
It's a bit more complex than this, the update function does apply the non-destructive changes based on the changed definition and code. So right now, it does only make that change. In case we'd add more code to the storage schema class, it would attempt to add that too.
But there is no other way, that's how it is done in core as well.
Comment #11
kristiaanvandeneyndeFixed, thanks!