Drupal 8 port

Good start.

1. +++ b/config/install/php_ffmpeg.settings.yml
   @@ -0,0 +1,5 @@
   +php_ffmpeg_ffmpeg_binary: ''
   +php_ffmpeg_ffprobe_binary: ''
   +php_ffmpeg_timeout: 0
   +php_ffmpeg_threads: 0
   +php_ffmpeg_monolog_channel: null
   

   no need to repeat the module prefix in the config keys, they are already namespaced.

2. +++ b/php_ffmpeg.info.yml
   @@ -0,0 +1,9 @@
   +test_dependencies:
   +  - ctools
   +  - token
   

   really needed or copied from somewhere?

3. +++ b/php_ffmpeg.module
   @@ -1,118 +1,5 @@
    /**
     * @file
   - * Hook implementations and shared functions for the PHP-FFMpeg module.
   + * Hook implementations functions for the PHP-FFMpeg module.
     */
   

   you can remove the file if there is nothing left in here. although technically, all modules should have a hook_help() implementation, so keeping it is fine too.

4. +++ b/src/FFMpegCache.php
   @@ -0,0 +1,72 @@
   +
   +/**
   + * @file
   + * @todo add file description
   + */
   

   no @file on classes anymore.

5. +++ b/src/FFMpegCache.php
   @@ -0,0 +1,72 @@
   +/**
   + * Class FFMpegCache
   + *
   + * Adapter between Doctrine cache needed by FFMPeg library and Drupal cache.
   + */
   +class FFMpegCache implements Cache {
   

   No need for that standard Class XY thing that phpstorm ads. just keep the second line.

6. +++ b/src/FFMpegCache.php
   @@ -0,0 +1,72 @@
   +   */
   +  public function fetch($id) {
   +    return $this->cache->get($id);
   +  }
   +
   

   wondering if we should have a prefix, no idea what those cache ids will look like.

7. +++ b/src/FFMpegFactory.php
   @@ -0,0 +1,63 @@
   + * @file
   + * Contains service for the FFMPeg API integration.
   + */
   +
   

   also remove

8. +++ b/src/FFMpegFactory.php
   @@ -0,0 +1,63 @@
   +        \Drupal::logger('php_ffmpeg'),
   +        \Drupal::service('php_ffmpeg.cache')
   

   you can inject those into the factory. to do that with the logger, you can define it as a service too, core has examples.

9. +++ b/src/FFMpegFactory.php
   @@ -0,0 +1,63 @@
   +      'ffmpeg.binaries'  => \Drupal::config('php_ffmpeg.settings')->get('php_ffmpeg_ffmpeg_binary'),
   +      'ffprobe.binaries' => \Drupal::config('php_ffmpeg.settings')->get('php_ffmpeg_ffprobe_binary'),
   +      'timeout'          => \Drupal::config('php_ffmpeg.settings')->get('php_ffmpeg_timeout'),
   

   same for the config, inject the config factory.

10. +++ b/src/Form/PhpFFMpegSettings.php
    @@ -0,0 +1,109 @@
    +    $form['php_ffmpeg_ffmpeg_binary'] = [
    +      '#type' => 'textfield',
    +      '#title' => t('ffmpeg binary'),
    +      '#description' => t('Path to the ffmpeg binary. Leave empty if the binary is located within system PATH.'),
    

    I would remove the prefix here as well.

11. +++ b/src/Form/PhpFFMpegSettings.php
    @@ -0,0 +1,109 @@
    +    if ($form_state->getValue(['php_ffmpeg_timeout']) && (!is_numeric($form_state->getValue(['php_ffmpeg_timeout'])) || $form_state->getValue(['php_ffmpeg_timeout']) < 0 || (intval($form_state->getValue(['php_ffmpeg_timeout'])) != $form_state->getValue(['php_ffmpeg_timeout'])))) {
    +      $form_state->setErrorByName('php_ffmpeg_timeout', t('The value of the Timeout field must be a positive integer.'));
    +    }
    +    if ($form_state->getValue(['php_ffmpeg_threads']) && (!is_numeric($form_state->getValue(['php_ffmpeg_threads'])) || $form_state->getValue(['php_ffmpeg_threads']) < 0) || (intval($form_state->getValue(['php_ffmpeg_threads'])) != $form_state->getValue(['php_ffmpeg_threads']))) {
    +      $form_state->setErrorByName('php_ffmpeg_threads', t('The value of the Threads field must be zero or a positive integer.'));
    +    }
    

    you can do this using #type number and set #step to 1 and #max/#min accordingly.

12. +++ b/src/Tests/PHPFFMpegTestCase.php
    @@ -0,0 +1,111 @@
    +
    +  protected $profile = 'standard';
    +
    

    standard is going to be extremely slow. lets try to change to testing.

13. +++ /dev/null
    @@ -1,128 +0,0 @@
    -    parent::setUp();
    -    $this->backend = new PHPFFMpegCacheTestCaseMemoryCache();
    -    $this->prefix = $this->randomString();
    -    $this->cache = new PHPFFMpegCache($this->backend, $this->prefix);
    -  }
    -
    -  public function testFetch() {
    -    $cid = $this->randomString();
    -    $value = $this->randomString();
    -    $this->backend->set("{$this->prefix}:{$cid}", $value);
    

    we still have the adapter, so lets keep this test.

    Should be quite easy to convert this to a unit test that uses mocks.

1. +++ b/src/Tests/PHPFFMpegTestCase.php
   index 0000000..2e8d807
   --- /dev/null
   
   --- /dev/null
   +++ b/tests/src/Kernel/PHPFFMpegCacheTest.test
   

   wrong file extension.

2. +++ b/tests/src/Kernel/PHPFFMpegCacheTest.test
   @@ -0,0 +1,64 @@
   +class PHPFFMpegCacheTest extends KernelTestBase {
   

   a kernel tests works too I suppose, although a unit test here would be nicer IMHO

Working on this in a PR now, to have tests run: https://github.com/FloeDesignTechnologies/drupal-php-ffmpeg/pull/1

Awesome!

I've not worked with Drupal 8 much but this looks good to me. Anything missing or do you consider this RTBC?

Also, would you be interested becoming maintainer for the Drupal 8 version?

Currently the port does not seem to support Monolog but retains configuration for a Monolog channel. I'm not convinced Monolog support is a must-have in Drupal8, so removing it from the settings should be enough.

The logger adapter is removed because Drupal watchdog already implements the psr logger interface.

We inject a watchdog channel logger, someone could alter the service definition and log it to another place, but I think it is more likely for a drupal site to switch out the dblog module completely and log everything somewhere else?

I think it's pretty much ready, still need to do some final tests, however. I just pushed a small commit with a problem that I noticed that broke the simpletest UI, that is also fixed now. Tests in the PR are passing, so that's good. I can clean up those commits a bit if you want. Committing as a single patch/commit is also fine with me.

Sure, I can help with maintaining, can't promise much since I have a ton of modules where I'm involved. But I'll try to follow eventual 8.x issues and help with reviews/commits.

I've seen injection of the logger. Thanks to the service injection and the service override in Drupal, it can easily be replaced if wanted. So Monolog support in the module would not be relevant. We just need to remove the code for the monolog_channel setting (in the settings form and settings definition yaml files).

This is a very low maintenance module (the module is simple, and there isn't many users anyway). I'll add you as maintainer. I can still take care of the merge if you want.

Le'ts not forget to update the README (and module description on drupal.org) too.

Ok, updated the README, removed the monolog setting and added the missing probe service.

I think we're good to go?

Do you want me to merge this to d.o? What is your process with the github repository, is drupal.org the main repository and the other is then updated? (I can't push to that I guess)

Yes, we are good to go.

I would like to keep the repository on d.o the main one. The GitHub one should be a mirror. But I'm fine as long as both repo are kept fully in sync. I added you as collaborator on GitHub, so you can now commit there too. Which mean I'm okay with you handling the merging. If not done, I'll handle it myself in the coming day.

Once this is done, I'll make a 8.x-1.0 release of the module.

Ok, I pushed the code to drupal.org and github, looks like I can't create releases, so assigning to you for that.

Would be great if you could do that asap, then I can already switch to an official release (I don't care about dev or alpha or stable, as long as there's anything) in our project, which will make updating easier.

I made a RC release (and a dev one too). If nothing comes up before, I'll make it the 8.x-1.0 release around September 1st.

@berdir Do you want MD Systems to be added as supporting organization for the development of the D8 version ?

Yes please :)

I'd say we can close this issue now?

Yes off course.
Thank you for the port.

Version 8.x-1.0 released. Again, thanks Berdir, s_leu and MD System for the port to Drupal 8.