-
Notifications
You must be signed in to change notification settings - Fork 118
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
Remove Feature-ness of Islandora Core Feature. #968
Conversation
I don't know if we can proceed. The error now is:
This seems to imply that we can't both install from existing config and load modules with their own config. |
We need to move the configs from |
Does that leave the "core feature" with a useful reason for existing? Noting that with the standard install profile and/or installing required modules like filehash, those configs will always already be installed so the version in the "core feature" - with our islandora stuff - will be ignored. The Starter Site installs versions of all these configs, so the core feature won't come into play there either. Maybe we should examine what's in this folder and trim out what's not useful in a standard install? |
If we ever want to let someone install islandora on an existing Drupal site (like I did for UNLV), that config needs to be installable on it's own. That said, we could add it directly to the islandora module's config, instead of using a separate "core" config module. |
If someone were installing on an existing drupal, they'd already have - so would skip - a big chunk of the config that we considered "core". This includes messing with existing media types. Would they (a) be informed that they're skipping some config as it already exists, and (b) have a recourse option to install the skipped configs if they so desire? |
I guess that depends on the definition of core; which admittedly is a conversation with a long and sometimes difficult history as I'm sure you recall. How much should a site be handed when they install the core module? This is why we broke out islandora_defaults from islandora oh so long ago. The config that remained in islandora_core_feature are there because we make code references to them. Field storage configs are a good example because we make several code references to 'field_member_of' but a site can choose to add the field it to their content type or not. We need these particular configs that are in core on every site or things blow up. Beyond that how to implement islandora on their site in terms of what content types will include islandora-provided fields, what taxonomy and terms they add, etcetera, becomes their choice. Good documentation of that, of course, is important. |
Another thought on this line:
I think this conflates "core" with "standard". Again, the division between islandora/islandora_core_feature and islandora_defaults (which is now supplanted with the starter site) represents this same division. The config in islandora_core_feature is what I consider "core" (those things code requires to exist) and the starter site (formerly islandora_defaults) I consider "standard" (what we expect most sites will want to build with it). |
Questions from the tech call:
Testing note: make sure this doesn't break an existing site. |
I can't get the PR's patch to apply to 2.x:
|
@seth-shaw-asu that was because we already changed our workflows to use the 3.x branch instead of the 2.x branch of the |
* Remove Feature-ness of Islandora Core Feature. * Remove features bundle config. * Remove from composer.json. * Remove dependency on Features UI. * Rename install dir to optional. * Update text_extraction_defaults and remove 'fim' from workflows.
GitHub Issue: #902 (comment)
Release pull requests, etc.)
What does this Pull Request do?
Removes the "Feature"-ness of islandora core feature.
What's new?
How should this be tested?
Does it install properly, and still install the needed configs?
Does it update okay on existing sites?
Documentation Status
Additional Notes:
Any additional information that you think would be helpful when reviewing this
PR.
Interested parties
@seth-shaw-asu