-
Notifications
You must be signed in to change notification settings - Fork 30
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
Issue 1561: Remove dependency on Name module #54
Conversation
@mjordan, can haz review? |
Sorry, will do by end of this week. |
Sorry I missed this the first time around! :D Thanks so much for doing this Seth! If I have time I will try to test it out, but I can't make any promises. |
@seth-shaw-unlv when I import the controlled access terms defaults feature, I get a WSOD with this in my apache error.log:
Any suggestions? |
Strange. I'll see if I can take a look at it today. |
@mjordan, okay, I see what is happening here... Features isn't intelligent enough to distinguish the optional v. install directory. So, if you simply So, when you go to features, explicitly omit the person name fields and field storage configs. Although, I guess the best way to test is to build a fresh install targeting this branch.... |
We can either do that by moving the branch to the main repo and update an inventory variable OR take a local copy of the playbook and hack out the controlled access terms so it doesn't get installed during the build add it manually afterward... |
I've moved a copy of the branch to the Islandora repo and I've kicked off a fresh build; however, now that I think about it, I think I would ALSO need to make a testing branch of Islandora Defaults and update it's composer requirement of controlled access terms to avoid a composer version conflict. 🤦♂️ Maybe I'll just kill the post-install step to resolve this manually... |
Oh, bother, it isn't as simple as that, either. I need to carve out the modules enabled by the install drupal step as well. |
Just thought of something - will changes aroud this require an update hook in the .install file? |
No. This only impacts new installs. Existing sites will not see any changes. We would only need an update hook if we were replacing the primary and alternate name fields with something else and had to update existing data. I went ahead and created a testing PR on islandora_defaults. This means all you need to do is build a fresh playbook (not using the pre-built VM) with the composer require as "islandora/islandora_defaults:dev-issue-1561" (see inventory/vagrant/group_vars/webserver/drupal.yml circa line 22). This actually surfaced an issue with one of the configs where field_permissions had snuck in. I've restarted my build. |
@mjordan , new testing instructions:
|
I think I'm doing something wrong. Using ubuntu as the
|
@mjordan, you dropped the "dev-" in the composer line. "islandora/islandora_defaults:dev-issue-1561" |
I just checked Travis. None of these messages are related to this particular PR. |
Rebuilding now. |
@seth-shaw-unlv fixed my ansible variable, all ran OK until I hit this: Here's the entire stderr output, for context:
Looks like the islandora_search configs need to be updated?
|
I think you are right... give me a minute. |
@mjordan, we are green now. I've also made a PR for islandora_defaults to fix the islandora_search. Give it a try again. |
Do I need to do anything to pull that in or just rebuild? |
@mjordan, just vagrant destroy and rebuild. |
Rebuilt using those updates, getting this:
|
@mjordan , that is related to the islandora_defaults PR. I think I have a solution for that... but the final bit will involve a PR to islandora_playbook to stop trying to feature import the controlled_access_terms_defaults config (which is unnecessary). |
@mjordan, sorry this has been such a pain to test! I think we may finally have it though... I have a new islandora_playbook PR that simply stops attempting to feature import controlled_access_terms_defaults (but also targets these testing branches). Once everything is approved, I can remove the edits to the islandora_playbook and islandora_defaults PRs targeting these issue branches before merging. Then the merge order is islandora_playbook→islandora_defaults→controlled_access_terms. |
No problem. Am testing Islandora-Devops/islandora-playbook#190 now. |
Build incorporating Islandora-Devops/islandora-playbook#190 successful: But within the Drupal instance,
|
@mjordan, I had already updated that branch to no longer point at the new controlled access terms branch when you stated that build. |
Writing from my phone while I wait for the toddler to drop into a deeper sleep before I can put her down. I can chat about it on slack when I get back to the computer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good to me (see #54 (comment)). @rosiel you're listed as a reviewer, you OK with this?
@mjordan, we need Islandora/islandora_defaults#36 approved and merged first. If you are happy with it I can remove the reference to this PR for it to be merged. Then we can merge this one. |
Islandora/islandora_defaults#36 tested and merged. |
@mjordan , go ahead and squash/merge this one. It was islandora_defaults that we needed updating before merging. I've created a new PR to fix it: Islandora/islandora_defaults#37. |
@seth-shaw-unlv anything else we need to do? Thanks for this, it was pretty complex (to me anyway)! |
@mjordan, nope! All done. When I started I didn't realize how complex testing it thoroughly would be. It is good to put this one to rest. |
I'll close the issue. |
Not 100% sure if this is related but I'm now seeing this on fresh installs after attempting to enable the feature. > drush fim --no-interaction --yes controlled_access_terms_defaults
In FieldStorageConfigStorage.php line 174:
Unable to determine class for field type 'name' found in the 'field.storage.0' configuration
In DiscoveryTrait.php line 53:
The "name" plugin does not exist. Valid plugin IDs for Drupal\Core\Field\FieldTypePluginManager are: comment, typed_rela
tion, edtf, authority_link, datetime, file, file_uri, geolocation, image, link, list_float, list_string, list_integer, p
ath, text_long, text_with_summary, text, decimal, entity_reference, string, uuid, boolean, map, changed, created, passwo
rd, float, language, string_long, integer, email, uri, timestamp
|
I've read through the issue history, the module and some more background information. I now understand that it must be enabled through the Features
@seth-shaw-unlv, and @mjordan, could this instead be split into two separate features? I ask because I have an automated process by which I create sites, where no manual steps are used. I'm willing to make those changes and tests them if a pull request would be accepted. |
@nigelgbanks, doing a feature import isn't necessary for using the module. I don't recall why we started doing the feature import for islandora_defaults as part of the install process, but simply installing the module is all you need to do for start up. The feature import is only a convenience for updating a feature after making local changes to the module's configurations or testing a PR. |
Thanks @seth-shaw-unlv that makes sense. |
GitHub Issue: Islandora/documentation#1561
Alternate to @rosiel's PR: #53
What does this Pull Request do?
Removes the dependency on the Name module (they will only see the alternate name string field on a fresh islandora install), but keeps the related fields around in case someone wants to install the module themselves. (Prevents breaking new ArchivesSpace Module installs which use these fields.)
What's new?
*. Moves the person preferred name and alternate name fields from install to optional.
How should this be tested?
http://localhost:8000/admin/modules/uninstall
) the Name module. (That is the primary requirement.)http://localhost:8000/admin/config/development/features/diff/controlled_access_terms_defaults
)Additional Comments
I figured it would be easier to just go ahead and make a fresh PR rather than making @rosiel slog through the manual edits again. Let me know if there is anything I missed.
Interested parties
@Islandora/8-x-committers, especially @rosiel.