Problem/Motivation
In #2482295: Rebuilding field map with many bundles/fields is very slow, the bundle field map was moved from being generated on demand and cached to being saved in the KV store. I've encountered a number of projects where this has gone out of sync.
When this happens it can be really hard to track down, since there aren't any immediate adverse effects you can notice the issue long after it was caused. However it can still cause errors and create noisy log entries that emanate from views.
Proposed resolution
The map used to be generated on demand, so at least provide some method of programatically rebuilding the map, so when troubleshooting issues that are caused by an out of sync map, there is an easier way to diagnose and fix the problem.
Remaining tasks
Address #34
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#39 | rebuild_field_map-3129179-39.patch | 11.13 KB | pmagunia |
#36 | 3129179-nr-bot.txt | 2.38 KB | needs-review-queue-bot |
Issue fork drupal-3129179
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI have been using the following snippet, should be easy enough to move into a function or service:
Comment #5
GrimreaperHello,
@Sam152 thanks for the code snippet, it saved my day!
Comment #6
loze CreditAttribution: loze commentedThis has saved me multiple times as well.
I think this should be a link in the devel module or maybe admin_toolbar_tools.
Comment #7
loze CreditAttribution: loze commentedI've submitted a patch using this snippet to admin_toolbar #3190279: Provide a link to rebuild the persistent bundle field map
Comment #8
GrimreaperThanks @loze for the input and the patch done on admin_toolbar!
Comment #9
AndyF CreditAttribution: AndyF at Fabb for FRUITION, Sport Obermeyer commented#2 saved my skin too, thanks @Sam152!
While I think it's great to be able to conveniently trigger it, I wonder if the actual functionality shouldn't live in core?
Thanks!
Comment #10
loze CreditAttribution: loze commented@AndyF - I agree it should live in core, but it will take literally years for that to happen. ;)
So I summitted it to admin_toolbar in hopes that getting it in there would be quicker.
Comment #11
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI'd like it to be in core, so that it can be tested and maintained as changes in the field system occur. I don't think it needs any kind of integration with the UI, just being there as a public API for other modules like admin toolbar to call off to would be an improvement imo.
Comment #12
jacktonkin CreditAttribution: jacktonkin at ISSUP commentedThe code in #2 will also add base fields to the map, so should probably filter these out. Not sure if
$field_definition->getFieldStorageDefinition()->isBaseField()
is sufficient for this.Comment #14
hideaway CreditAttribution: hideaway at jobiqo - job board technology commentedCan confirm that code needs an improvement from #12 as jacktonkin wrote.
Also the bundle list is not a simple array, it should contain bundle also as a key:
$map[$entity_type_id][$field_name]['bundles'][$bundle] = $bundle;
So the looping part should be:
Last problem is that if you already used original code from #2 you have stored map items for entities which shouldn't be there - the entities which don't have any configurable field. You'll have map for
file
,shortcut
,path_alias
,redirect
, and others. I highly suggest to one time delete the former map completely before you set a new one:Comment #17
scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commentedThanks, the comments were all very helpful. I thought I'd take a stab at moving this a step further. EntityFieldManager seemed an an appropriate place, but happy to move it if not. I've also referenced it from the config-import notice that alerted me to the issue in my case.
https://git.drupalcode.org/project/drupal/-/merge_requests/1434
I wasn't sure how to go about testing this. Perhaps setting something out of sync programmatically, triggering a rebuild, and testing that the map is now back in sync?
Comment #19
romainj CreditAttribution: romainj as a volunteer commentedI think that kind of snippet could be part of the Devel module but not the Admin Toolbar module.
Comment #20
scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commentedI think a big part of the benefit of having this in core is that the error message can give a more helpful hint as to what might be going wrong and how to solve it (as the merge request code does). The hardest part of hitting this error is actually tracking down what has gone wrong as the issue summary notes.
I suppose an alternative is to just have this cover changes to the error message, pointing in the direction of a Devel option; ie a compromise of the two.
Comment #21
AnybodyComment #22
romainj CreditAttribution: romainj as a volunteer commentedComment #23
romainj CreditAttribution: romainj as a volunteer commentedComment #24
AndyF CreditAttribution: AndyF at Fabb for Sport Obermeyer commentedThe MR looks functionally good to me and seems to work, thanks! @romainj can I just confirm that it's Needs work for tests only?
Does it make sense (in a follow-up perhaps) to add a
hook_requirements()
check that your current field map matches the expected one?I've just attached a patch using the MR diff from
6b08a1819216019fe00abb6dbd172895bc6923b7
for Composer.Thanks!
Comment #25
AndyF CreditAttribution: AndyF at Fabb for Sport Obermeyer commentedJust wondering, is there any reason we shouldn't clear out the map completely on every rebuild?
Comment #27
steinmb CreditAttribution: steinmb as a volunteer commentedComing from #3125827: Prevent leftover fields from causing errors like "Call to a member function getLabel() after enabling layout_builder" - Tested with a D9.43 site that broke during field migration from D7. It seemed to find a broken leftover from Scald installation. D7 no longer have Scald installed however it seem to have left orphan field behind.
Applied path and tried to trigger a rebuild but the problem still persist. Let me know if I used it right?
Here is a full run down of how to break a D9 site during migration:
Also tried to rollback the field migration and then rebuild with
rebuildBundleFieldMap()
but the problem still persist.Comment #28
AndyF CreditAttribution: AndyF at Fabb commentedAs @hideaway mentions in #14, while the above will rebuild the bundle field map for those entity types that should have an entry, it won't clear entries for entity types that shouldn't be in there in the first place. So to be really confident I think you'll want:
And I'm thinking that should probably be built into the rebuild command personally... does anyone see an issue with that?
Comment #29
AndyF CreditAttribution: AndyF at Fabb commentedI've just rebased against 9.5.x. I don't think I have the rights to change the MR target branch tho... @scott_euser could you possibly change that?
Thanks!
Comment #30
AndyF CreditAttribution: AndyF at Fabb commentedI've added a simple test: @scott_euser if you're able to change the target branch, could you also switch the issue to Needs Review> to trigger testbot?
Thanks!
Comment #31
AndyF CreditAttribution: AndyF at Fabb commentedComment #32
scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commentedHi AndyF,
Thank you for your work on this, target branch updated.
Thanks!
Scott
Comment #33
AndyF CreditAttribution: AndyF at Fabb commentedCheers @scott_euser, looks good!
I've just made a minor update to avoid a deprecation in the test, fingers crossed.
If that does the job, I think I'm gonna be bold and update the method to completely clear out the bundle field map (rather than just overwriting existing data), and slightly modify the test to verify that.
Thanks!
Comment #34
AndyF CreditAttribution: AndyF at Fabb commentedOK, so I've updated it now to clear out the bundle field map before rebuilding it. (So #28 is no longer accurate.)
Regarding the test, there were a couple of things I was uncertain about:
Thanks!
Comment #36
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #39
pmaguniaAdding a file patch in case anyone want to link. Identical to #38
Comment #40
pmaguniaComment #41
pmaguniaIf someone wanted to use the code in the patch:
Comment #42
smustgrave CreditAttribution: smustgrave at Mobomo commentedSeems to be an open question in #34 could that be answered?
Comment #43
joachim CreditAttribution: joachim as a volunteer commented> Should I assert against an explicit expected bundle field map, or just test that it's the same before and after a rebuild. Normally I like to be really explicit in tests, but in this case I thought that would introduce additional brittleness, so I just test it's the same before and after.
I think testing it's the same before and after is potentially good, in that as you say it keeps the test simple.
And I see that the test is deleting the map -- so the intention is that we're checking that the rebuild actually can rebuild from scratch. However, we're not checking that after the deletion the map is empty!
> How complex a bundle field map should I be making for the test? I've kept it simple for the time being.
What are the different cases we should consider?
Comment #44
joachim CreditAttribution: joachim at Factorial GmbH commentedAlso,
> public function rebuildBundleFieldMap() {
The other methods that work with the field map call it just 'field map', so the use of 'bundle' in this method name is confusing.Scratch that, the field map methods show base fields as well.
Comment #45
joachim CreditAttribution: joachim at Factorial GmbH commentedrebuildBundleFieldMap() should finish off by clearing the 'entity_field_map cache item, as otherwise it won't have any apparent effect.