Drupal 9 readiness

There are many more errors then the one in the IS.

Attached the scan of version 2.2. I checked and some of them exists on 3.x branch too, but I don't have the branch checked out with me at the moment so I can't scan it and compare.

Moved to another issue I meant to reply on

This issue still needs work as per comments on #3.
Also re-publish the .txt file with latest deprecations report.

Made additional fixes.

Edit: After composer require --dev drupal/core-dev:^8 and rerunning drupal-check it reports no errors. The same applies when I scan the module with the Upgrade Status module.

1. +++ b/bynder.install
   @@ -45,8 +46,8 @@ function bynder_requirements($phase) {
      $source = drupal_get_path('module', 'bynder') . '/images/icons';
      $destination = \Drupal::config('media.settings')->get('icon_base_uri');
   -  file_prepare_directory($destination, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);
   -  $files = file_scan_directory($source, '/.*\.(svg|png|jpg|jpeg|gif)$/');
   +  \Drupal::service('file_system')->prepareDirectory($destination, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS);
   +  $files = \Drupal::service('file_system')->scanDirectory($source, '/.*\.(svg|png|jpg|jpeg|gif)$/');
      foreach ($files as $file) {
   

   these file system methods require at least 8.7

2. +++ b/src/Exception/BynderException.php
   @@ -54,11 +55,13 @@ abstract class BynderException extends \Exception {
        $log_message = NULL,
   -    $log_message_args = []
   +    $log_message_args = [],
   +    MessengerInterface $messenger
   

   this is an exception class, not a service. would be quite awkward to update the code everywhere to pass this in.

   It's also at the end of optional arguments

   Let's just use the MessengerTrait here and als in several other classes that need it.

3. +++ b/src/Plugin/EntityBrowser/Widget/BynderUpload.php
   @@ -378,7 +402,7 @@ class BynderUpload extends BynderWidgetBase {
          else {
   -        drupal_set_message(t("Your file was uploaded to Bynder but needs to be approved before you can use it. Please go to your Bynder waiting room and review the uploaded assets."), 'warning');
   +        $this->messenger->addWarning(t("Your file was uploaded to Bynder but needs to be approved before you can use it. Please go to your Bynder waiting room and review the uploaded assets."));
          }
   

   switch to $this->t() when you touch it, make sure we include the StringTranslationTrait if it's not already there from base classes (should be for plugins)

Working on it will update by today EOD.

Fixed below as per suggestion in #14

1. Used core_version_requirement: ^8.7 || ^9 in *.info.yml files.
2. use the MessengerTrait in BynderException.php file
3. Replaced t() with $this->t()

1. +++ b/bynder.info.yml
   @@ -5,8 +5,7 @@
      - drupal:media (>= 8.4.0)
      - ctools:ctools
      - entity_browser:entity_browser (>= 8.x-2.x)
   -  - jquery_ui_tooltip:jquery_ui_tooltip
   reverted:
   
   +++ a/bynder.libraries.yml
   @@ -52,4 +52,4 @@
        - core/drupal
   +    - core/jquery.ui.tooltip
   -    - jquery_ui_tooltip/tooltip
   diff -u b/modules/bynder_select2/bynder_select2.info.yml b/modules/bynder_select2/bynder_select2.info.yml
   

   looks like this reverted the jquery_ui dependency, that's not correct.

2. +++ b/src/Exception/BynderException.php
   @@ -22,4 +22,11 @@
    
      /**
   +   * Message level to be used when displaying the message to the user.
   +   *
   +   * @var string
   +   */
   +  protected $messageLevel = 'error';
   +
   

   this was removed before, which is fine as it's not needed anymore. I can't see how anyone would rely on that.

Updates:

1. #14.1, #17.1 core_version_requirement is available since 8.7.7, set the requirement to that.
2. #14.2 Removed injected Messenger service. It uses MessengerTrait now.
3. #14.3 This was a change in the static function, so we cannot use the instance. As a result, we can remove injected file system and messenger services.
4. #18.1 Fixed.
5. #18.2 Fixed.

Regarding the tooltip change there is a separate issue #3151012: Make the jquery_ui_tooltip contrib module dependency optional. I left a comment about the optional dependency #3151012-comment-3.

+++ b/bynder.info.yml
@@ -2,10 +2,11 @@ name: Bynder
   - drupal:media (>= 8.4.0)
   - ctools:ctools
   - entity_browser:entity_browser (>= 8.x-2.x)
+  - jquery_ui_tooltip:jquery_ui_tooltip

lets keep this as a strict dependency for now. We can use the other issue to make it optional.

Means we also need a composer.json entry for this and an update function to enable the module if it's present.

Thanks for review I will address the above changes in next 6-7 hours

Updates:

1. 18.1 Revert back the changes
2. 18.2 Removed protected $messageLevel = 'error'; from /src/Exception/BynderException.php
3. 19.1 Updated core_version_requirement to core_version_requirement: ^8.7.7 || ^9
4. 19.3 Removed injected file system and messenger services

Updates: fixed remaining usages of $messageLevel from #23, added match_limit key in the form displays, added $defaultTheme property in the browser tests, small fixes...

Tests need to be fixed but that is out of scope here.

Checked tests in more details.

All tests pass now except Drupal\Tests\bynder\Functional\UsageTest which requires entity_usage:1.x to pass.

Committed this.

Here is the patch that makes Bynder 2.x branch Drupal 9 compatible.

All the tests are passing locally.

1. +++ b/bynder.install
   @@ -198,3 +200,14 @@ function bynder_update_8009() {
   +/**
   + * Enables the jquery_ui_tooltip module.
   + */
   +function bynder_update_8010() {
   +  /** @var \Drupal\Core\Extension\ModuleExtensionList $module_list */
   +  $module_list = \Drupal::service('extension.list.module');
   +  if ($module_list->exists('jquery_ui_tooltip')) {
   +    \Drupal::service('module_installer')->install(['jquery_ui_tooltip']);
   +  }
   

   Ah, I forgot one thing about the update function in 3.x, we need to avoid conflicts between the branches. I'll update that to 8300. It won't hurt if it runs twice and in this case, it doesn't matter, but future updates might then be skipped if people update later from 2.x to 3.x.

2. +++ b/config/optional/core.entity_form_display.media.bynder.default.yml
   @@ -48,6 +48,7 @@ content:
          size: 60
          placeholder: ''
   +      match_limit: 10
        region: content
   

   another thing I missed. match_limit actually requires 8.8, tests at least would fail on 8.7.

3. +++ b/modules/bynder_select2/bynder_select2.install
   @@ -8,8 +8,8 @@ function bynder_select2_requirements($phase) {
    
   -  if (function_exists('libraries_get_path')) {
   -    $path = libraries_get_path('select2') . '/dist/js/select2.full.min.js';
   +  if (\Drupal::hasService('library.libraries_directory_file_finder')) {
   +    $path = \Drupal::service('library.libraries_directory_file_finder')->find('select2/dist/js/select2.full.min.js');
      }
   

   another thing I missed.

   usually it's better to keep the libraries.module integration to not break it on 8.9, but, as I see, this never implemented the necessary hook_library_info() alter, because the lookup doesn't just magically work then, unlike the new D8.9 feature.

   Given that, I'm fine to remove this, but the new method can return FALSE, so I extended the is_file() check below to do $path && file_exists($path) to avoid a file_exists(FALSE) check.

I pushed the version and library check directly to 8.x-3.x and cherry-picked it into the 8.x-2.x commit.

Berdir, your last commit results in a bad yaml configs, see: https://git.drupalcode.org/project/bynder/-/commit/c2d670eb6845dc1bf4843...

@akucherov: Ooops, nice catch! I've pushed a fix to 8.x-2.x a while ago and now also to 8.x-3.x as I didn't realize the same mistake was there as well.