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.
Follow-up from #1588422-155: Convert contact categories to configuration system
Needs tests to make sure that contact categories has clean upgrade path to CMI
Comment | File | Size | Author |
---|---|---|---|
#25 | update_config_manifest_add-1817182-24.patch | 868 bytes | yched |
#18 | 1817182-contact-upgrade-18.patch | 6.72 KB | yched |
#12 | 10-12-interdiff.txt | 2.41 KB | alexpott |
#12 | 1817182-contact-upgrade-12.patch | 6.72 KB | alexpott |
#10 | 1817182-contact-upgrade-10.patch | 5.18 KB | alexpott |
Comments
Comment #1
catchComment #2
sunI discussed this to some length with @alexpott already, and I'd like to see "combined/cumulative" ConfigUpgradePathTest; i.e.:
./upgraded_config/
directory with actual config files and simply compare and assert that the final configuration after upgrading is identical to that directory.Comment #3
andypostProbably we need to call entity_create() in hook_update_N() to assign uuid automatically
Comment #4
andypostMaybe this one better, not sure it's ok to use entity_create here
Comment #5
andypostThere's a still open question about uuid assign:
- all entities are gets uuids with update_add_uuids() so should we assign uuid to config entities when upgrade?
- should we make a similar function for config entities or unify update_add_uuid()?
- what is proper way to assign - config->save() or
entity->save()? - last one executes hooksComment #7
andypost#4: 1817182-contact-upgrade-4.patch queued for re-testing.
Comment #9
andypost@sun please answer questions in #5
So we can't use
entity->save()
in hook_update_N()Glad to get review of #3
Comment #10
alexpottYep we can't use entity_create() in a hook_update_N.
#3 looks mostly good - I've rerolled and combined the tests so we don't have to run the upgrade twice. I've also added the langcode into the configuration entity as at the moment this is added to all entities.
Comment #11
alexpottHmmm... and we've also got to create a manifest file :(
Comment #12
alexpottPatch adds a generalised method for hook_update_N functions to create a manifest file, uses during contact category entity creation and tests it.
Comment #13
yched CreditAttribution: yched commentedGreat - existing "X to config entities" updates did not create manifest entries so far, and currently the one in #1852966: Rework entity display settings around EntityDisplay config entity doesn't either, so this is badly needed.
Minor nitpick :
- The function is not really about creating a manifest file, but inserting entries in it (creating the file if needed)
- We have similar "generic helper functions for special use in hook_update_N() where you're not supposed to use regular APIs" in core right now, and they're named _[system]_[7000|8000]_action()
_update_8000_field_sql_storage_write()
_update_7000_field_create_field()
_update_7000_node_get_types()
--> may I suggest _entity_8000_manifest_insert() ?
Side note: in #1802750-5: [Meta] Convert configurable data to ConfigEntity system I wondered whether there would be a way to simplify creation of UUIDs too. Like manifest files, it's really easy to forget (the upgrade for image styles still misses UUIDs), and you get a fully functional system that will only break when you try to import config.
Maybe we could discuss this over there ?
Comment #14
yched CreditAttribution: yched commentedAlso: it seems the function currently supports re-adding the same entry even if it already exists in the manifest ?
That is going to be very handy, because you don't really need to care whether the config file exists or is being created.
So IMO this behavior should be advertised as a feature in the phpdoc :-)
Comment #15
BerdirYour examples and then pattern doesn't seem to match? Shouldn't it be _update_8000_entity_manifest_insert()? And not sure about entity vs. config, the manifest file is arguably more related to config? It is implemented in the entity storage controller, but the entity system doesn't know/care about it.
Tests and update function looks great otherwise.
Comment #16
yched CreditAttribution: yched commented@Berdir: er, yes, sorry, #13 is pretty mixed up :-/.
Right, something like _update_8000_config_manifest_insert() would be good.
Comment #17
yched CreditAttribution: yched commentedWell, I was about to roll a patch to rename the function, but I bumped into the helpers that currently exist in this "variable / config / state" area:
update_variable_get|set|del() (operate on {variables}, probably bound to disappear at some point)
update_variables_to_config()
update_variables_to_state()
update_add_uuids()
The update_config_manifest_create() function added in this patch does fit consistently there.
So it seems we do have different patterns :
- _update_[N]000_*() for update-safe versions of API functions
- update_*() for general helpers
Not sure this is ideal, given that the separation is not really clear cut, and that the latter functions then sit next to internal update mechanisms like update_build_dependency_graph(), update_is_missing()...
The conceptual difference between "helper funcs for update hooks" & "internal update.php mechanisms" is much more relevant IMO,
But at least the function as it is named right now is somewhat consistent with the current update_variables_to_config() & friends, so there's no reason to block this specific patch...
Comment #18
yched CreditAttribution: yched commentedJust slightly renamed the function and adjusted the phpdoc - it's not only used to create manifest files, but also to add entries in an already existing manifest.
Don't credit me.
Comment #19
yched CreditAttribution: yched commentedOpened #1871270: Standards for hook_update_N helpers for the naming discussion.
Comment #21
yched CreditAttribution: yched commented#18: 1817182-contact-upgrade-18.patch queued for re-testing.
Comment #22
andypostI think "add" is better then "create" as function actually adds a manifest for existing config
Comment #23
webchickCommitted and pushed to 8.x. Thanks!
Comment #24
yched CreditAttribution: yched commentedOuch, actually the newly added update_config_manifest_add() helper doesn't work if the $ids of the config entities contain dots.
$config->set($name, $value) treats dots in $name as nesting indications, i.e $config->set('my.id', $value) results in $config['my']['id'] = $value.
ConfigStorageController::save() saves manifests by doing :
(the
if (!in_array())
check seems weird there, but the point is the data is written as a whole by setData())Comment #25
yched CreditAttribution: yched commentedLeft a comment about the
if (!in_array())
check in #1697256-152: Create a UI for importing new configuration.Meanwhile, here's a fix for update_config_manifest_add().
Comment #26
plachMakes sense to me.
Comment #27
Dries CreditAttribution: Dries commentedCommitted to 8.x.