search

Islandora / controlled_access_terms

Drupal module for subject and agents
GNU General Public License v2.0
7 stars 30 forks source link

Drupal 9 #58

Closed seth-shaw-unlv closed 3 years ago

seth-shaw-unlv commented 3 years ago

GitHub Issue: Resolves https://github.com/Islandora/documentation/issues/1679

What does this Pull Request do?

Drupal 9 readiness.

What's new?

How should this be tested?

Interested parties

@Islandora/8-x-committers, esp @kayakr

seth-shaw-unlv commented 3 years ago

Any takers, @Islandora/8-x-committers?

elizoller commented 3 years ago

i can give it a whirl if you'd like - anything in particular i should look for?

seth-shaw-unlv commented 3 years ago

Nope. It should be ready to go. I just need a Drupal 9 compatible release so I can get my ArchivesSpace module updated too.

seth-shaw-unlv commented 3 years ago

Although a smoke test never hurts!

elizoller commented 3 years ago

Upgrade status is returning 2 warnings:

Controlled Access Terms, --
Scanned on Mon, 11/16/2020 - 15:42

FILE:
web/modules/contrib/controlled_access_terms/tests/src/Kernel/EdtfUtilsTest.php

STATUS         LINE                           MESSAGE
--------------------------------------------------------------------------------
Check manually      Class PHPUnit\Framework\TestCase not found.
--------------------------------------------------------------------------------
Check manually 15   Class PHPUnit\Framework\TestCase not found.
--------------------------------------------------------------------------------
seth-shaw-unlv commented 3 years ago

Huh... I wonder how it missed that before. 😕

Thanks.... I'll see what I can do.

seth-shaw-unlv commented 3 years ago

@elizoller, do you have devel installed on the test box? See https://www.drupal.org/project/upgrade_status/issues/3137754.

kayakr commented 3 years ago

@seth-shaw-unlv The changes visible at Files changed make sense with respect to compatibility to Drupal 9 but I look at the composer patch I'd need to apply https://patch-diff.githubusercontent.com/raw/Islandora/controlled_access_terms/pull/58.patch it includes a bunch of other changes around controlled_access_terms_defaults config yml's and field changes. Am I driving composer wrong?

elizoller commented 3 years ago

i did drush pm-uninstall devel and re-ran drush us-a controlled_access_terms and got the same output - is that what you meant by having 'devel' installed?

seth-shaw-unlv commented 3 years ago

@kayakr, I believe your patch was based on an older commit. The master→main commit that contains a bunch of these changes was back in September.

seth-shaw-unlv commented 3 years ago

@elizoller, huh... no devel should be installed: drush en -y devel.

elizoller commented 3 years ago

devel was enabled before. did drush pm-enable devel and drush us-a controlled_access_terms and got the same error. i'll try another build.

kayakr commented 3 years ago

@kayakr, I believe your patch was based on an older commit. The master→main commit that contains a bunch of these changes was back in September.

https://patch-diff.githubusercontent.com/raw/Islandora/controlled_access_terms/pull/58.patch is a patch based on your current PR. What I'm seeing is the changes included in https://github.com/Islandora/controlled_access_terms/compare/8.x-1.x...seth-shaw-unlv:8.x-1.x - did you rebase your fork before making the Drupal 9 changes?

seth-shaw-unlv commented 3 years ago

@kayakr, oh! I see that now. I thought I had rebased my branch before I made the changes... and the github interface is only showing what I changed... but maybe I didn't rebase and Github is glossing over that.

dflitner commented 3 years ago

Controlled Access Terms, -- Scanned on Mon, 11/16/2020 - 16:03

No known issues found.

This is with the Islandora box from dev.

seth-shaw-unlv commented 3 years ago

Ah, I simply did a merge upstream (see https://github.com/Islandora/controlled_access_terms/pull/58/commits/720e64ec81d675822ad129dc320fa8669ad9a17c), instead of a rebase.

seth-shaw-unlv commented 3 years ago

Controlled Access Terms, -- Scanned on Mon, 11/16/2020 - 16:03

No known issues found.

This is with the Islandora box from dev.

🎉 At least it isn't just me. 😅

elizoller commented 3 years ago

then ignore me - debbie, review and merge?

seth-shaw-unlv commented 3 years ago

Ah, for future reference, @elizoller,

Adding drupal/core-dev

In order to be able to run Drupal core's PHPUnit test suite, you will have to install an additional metapackage, drupal/core-dev.

(See https://www.drupal.org/docs/develop/using-composer/starting-a-site-using-drupal-composer-project-templates#s-adding-drupalcore-dev.)

seth-shaw-unlv commented 3 years ago

Thanks, @dflitner!

elizoller commented 3 years ago

thank you @seth-shaw-unlv that did the trick and thanks @dflitner for swooping in with the merge