Goal: Remediate this module to prepare for D9.

Requirements:
1. Assign yourself the module.
2. Review the module for D9 readiness using status upgrade module or Drupal check
3. Note issues in the report
4. Remediate them and report back to this ticket
5. Resolve when all deprecations are completed

------ ---------------------------------------------------------------------------------------------------------------------------------------
Line modules/video_embed_media/modules/vem_migrate_oembed/tests/src/Functional/oEmbedUpdateTest.php
------ ---------------------------------------------------------------------------------------------------------------------------------------
Class Drupal\Tests\vem_embed_media\Functional\oEmbedUpdateTest was not found while trying to analyse it - autoloading is probably not
configured properly.
------ ---------------------------------------------------------------------------------------------------------------------------------------

------ ---------------------------------------------------------------------
Line modules/video_embed_media/video_embed_media.install
------ ---------------------------------------------------------------------
21 Call to deprecated function file_scan_directory():
in drupal:8.8.0 and is removed from drupal:9.0.0.
Use \Drupal\Core\File\FileSystemInterface::scanDirectory() instead.
------ ---------------------------------------------------------------------

CommentFileSizeAuthor
#30 3097287-30.patch6.68 KBphenaproxima
#28 3097287-28.patch6.5 KBphenaproxima
#27 3097287-27.patch6.57 KBphenaproxima
#25 3097287-25.patch6.02 KBphenaproxima
#23 3097287-23.patch5.32 KBphenaproxima
#22 3097287-22.patch3.38 KBphenaproxima
#20 3097287-20.patch1.75 KBjoshi.rohit100
#17 interdiff_16-17.txt627 bytessaurabh.tripathi.cs
#17 video_embed_field-3097287-17.patch1.96 KBsaurabh.tripathi.cs
#16 video_embed_field-3097287-16.patch1.95 KBsaurabh.tripathi.cs
#16 interdiff_13-16.txt974 bytessaurabh.tripathi.cs
#15 video_embed_field-3097287-15.patch2.22 KBsaurabh.tripathi.cs
#14 video_embed_field-3097287-13.patch1.96 KBsaurabh.tripathi.cs
#13 interdiff_11-13.txt1.04 KBsaurabh.tripathi.cs
#11 video_embed_field-prepare_for_d9-3097287-11.patch2.21 KBsaurabh.tripathi.cs
#11 interdiff_6-11.txt761 bytessaurabh.tripathi.cs
#9 Screenshot 2019-12-02 at 2.39.23 PM.png321.54 KBsaurabh.tripathi.cs
#7 video_embed_field-3097287-6.patch885 bytessaurabh.tripathi.cs
#5 video_embed_field-3097287-5.patch1016 bytessaurabh.tripathi.cs
#4 video_embed_field-3097287-4.patch1018 bytessaurabh.tripathi.cs
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

saurabh.tripathi.cs created an issue. See original summary.

saurabh.tripathi.cs’s picture

Issue summary: View changes
saurabh.tripathi.cs’s picture

Project: Lightning » Video Embed Field
Version: 8.x-3.x-dev » 8.x-2.x-dev
Issue summary: View changes
saurabh.tripathi.cs’s picture

Status: Active » Needs review
FileSize
1018 bytes

Updating patch for depreciation.: Call to deprecated function file_scan_directory():

for Class Drupal\Tests\vem_embed_media\Functional\oEmbedUpdateTest was not found while trying to analyse it - autoloading is probably not
configured properly.

this seems to be an autoload issue. Not able to fix it.

saurabh.tripathi.cs’s picture

fixing bug in patch.

Sharique’s picture

Use filesystem service instead, of interface.

$files = \Drupal\Core\File\FileSystemInterface::scanDirectory($icon_folder, '/.*\.(svg|png|jpg|jpeg|gif)$/');

saurabh.tripathi.cs’s picture

Thanks, @sharique uploading new patch as per #6

joshi.rohit100’s picture

Status: Needs review » Needs work

Patch looks fine and applied cleanly. However, tests are failing and seems tests are failing because of colorbox related.

