The trusted data concept in the Configuration system is a code smell. It was introduced to improve the situation during site installation where the schema cache is rebuilt time and time again - see #2432791-53: Skip Config::save schema validation of config data for trusted data but it causes issues like #3346953: Role permissions are not sorted when saving via admin/people/permissions because it removed sorting in the role entity and adding it to the schema in #3039499: Role permissions not sorted in config export
If we add sorting to many configuration schemas if we have the trusted data concept we'll need leave code like the code in \Drupal\user\Entity\Role::preSave() to sort when it's trusted. That makes the system more fragile than it should be.
None
Calling \Drupal\Core\Config\Config::save() with the $trusted_data is deprecated
Calling implementations of \Drupal\Core\Config\Entity\ConfigEntityInterface::trustData() is deprecated
None
Show commandsStart 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:
changes
Comments
Comment #4
wim leersTrusting data == not validating it, so there's a definite connection to all the config validation work here too 🥸
Comment #5
wim leersOhhh! I've got some good news … while working on another issue (#3230826: Expose API to sort arbitrary config arrays: TypedConfigManager::getCanonicalRepresentation() should override its parent), I found blocking bugs (the validation logic for primitives does not match the config schema checking logic).
So I was forced to write test coverage. And I extracted that into a blocking, test-coverage-only issue. While working on that, I made the whole
Config::save(has_trusted_data: TRUE)thing that this issue aims to deprecate tested separately, to make it easy to extract in the future 👍Please review/help land #3421993: PrimitiveTypeConstraintValidator vs ConfigSchemaChecker: test coverage to establish the status quo, it will help reduce the BC break risk for this issue! 😊
(Thanks @bircher for pointing me to this issue!)
Comment #6
andypostAs I get on new API
Comment #8
andypostThe only question is how to deal with upgrade tests, when we must import old config
Comment #12
borisson_Looking at the test failures, there seem to be a few places left where we are still passing in the $has_trusted_data argument, such as in
Drupal\Tests\jsonapi\Functional\JsonApiRelationship.It looks like this has also found some more places where the schema does not match the data, such as
Drupal\Tests\search\Functional\Update\SearchBlockPageIdUpdatePath::testRunUpdates, is this because the test fixture contains the wrong data-type? Should we update the fixture?Comment #13
andypostI see no reason is to postpone it on not invented api yet, it looks like fixes for test are required and then #5 will be easier to fix
Comment #14
alexpottFWIW locally when installing the Standard profile this change makes next to no difference as multi-module install has made the optimisations due to trusting data less important.
Comment #15
alexpottComment #16
bircherThis is very interesting. We talked about doing that during more than a few Drupal events.
I am slightly surprised by how little changes are necessary to make it work. (I mean there are obviously a lot of changes but most of them just remove the deprecated method or argument) But I expected way more impact like in
Another interesting aspect is that the performance tests had their QueryCount and CacheSetCount reduced. I don't have the time right now unfortunately to dig into the performance impact.
And finally it is great that it leads to some re-ordering in the test config.
There are a couple of todos, but I also don't know at the moment what is best there!
I left a comment on a comment that I stumbled on and it took me a while to understand how the comment would not just address the reviewer of the MR but the reader of the changed code.
Comment #17
catchI asked @alexpott about this, it's because of the change in views data to not load all the actions for an entity and pick one. Because that was the only code loading actions on most pages (e.g. pages that don't have any actions on them otherwise), that work just disappears. So it's an indirect result of the MR but still good to drop some useless work on nearly all Drupal sites after every cache clear.
Comment #18
alexpott@bircher the performance test changes are due to views data requiring less queries on a cold cache - see https://git.drupalcode.org/project/drupal/-/merge_requests/6928/diffs#24... - we no longer are loading all the action configuration entities as the bulk form view field plugin can not be broken when we save a view which in HEAD it is prior to any actions existing for media entities.
Comment #19
penyaskito+1000 to this, one concern though:
I think I've used this to create invalid config for the previous state of upgrade tests that actually, well, fix config schema. Probably not in core.
I know it's not the best way to do that (a direct db
insert()should be preferred, buttrustData()is better for legibility). So this wouldn't be an option anymore?Comment #20
bircherRE #19
you can just use the new test trait added in this MR. Should we add a CR for that?
Comment #21
alexpott@penyaskito @bircher Yeah we need to add the test trait to the CR... I think I wrote the CR before I added the trait. The CR covers how to update config without schema in hook_update_N functions already.
Comment #22
alexpottI updated the CR without info about the trait for tests.
Comment #23
borisson_I think the CR looks great with these additional changes.
I've re-read the merge request a couple of times now, and I can't find anything I'd ask for more information about or that needs to be changed.
Comment #24
borisson_All the remarks I had are now resolved. I don't know what else I can ask for before setting this to rtbc.
Comment #25
alexpottComment #26
alexpottNote this is undoing some of #2432791: Skip Config::save schema validation of config data for trusted data. That issue addressed three separate performance concerns to do with schema:
1. Slow site install
2. Slow configuration import
3. Slow ConfigEntityBase::toArray() causes issues on cold cache node/1
This change does not rollback anything to do with the fixes for cold cache node/1 performance - to the toArray() optimisations are still in place.
Point 1 is now mitigated a bit by multi-module install meaning less schema rebuilds during a site install. Note we still have to apply schema to all the config being installed. Point 2 has no mitigation. @donquixote has proposed a possible mitigation for both points 1 and 2. We could sign the configuration with the schema version used when writing the config. This feels quite complex. Another proposal would be for the schema service to become config sync aware and to allow us to not use schema during config sync. However in HEAD we're using config schema during import as the original issue didn't actually address point 2. So I feel that the multi-module install mitigation is enough to allow us to proceed with this change.
Comment #27
catchOne question on the MR, also needs a rebase.
Comment #28
alexpottDiscussed the hasTrustData() deprecation with @catch. We decided to hold of on deprecating that till Drupal 13 and once the ability to trustData has been removed to make it easy for contrib to upgrade and only worry about calls to trustData() in Drupal 11.4 and up... and then the 3 modules that use hasTrustedData() can remove those calls in Drupal 13 when that method will only ever return FALSE.
Given this is just removing a deprecation returning to RTBC and updating issue summary and change record.
Comment #31
catchWas a bit concerned we might see an increase in MR pipeline times, but the past couple of runs here are in the range of what we're seeing at the moment (which is extremely variable).
Apart from that un-deprecation (and some temporary confusion about how this would or wouldn't impact config sync) I don't have anything left here.
Committed/pushed to main, thanks!
Will need a rebase for 11.x due to performance tests.
Comment #32
andypostThank you! As igbinary coming to core it opens up door to #1977206: Default serialization of ConfigEntities
Comment #34
andypostbackported https://git.drupalcode.org/project/drupal/-/merge_requests/15048
Comment #35
alexpottThe 11.x branch is looking good - thanks @andypost
Comment #36
catchCommitted/pushed to 11.x, thanks!
Comment #40
donquixote commentedNice to see this merged!
One thing to keep in mind:
Even if a configuration was "normalized" at the time it was saved, it can still become denormalized over time:
- For regular config, when the schema changes (e.g. new options introduced)
- For config entities with relationships, like entity view displays, when other config entities are created that cause noise, e.g. new fields.
Doing the normalization on save does not fully prevent that additional noise, but it does improve the situation a lot.