Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
testbot playground for #1497374: Switch from Field-based storage to Entity-based storage
If you want to help, please
- Pick a failing test from the latest patch
- File an issue on https://drupal.org/sandbox/chx/1857558 with the classname as the node title
- Fix test
- File patch
- Rinse and repeat
There are no systematic issues we are aware of, it's the tests themselves that are broken and need to be fixed one by one. Ping chx on IRC for help as necessary.
Comment | File | Size | Author |
---|---|---|---|
#132 | interdiff_123_124.txt | 126.06 KB | chx |
#131 | field_storage-testbot-2047777-130.patch | 603.97 KB | yched |
#128 | field_storage-testbot-2047777-127.patch | 603.91 KB | yched |
#126 | field_storage-testbot-2047777-126.patch | 598.55 KB | yched |
#124 | field_storage-testbot-2047777-124.patch | 598.45 KB | yched |
Comments
Comment #1
yched CreditAttribution: yched commentedCurrent state of the sandbox.
Comment #3
yched CreditAttribution: yched commentedInvestigating those numerous "FieldException: Attempt to create an instance of unknown, disabled, or deleted field XXX in Drupal\field\Plugin\Core\Entity\FieldInstance->__construct()" fails.
Comment #4
yched CreditAttribution: yched commentedA couple fixes.
Comment #6
yched CreditAttribution: yched commentedBetter with a reroll.
Comment #8
yched CreditAttribution: yched commentedHmm, that's *more* fails :-(.
I can only assume that less early fatals means more asserts get a chance to fail (there is also more passes...)
Comment #9
yched CreditAttribution: yched commentedThat was the case. All upgrade tests now failed a bit later, with 2 fails and 4 warnings instead 1 fail and 1 warning in previous patch, that pretty much accounts for the difference...
Attached patch should fix upgrades, and a bunch of other fatals.
Comment #11
yched CreditAttribution: yched commentedOther attempt at fixing the upgrade path.
taxonomy_update_8007() accounts for a column being renamed (tid -> target_id)
- It needs to run *before* field_update_8006(), because that one only moves data columns that have the expected column name as per the current field schema (target_id)
It would probably be best if field_update_8006() just moved all the columns it found in the original tables (removing the need to rely on the runtime field schema / columns), but I'm not sure there's a way to do that. @chx ?
- It then needs to hardcode the old table names, and cannot use DatabaseStorageController::_fieldTableName() since that one will return the new table name, which will only exist after field_update_8006().
Comment #13
yched CreditAttribution: yched commentedTrying something.
Comment #15
yched CreditAttribution: yched commentedDrats. Re-trying after reroll.
Patch additionally contains a couple more fixes.
Comment #17
yched CreditAttribution: yched commentedOK, reverted my attempt on the query in field_update_8006() (see interdiff from #13)
Just fixed the method name (->fromQuery() should be from()), and db_select()->fields() needs a table alias.
Plus a couple more fixes here and there.
Comment #18
yched CreditAttribution: yched commentedIdentified, but not fixed yet:
- user_update_8012() needs some special care, it tries to write to the old data tables. See fails in UserPictureUpgradePathTest
- Tests having errors like "Attempt to create an instance of unknown, disabled, or deleted field %foo" in FieldInstance->__construct() probably come from tests that try to create two instances on two different entity types, confident that the field exists - works in HEAD, but now they need to create new fields on each entity type... I fixed one in FieldAttachOtherTest
- we do have errors caused by table names getting too long...
I fixed a couple by reducing random strings / field names, but we'll need a more general fix.
- Help welcome on tests for which the qa.d.o only reports "The test did not complete due to a fatal error"
I'm done for today. Help moving this forward is welcome :-)
Comment #19
yched CreditAttribution: yched commentedRegarding the table names: I'm not sure how we're going to escape including an auto truncating mechanism in DSC::_fieldTableName(). That would mean storing and fetching a map in state()...
Comment #21
yched CreditAttribution: yched commentedReroll after #2045919: Replace all module_implements() deprecated function calls..
Also - I won't be able to handle all the test fails all by myself, need help here :-)
Comment #22.0
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #23
chx CreditAttribution: chx commentedI will throw myself into the fray tomorrow.
Comment #24
amateescu CreditAttribution: amateescu commentedRerolled after #2017207: [meta] Complete conversion of users to Entity Field API and fixed a lot of tests. Let's see what's left :)
Comment #26
yched CreditAttribution: yched commentedw000t !
Comment #27
yched CreditAttribution: yched commentedShould fix a couple more.
Comment #29
yched CreditAttribution: yched commentedYep, silly me.
In addition, should hopefully fix OptionsWidgetsTest.
Comment #31
yched CreditAttribution: yched commentedPushed fixes for:
EntityQueryTest
ContentTranslationSyncImageTest
NormalizeTest
NodeAccess*Test
EntityResolverTest
EntityTranslationTest
The remaining fails are in Views and upgrade path.
Will try to look into views before posting an updated patch.
Comment #32
yched CreditAttribution: yched commentedShould fix at least some views fails.
I'd really welcome help on the upgrade fails, running them locally makes my poor crappy laptop crawl to a corner and die :-p
Comment #34
yched CreditAttribution: yched commentedmerged HEAD
Comment #36
chx CreditAttribution: chx commentedHere's an attempt at the field upgrade. It introduces a new update field pseud-hook that needs to be documented.
Comment #37
plach@chx:
No patch :)
Comment #38
chx CreditAttribution: chx commentedComment #40
yched CreditAttribution: yched commentedFYI - pushed:
- Merged HEAD
- re-fix DenormalizeTest after #1972116: Add REST test coverage for nodes.
Does not deserve a new patch for now...
Comment #41
amateescu CreditAttribution: amateescu commentedPushed fixes for all Views tests. We're getting very close :)
Comment #42
yched CreditAttribution: yched commentedThe comment would need to be updated accordingly.
Also, that's not the problem here - user_8011, as currently coded, puts data in the old field tables.
Either it stays this way and needs to run *before* field_8006, or it stays running after but then has to be modified to put data in the new tables.
The latter seems preferable ?
Why is that ?
Comment #43
amateescu CreditAttribution: amateescu commentedFixed Drupal\file\Tests\FileListingTest and Drupal\system\Tests\System\PasswordHashingTest.
We're down to only two tests:
- Drupal\system\Tests\Upgrade\ModulesDisabledUpgradePathTest
- Drupal\system\Tests\Upgrade\UserPictureUpgradePathTest
@chx, your turn :)
Comment #44
chx CreditAttribution: chx commentedBecause... you can't upgrade custom indexes, not easily, not with the column names changing. In case, the table creation simply fails if we copy over the fid index... we can add another hook if you want. Or we can say that people with custom data need to write custom upgrades.
Comment #45
yched CreditAttribution: yched commentedHm. Then I'm afraid it's more serious than that.
We're not supposed to rely on plugins within updates - #2055605: Disallow use of plugins during update. That means, no asking the "field type" plugin for the schema() of a given field.
#2032369: _update_8003_field_create_field() relies on field schemas never changing and plugins being available, for example, changed _update_8003_field_create_field() calls to have the caller explicitly inject a hardcoded 'schema' for the field to create.
In our case here, this means we're not supposed to call DatabaseStorageController::_fieldSqlSchema($field) in field_update_8006().
And de facto, the current field_update_8006() will fail on fields whose type is unknown.
The ideal flow for field_update_8006() our case would be:
1) Create a new table by just copying the structure of the existing one
Just take what's there and reproduce it as is, no calling runtime code to rebuild the schema we expect (which includes column names mismatches)
2) Copy the relevant values as is (all columns, no renames) from the old table to the new table
3) Drop the 'entity_type' column in the new table
Done - then each field type module can take care of renaming columns and indexes in their own separate updates, that just need to run after field_update_8006().
1) is just "CREATE TABLE new_table LIKE old_table" in MySQL - problem is, of course, I don't know if we have a cross DB / PDO API in Drupal that allows that :-/.
Comment #46
chx CreditAttribution: chx commentedThat's the problem: we do not have a way to copy table structures in Drupal or elsewhere.
We could introduce
db_copy_table('src', 'dst');
.In MySQL this is
CREATE TABLE dst LIKE src
In SQLite we could read the definition of src by
SELECT sql FROM sqlite_master WHERE tbl_name='src'
and then massage it. We need to deal with CREATE TABLE and CREATE INDEX separately but it is definitely doable.In PostgreSQL we can do
CREATE TABLE dst (LIKE src INCLUDING ALL)
.In MSSQL you are SOL.
Oracle has an SQLite-like solution
SELECT dbms_metadata.get_ddl( 'TABLE', 'src' ) FROM DUAL;
Comment #47
yched CreditAttribution: yched commentedHm, also, we currently don't drop the old table at the end ?
Comment #48
chx CreditAttribution: chx commentedWe never drop tables during updates -- why would we? Let the users have their data in case we missed something. Dropping tables is easy manually.
Comment #49
yched CreditAttribution: yched commented[edit: #47 was a crosspost]
Does #46 mean "let's go for it" ?
Also, one issue with our support for db_insert()->from($query) is that it assumes $query selects an explicit list of fields, the code doesn't support "INSERT INTO new SELECT * FROM old WHERE..." (that syntax is supported in MySQL, pgSQL & SQLite, though)
That one is easily worked around, though - first do a "SELECT * FROM old LIMIT 1", then grab keys in the result. Meh.
Comment #50
chx CreditAttribution: chx commentedYes, #46 means I will add a copy table. Ouchie.
Comment #51
yched CreditAttribution: yched commentedWay cool. Thanks :-))
Comment #52
chx CreditAttribution: chx commentedRe #49,
is in core, it's certainly supported.
Comment #53
chx CreditAttribution: chx commented#2056133: Add db_copy_table_schema has been filed
Comment #54
yched CreditAttribution: yched commentedre #52 - it's not, I tested it :-)
If you look at the existing core examples for ->from($query), $query always uses an explicit list of fields.
I opened #2056363: INSERT INTO table SELECT * FROM ... not supported
Comment #55
chx CreditAttribution: chx commentedThanks for that issue, we might be green with it. This contains both the db_copy_table and that INSERT patch for testing purposes.
Of particular note is the passing of the field schema into _fieldSqlSchema to avoid an (impossible) plugin lookup. We already mandate it on a doxygen level for _update_8003_field_create_field and it is indeed specified in both usages of _update_8003_field_create_field so it was easy.
Comment #57
chx CreditAttribution: chx commentedyay last exception! Very easy fix, just a new test needing a conversion.
Comment #59
chx CreditAttribution: chx commentedMerge conflict resolved.
Comment #60
yched CreditAttribution: yched commentedYay, green !
Refactored a couple stuff in field_update_8006():
- inlined a couple vars, added a couple comments
- drop the 'entity_type' column in the new migrated tables
Plus:
- moved file_8003 dependency from field_8003 (CMI) to field_8006 (table reorg)
- changed taxonomy_8007 to run after field_8006 - taxonomy_8007 and file_8003 do exactly the same thing, better if they work the same way.
Did not push yet, let's see what the bot says.
Comment #62
yched CreditAttribution: yched commentedDoh, #60 updated the comment but forgot to actually bump the dependency number for taxonomy_update_8007...
Comment #63
yched CreditAttribution: yched commentedOk, pushed #60 / #62.
Comment #64
yched CreditAttribution: yched commentedLooks like this change is not needed, FieldUpgradePathTest is still green if I revert it locally.
@chx, is there another reason or can we just revert it ?
Comment #65
jibranHere is my not very technical review. Ignore whatever is irreverent.
@return missing.
I don't know but this should be on same line till 80 chars limit and no empty line needed.
order is wrong.
{@inheritdoc}
typehint missing
Can we add @see here?
typehint missing.
Why not use rev here as well? I like rev. If it doesn't break api.
This needs proper doc block.
This should be injected but probably in follow up.
This should be on same line
typehint missing.
We should inject/pass __construct the moduleHandler or at least use Drupal::
typehint missing.
Can we change this to "field:{$this->entityType}:$id"
Do we need @return if yes then typehint missing.
Should be on same line.
Can we use Drupal:: and not use deprecated functions anymore?
Doc block above this function needs update.
Doc block needs update.
Extra white space.
Little description will help.
Doc block need @param and @return.
typehint and doc block.
Typehint.
Can we use Drupal::?
I'll try to review remaining patch. Not changing status cuz I don't want anyone to kill me. :D
Comment #66
yched CreditAttribution: yched commentedMerged & pushed HEAD.
Re @jibran #45: Thanks for the review. Answering below, assuming remarks are numbered.
1. That's for #2056133: Add db_copy_table_schema, that is just included here until it gets in. + I don't think we add @return statements when there is no return value.
2. Nope, 1st line has to be one sentence whenever possible. Then, empty line and additional remarks.
3. I don't think we have an official standard about the order of "use" statements, but why not. And grouping is better indeed. Fixed.
4. Not part of the patch ?
5. 6. 7. Fixed
8. table names are being adjusted in the main issue
9. Not part of the patch
10. Not introduced by the patch + no injection possible in entity controllers for now
11. Same as 2.
12. Fixed
13. This code is just moved around as is, let's keep it that way, there will be issues to remove module_invoke_all()
14. Fixed
15. Why not. Fixed.
16. That comment is moot anyway. Removed.
17. Same as 2. Fixed to split the second sentence to a separate paragraph.
18. Same as 13. This patch is too massive to do that here.
19. 20. 21. Fixed.
22. True, but _update_8000_field_sql_storage_write() is just copied as is from the previous field_sql_storage.install, and I'm still pondering about it. Left as is for now.
23. Fixed
24. Already the case in HEAD, patch is big enough.
25. It's true for most of FieldInfo methods, but fixed this block.
26. Already the case in HEAD, not for this patch.
Pushed the attached interdiff
Comment #67
jibran@yched Thanks for explaining all the points in detail.
Review of remaining patch as promised.
Can we use "$entity_type.$bundle.{$this->name}".
Order :)
typehint missing.
Comment #68
yched CreditAttribution: yched commentedThanks.
Fixed, except createFileField(). The whole FileFieldTestBase.php is not correctly hinted, including the sister attachFileField() method; and at this point we're really trying to limit the patch size.
Comment #69
yched CreditAttribution: yched commented_update_8000_field_sql_storage_write() was still calling field_sql_storage module functions directly, not sure how block upgrade passed so far.
Checking a candidate fix.
Comment #70
yched CreditAttribution: yched commentedAttempt at removing hook_field_storage_pre_[load|insert|update]()
Comment #71
yched CreditAttribution: yched commentedAnd attempt at handling "table name too long" as per #1497374-146: Switch from Field-based storage to Entity-based storage
Comment #72
yched CreditAttribution: yched commentedAlright, then attempt to revert the test changes that were made to avoid "table name too long"
Comment #73
chx CreditAttribution: chx commentedFantastic. fubhy suggested that we re-enable (!) the modules before uninstalling them so to remove the hacking of EntityManager.
I will explore this during the week.
Edit: nope. berdir points out that enabling might be a problem as dependencies might be missing. We will just leave it in and add a todo to kill the hack when disabled modules are gone.
Comment #74
yched CreditAttribution: yched commented@chx: OK - you take care of the the todo ?
Comment #76
yched CreditAttribution: yched commentedDoh, simpletest pefixes are 16 chars, not 14...
Fixed that, and moved the truncate / hash code to a helper method (see patch in the main issue)
Comment #77
yched CreditAttribution: yched commentedTrying to fix views integration, as mentioned in #1497374-158: Switch from Field-based storage to Entity-based storage
Comment #78
swentel CreditAttribution: swentel commentedTesting refactoring of FieldInfo::getFieldMap() - $field->getBundles() needed refactoring immediately too.
Comment #80
yched CreditAttribution: yched commentedViews intagration - a couple fixes
Comment #82
swentel CreditAttribution: swentel commentedSome obvious fixes for infomap/getbundles
Comment #84
swentel CreditAttribution: swentel commentedWith another obvious fix
Comment #86
swentel CreditAttribution: swentel commentedComment #88
swentel CreditAttribution: swentel commentedThis should be green. @yched I also had to change code in field.views.inc so I'll push once this one comes back green.
Comment #89
yched CreditAttribution: yched commentedHah, this one should hopefully be green too :-)
But no pb, I can reroll on top of your changes ($field['bundles'], right ?)
Comment #90
swentel CreditAttribution: swentel commented$field['bundles'] indeed. Bot hasn't even started yet and many waiting in the queue, looks like the merge will probably be tomorrow :)
Comment #91
yched CreditAttribution: yched commented@swentel - remarks on #88:
Space added at the wrong place ;-)
This was looping on the entity_types, so it looks like the foreach is not needed at all now ?
Needless empty line :-p
Shouldn't be needed, it can stay keyed by field UUID ?
Then just iterate on $field_map[$entity_type] ?
Similarly, we just use the entity_type, so the loop is not needed anymore ?
Whoops. That's correct, but we haven't updated the phpdocs for field_info_fields() & FieldInfo::getFields()
The code to filter out deleted fields in field_info_fields() needs to be updated as well...
My bad :-/, I'll fix that.
(Minor: while we're in there, $field_info could be renamed just $field here.)
Also, keys are not field names here, you need to use $field->name (or $field['field_name'] to keep BC-style)
Comment #92
yched CreditAttribution: yched commentedRegarding the last snippet - I was confused, it's worse than that actually.
field_info_fields() is currently broken, it still returns an array of fields keyed by field names, but that's wrong, there could be several fields with the same name in different entity types.
It needs to return an array keyed by entity type then field name, and then all current uses need to be updated as well :-/
Let's attack this after the current pending changes have been merged though...
So for now, RelationLinkManager::getRelations() should still assume a flat array of fields keyed by names.
Comment #93
swentel CreditAttribution: swentel commentedRight, sometimes you focus so hard on just getting something green .. :)
Update with interdiff on the interdiff. So yeah, RelationLinkManager::getRelations(), that's a weird one, it failed during refactoring, so somehow this magically works, but I'm too tired now how exactly it can work though.
Hmm, well fieldInfo->getFields() stores the field by uuid, so I think we're safe no (in regards of uniqueness at least) ?
Also, in getFieldMap(), there's this little piece of comment, especially the second part. We don't check on unknown entity types - not before either - should we add a check here ? There's a check though in getBundleInstances() - but again, a bit too tired right now to recheck. Minor is that 'type' is always overridden, but I guess that's not to critical.
Comment #95
yched CreditAttribution: yched commentedRight, fieldInfo->getFields() is good, but field_info_fields() has additional code to filter out deleted fields and re-key by field name. It needs to either re-key by entity_type then field_name, or just keep the UUID keys. Either way, phpdoc & consuming code needs to be updated. Well, let's get this green, and we'll see that later.
re: getFieldMap() / "Filter out instances of inactive fields, and instances on unknown entity types"
Hmm, tricky. I opened #2059965: Should FieldInfo::getFieldMap() filter data about unknown entity types. Let's leave it as is for now :-)
The '*' chars are misaligned in the phpdoc for field_info_field_map(), wrong copy paste from getFieldMap() ;-)
Comment #96
yched CreditAttribution: yched commentedAnd right, 'type' gets overwritten over and over again in getFieldMap(), but it's always the same type, so no need to bother with "set it only the first time around".
Comment #97
swentel CreditAttribution: swentel commentedShould be better, somehow missed my views changes in the last patch. No interdiff. Changed the '*' chars (djeezus man) and refactored RelationLinkManager::getRelations().
Also noticed that hook_field_views_data_views_data_alter() is not documented in any api file. I didn't refactor _field_views_get_sql_entity_types() (just return string instead of array) yet as that would trigger a lot of other changes as well. let's spin that off in a follow up ?
Comment #99
yched CreditAttribution: yched commentedyes, _field_views_get_sql_entity_types() is removed in the 'views intergration refactor patch'
Comment #100
swentel CreditAttribution: swentel commentedRight, and that's somehow being annoying at this point. Going for another botcheck first, but some views tests are failing.
Comment #102
swentel CreditAttribution: swentel commentedOk, I've merged your patch with mine, let's see - one local test was already ok, so good hopes. interdiff attached.
Comment #104
swentel CreditAttribution: swentel commentedright, forgot to update the hook implementations ...
Comment #105
swentel CreditAttribution: swentel commentednew patch after yched pushed his views refactoring.
Comment #107
swentel CreditAttribution: swentel commentedman ...
Comment #109
swentel CreditAttribution: swentel commentedThat was a nasty one. I'll push when it comes back green.
Comment #110
yched CreditAttribution: yched commentedDo not use truncated table names in views_data entries.
Comment #112
chx CreditAttribution: chx commentedyched what else is left, i saw the green in #172 in the parent
Comment #113
yched CreditAttribution: yched commentedTesting reroll this time...
Comment #115
yched CreditAttribution: yched commentedMissed one change.
Comment #116
yched CreditAttribution: yched commentedReroll
Comment #117
yched CreditAttribution: yched commentedas per the end of #1497374-212: Switch from Field-based storage to Entity-based storage:
- call fieldOp() methods explicitly instead of within invokeHook()
- merge fieldUpdate() & fieldInsert() into fieldSave()
Comment #118
yched CreditAttribution: yched commentedReroll
Comment #120
yched CreditAttribution: yched commentedMethod renames
Comment #122
chx CreditAttribution: chx commentedComment #123
chx CreditAttribution: chx commentedComment #124
yched CreditAttribution: yched commentedMethod renames (same as #120) + minor adjustements + chx's reroll.
Comment #126
yched CreditAttribution: yched commentedRe-reroll...
Comment #128
yched CreditAttribution: yched commentedforum.module changes got reverted in a previous reroll...
Comment #130
yched CreditAttribution: yched commentedSigh... Pushed yet another reroll, but those fails beat me for today...
Comment #131
yched CreditAttribution: yched commentedOK, I think I got it.
Comment #132
chx CreditAttribution: chx commentedyched, congrats on getting it to green again but I think you deleted field_sql_storage.module which caused the patch to swell to 600kb. But I can't get it back down to 500kb now it seems. I have made an interdiff.
Comment #133
yched CreditAttribution: yched commented@chx: the patch I post in the helper issue is the "FULL" patch (with old hook docs & field_sql_storage.module deleted and its tests re-added).
The sandbox contains the "REVIEW" patch - except I forgot to push the fix from #131 yesterday night :-/. Done now.
Will post an updated patch in the main issue with a summary of the changes & interdiffs.
Comment #134
jibranYay!!!
Comment #135.0
(not verified) CreditAttribution: commentedUpdated issue summary.