The file specified by the given app root, relative path and file name (/var/www/html//colorbox.info.yml) do not exist. in /var/www/html/core/lib/Drupal/Core/Extension/Extension.php:67

@saurabh.tripathi.cs can u pls check if these failures are specific to patch (which to me looks not) by running tests with and without patch?

saurabh.tripathi.cs’s picture

@joshi.rohit100 ., Yes I am getting the same issue with or without patch.

joshi.rohit100’s picture

@saurabh.tripathi.cs - OK! Then test failing looks not related to the patch.

Please create a new issue for failing tests.

Please add "core_version_requirement" in info.yml and composer.json file.

saurabh.tripathi.cs’s picture

Have created an issue for failing tests : here
Adding patch for changes in info file and composer.json.

joshi.rohit100’s picture

Status: Needs review » Needs work

@saurabh.tripathi.cs earlier, there were only 5 tests failing but with latest patch, its 7 now. So something is not correct. Pls check.

Also,

+++ b/modules/video_embed_media/video_embed_media.install
@@ -18,7 +18,7 @@ function video_embed_media_install() {
+  $files = $file_system->scanDirectory($icon_folder, '/.*\.(svg|png|jpg|jpeg|gif)$/');

This is available only in 8.8 version and thus this module won't be usable for other drupal8 version.

Please do something like

if (method_exists()) {
  // then use new code
}
else {
  // use old one.
}
saurabh.tripathi.cs’s picture

FileSize
1.04 KB

All the test related issues are failing due to unmet test dependencies

test_dependencies:
  - media_entity:media_entity
  - media_entity_embeddable_video:media_entity_embeddable_video
  - colorbox:colorbox

Still looking into it, have uploaded patch for fix suggested in #12

saurabh.tripathi.cs’s picture

saurabh.tripathi.cs’s picture

Have edited root path for module, checking if this fixes the issue.

saurabh.tripathi.cs’s picture

Fixing coding standards. And reuploading patch with a relative path, as adding absolute path is causing the patch to not get applied.

saurabh.tripathi.cs’s picture

saurabh.tripathi.cs’s picture

@rohit These tests are failing in other issues as well
for example: https://www.drupal.org/project/video_embed_field/issues/3086712
And these are unrelated, and we can merge this patch.
\
Video_embed_field.Drupal\Tests\video_embed_field\Kernel\ConstraintTest
Video_embed_field.Drupal\Tests\video_embed_field\Kernel\FieldOutputTest
Video_embed_field.Drupal\Tests\video_embed_field\Kernel\FormatterDependenciesTest
Drupal\Tests\video_embed_field\Kernel\VideoEmbedIFrameTest
" Video_embed_wysiwyg.Drupal\Tests\video_embed_wysiwyg\Kernel\FilterTest"

Extra tests which are failing in this issue, but are also unrelated.

ideo_embed_field.Drupal\Tests\video_embed_field\FunctionalJavascript\ColorboxFormatterTest
"
Drupal\Tests\video_embed_field\Functional\FormatterConfigurationTest"

Have already created an issue for failing tests: here

joshi.rohit100’s picture

  1. +++ b/composer.json
    @@ -3,5 +3,8 @@
    +    "drupal/core": "^8.7|^9.0"
    

    this should be double pipe not single. See https://www.drupal.org/node/3070687

  2. +++ b/video_embed_field.info.yml
    @@ -3,6 +3,7 @@ type: module
    +core_version_requirement: ^8 || ^9
    

    this should be
    ^8.7 || ^9

joshi.rohit100’s picture

Test failing looks not related to this.
Also, this patch can not be merged as the only required change regarding file system is in D-8.8 and thus this can only be merged and required only for D-8.8 version.

amitgoyal’s picture

Status: Needs review » Reviewed & tested by the community

#20 looks good to me.

phenaproxima’s picture

Title: Prepare Video Embed Field for Drupal 9 » Prepare Video Embed Field for Drupal 9
Status: Reviewed & tested by the community » Needs review
FileSize
3.38 KB

Not quite...there are still a couple of calls to entity_get_form_display() in a test. This patch removes them.

phenaproxima’s picture

Merged #3117181: Declare Drupal 9 compatibility into this one, so it will only take one commit to get this module D9 ready.

Status: Needs review » Needs work

The last submitted patch, 23: 3097287-23.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
6.02 KB

Made a couple of small changes which I hope will help fix most, if not all, of the failures.

Status: Needs review » Needs work

The last submitted patch, 25: 3097287-25.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
6.57 KB

This should hopefully do better.

phenaproxima’s picture

I just realized that my approach to the Colorbox info file is potentially destructive if Colorbox is actually installed. Seeing as how it is already explicitly declared as a test dependency, I don't think we need to "fake-enable" Colorbox in the kernel tests; let's enable it for real.

Status: Needs review » Needs work

The last submitted patch, 28: 3097287-28.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
6.68 KB

How 'bout this...

  • Sam152 committed 4902426 on 8.x-2.x authored by phenaproxima
    Issue #3097287 by saurabh.tripathi.cs, phenaproxima, joshi.rohit100:...
Sam152’s picture

Status: Needs review » Fixed

Nice, thanks.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.