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

Update drupal #12

Merged
merged 4 commits into from
Oct 26, 2022
Merged

Update drupal #12

merged 4 commits into from
Oct 26, 2022

Conversation

rosiel
Copy link
Contributor

@rosiel rosiel commented Oct 11, 2022

What does this Pull Request do?

Security updates to Drupal and twig.

  • Related GitHub Issue: (link) n/a

  • Other Relevant Links: (Google Groups discussion, related pull requests,
    Release pull requests, etc.)

What's new?

  • updates all modules
  • removes the patches to FITS

what's not new?

  • a bunch of configs seem to have changed but it's just the ordering of the yaml.

How should this be tested?

Does it work/load drupal?

Documentation Status

  • Does this change existing behaviour that's currently documented? no
  • Does this change require new pages or sections of documentation? no
  • Who does this need to be documented for? no
  • Associated documentation pull request(s): ___ or documentation issue ___

Additional Notes:

Any additional information that you think would be helpful when reviewing this
PR.

Interested parties

Tag (@ mention) interested parties or, if unsure, @Islandora/committers

provider: openseadragon
label_display: '0'
Copy link
Member

Choose a reason for hiding this comment

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

I don't know where you can see these schemas (or if you can) but I noticed this label_display has a value of '0' (a string) and below (line 45 -> 49) it is label_display: visible. So does '0' mean "visible" or "hidden" or something else?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Strictly, this should be referring to https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/config/schema/core.data_types.schema.yml#L303-305 (via https://github.com/Islandora/openseadragon/blob/8f9eeff9d3be213dfa5e0477959df5882776cb44/config/schema/openseadragon.schema.yml#L15-L16)... we're dealing with the settings of a block here which should be defined as block_settings; however, I seem to recall digging in to things a bit in the semi-recent past, and the placing of blocks via contexts breaks the associations back to the underlying schema definitions... instead rolling their own schema which seems like copypasta of the core block_settings type with some additional context specific properties.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Digging deeper, the concrete value seems to come from the block plugin; for example, the trait associated with the BlockPluginInterface defines a checkbox for this property... When checked, would give the #return_value of BlockPluginInterface::BLOCK_LABEL_VISIBLE... when unchecked, 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So maybe some of these are just stale? I notice both this-here-referenced block config with label_display: 0 and the one below (lines 45-49 with label_display: visible) are both for the bartik theme. We haven't used that in ages! And we have documented the need to update all of them to use Olivero. Hopefully that will flush out any stale config language in these contexts.

Copy link
Member

@whikloj whikloj left a comment

Choose a reason for hiding this comment

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

I don't understand this enough to approve or disapprove. I had one comment (above) and I noticed that the islandora_fits patch is removed, was this change added elsewhere?

Other than that I can say that I reviewed all the files (except composer.lock, because I'm not crazy) and they all seem to just be re-ordering of elements.

There were some small changes (some I have lost) one example is in config/sync/search_api.index.default_solr_index.yml where a multilingual key is added and the roles was modified from an associative array to a regular array.

@rosiel
Copy link
Contributor Author

rosiel commented Oct 26, 2022

Yes, when I installed the security updates there were some database updates to do. I recall one of them - i think it was for search_api - was related to multilingual.

@seth-shaw-asu seth-shaw-asu self-requested a review October 26, 2022 17:15
@seth-shaw-asu
Copy link
Contributor

I did a git pull on isle-dc to get the latest updates and then modified my Makefile to use this branch:

diff --git a/Makefile b/Makefile
index 65251df..2afce8d 100644
--- a/Makefile
+++ b/Makefile
@@ -540,7 +540,7 @@ starter_dev: QUOTED_CURDIR = "$(CURDIR)"
 starter_dev: generate-secrets
        $(MAKE) starter-init ENVIRONMENT=starter_dev
        if [ -z "$$(ls -A $(QUOTED_CURDIR)/codebase)" ]; then \
-               docker container run --rm -v $(CURDIR)/codebase:/home/root $(REPOSITORY)/nginx:$(TAG) with-contenv bash -lc 'git clone -b main https://github.com/Islandora/islandora-starter-site /home/root;'; \
+               docker container run --rm -v $(CURDIR)/codebase:/home/root $(REPOSITORY)/nginx:$(TAG) with-contenv bash -lc 'git clone -b update-drupal https://github.com/rosiel/islandora-starter-site /home/root;'; \
        fi
        $(MAKE) set-files-owner SRC=$(CURDIR)/codebase ENVIRONMENT=starter_dev
        docker-compose up -d --remove-orphans

I then did make starter_dev and it spun everything up without error. The status page indicated we were using the newest Drupal version. 👍

However, a lot of things aren't working. First, the status page reported SOLR wasn't working (it was using 127.0.0.1 for the host, changing that to 'solr' fixed it). Also, derivatives aren't working either. I presume these issues are just starter site issues + isle in general and not necessarily a blocker for this PR, correct?

@rosiel
Copy link
Contributor Author

rosiel commented Oct 26, 2022

Hm. It's quite possible that starter_dev doesn't run the "isle-ify" (sorry!) code that updates all the external services to their ISLE locations. I was fairly certain that starter did that.

@seth-shaw-asu seth-shaw-asu merged commit e04d7c0 into Islandora-Devops:main Oct 26, 2022
@rosiel rosiel deleted the update-drupal branch October 27, 2022 21:28
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.

4 participants