Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CTDA9-298: Add Islandora core feature dependency #4

Closed
wants to merge 4 commits into from

Conversation

bibliophileaxe
Copy link
Collaborator

No description provided.

Copy link
Contributor

@adam-vessey adam-vessey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rolling #5 instead.

@@ -23,4 +33,5 @@ function islandora_hocr_install() {
'description' => 'hOCR Derivative Term',
'field_external_uri' => ['uri' => 'https://discoverygarden.ca/use#hocr'],
])->save();

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a somewhat round-about way of attempting to account for a config dependency before creating content. Like, I feel that this is expecting there to be eventually references in code to this particular term (via the URI or whatever), so we need to create it, but this should not be the case: The URI should just be used in config. Like: Derivative actions are config. Views/filters for finding content with this term when building views and the like, where ultimately, a failure to find it just means there would be no results, but is config. Whatever eventual indexing processor stuff, should be config.

Which is to say: In the general case, strongly requiring the installation of a term in a particular vocab at module installation in the system is probably not desirable. Having a migration(/some other process) which users can invoke, which can either fail explicitly or be rolled-back/retried/updated if the term was not fully successfully created. With a migration, we could make use of the islandora migration_group that we already expect to invoke at site installation... ultimately, this is the kind of thing that could be part of a site-specific configuration, which seems to be the approach that the IF looking to pursue...

Comment on lines +44 to +47
$bundle_name = 'taxonomy.vocabulary.' . $bundle;
$config_record = $config_directory->read($bundle_name);
$config_storage = \Drupal::service('config.storage');
$config_storage->write($bundle_name, $config_record);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the general case, not sure we can(/should try to) really assert that this taxonomy exists? Like: If the config didn't happen to exist in the sync directory, how would this behave? Do we want to have to know how this would behave?

Comment on lines +41 to +61
$config_directory = new FileStorage(Settings::get('config_sync_directory'));
foreach ($entitytypes_fields as $entitytype => $bundles) {
foreach ($bundles as $bundle => $fields) {
$bundle_name = 'taxonomy.vocabulary.' . $bundle;
$config_record = $config_directory->read($bundle_name);
$config_storage = \Drupal::service('config.storage');
$config_storage->write($bundle_name, $config_record);
foreach ($fields as $field) {
$field_storage_name = 'field.storage.' . $entitytype . '.' . $field;
$config_record = $config_directory->read($field_storage_name);
if (!FieldStorageConfig::loadByName($config_record['entity_type'], $config_record['field_name'])) {
FieldStorageConfig::create($config_record)->save();
}
$field_config_name = 'field.field.' . $entitytype . '.' . $bundle . '.' . $field;
$config_record = $config_directory->read($field_config_name);
if (!FieldConfig::loadByName($config_record['entity_type'], $config_record['bundle'], $config_record['field_name'])) {
FieldConfig::create($config_record)->save();
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure that we want to broach what is effectively dynamic config imports... like: Can see thing quickly getting overly complicated, if whatever configs in-turn define dependencies... like: Kind of gets into having to flood the graph, which could in the worst case result in a full import?

Otherwise, if we really wanted to, could possibly pursue something like the Config Importer and Tools module/project; however, yeah, still of the mind that strongly requiring the creation of this term at module installation is probably not desirable.

@adam-vessey
Copy link
Contributor

Superseded by #5

@adam-vessey adam-vessey closed this May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants