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

Investigate need for file_public_base_url #4276

Open
wants to merge 14 commits into
base: 2.x
Choose a base branch
from

Conversation

paul-m
Copy link
Contributor

@paul-m paul-m commented Sep 5, 2024

Fixes [#4205]

NOTE: Do Not Merge Just Yet.

This PR requires a change to the DKAN DDev add-on. See GetDKAN/ddev-dkan#52

Describe your changes

  • Adds tests for ResourceImporter, kernel and functional.
  • Performs minor refactoring of ResourceImporter:
    • Short-circuits unneeded else statements.
    • Injects services explicitly.
  • Adds steps to SampleContentCommandsTest to prove that the Drush commands work without setting file_public_base_url.

Discoveries

  • Drupal\common\Util\DrupalFiles uses the deprecated system_retrieve_file() function, which goes away in Drupal 11.
  • DrupalFiles also does not handle file wrappers properly, leading to an inability to write a kernel test. (Kernel tests use in-memory vfsStream file wrappers.)

Follow-up recommendations:

  • Refactor DrupalFiles to play nice with vfsStream and also to not use system_retrieve_file().
  • Turn Drupal\Tests\harvest\Functional\Transform\ResourceImporterTest into a kernel test.
  • Investigate whether we actually need DrupalFiles, since it's only used in Drupal\harvest\Transform\ResourceImporter and ResourceLocalizer service.

QA Steps

  • Stand up a local build with the special settings config
  • Create sample content
  • Remove the config
  • Remove sample content
  • Create sample content again

Checklist before requesting review

If any of these are left unchecked, please provide an explanation

  • I have updated or added tests to cover my code
  • I have updated or added documentation

@paul-m paul-m marked this pull request as ready for review September 11, 2024 18:49
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.

1 participant