New Drupal 8 Port

Time to bring this home :)

Work continued on https://github.com/LukyLuke/mailsystem with help from various contributors, it's working quite well, has a considerably nicer API than in 7.x and some unit tests and has been fairly stable (for an 8.x port) over the last few months.

It's not complete, I'm not sure how well the mail theme stuff is working and there are no tests for that nor UI tests in general.

The branch on the github repository is currently 8.x-4.x since it is a completely new implementation despite being based on the 8.x-2.x code base. Happy to discuss what we want to use for the branch name but it seems like a good idea to use something different to 8.x-2.x, also due to the somewhat unfortunate stable releases there.

Attaching a first patch against 8.x-2.x for reviews. I'd suggest to merge in the branch from 8.x-4.x and keep the commits, even though they don't follow d.o practice. We did that in a few other projects and I think it's fairer for all the contributors that helped with pull requests over the time.

@Les_Lim, are you still active here? I do have full access to the project but I'd like to give other maintainers some time to comment here.

Did a full review of the code base, turns out there are quite a few things that should take care of pre-merge.

Follow-ups:

- mail theme stuff (Definitely not working, mostly commented out)
- Convert html_to_text.inc to a class. Too bad that MailFormatHelper isn't a service, or we could have just replaced that.

1. diff --git a/README.html b/README.html
   old mode 100644
   new mode 100755
   

   All those permission changes should probably be reverted, seems like an unnecessary change.

2. +++ b/composer.json
   new file mode 100755
   index 0000000..256c9d8
   
   index 0000000..256c9d8
   --- /dev/null
   
   --- /dev/null
   +++ b/config/install/mailsystem.settings.yml
   
   +++ b/config/install/mailsystem.settings.yml
   +++ b/config/install/mailsystem.settings.yml
   @@ -0,0 +1,4 @@
   
   @@ -0,0 +1,4 @@
   +theme: 'current'
   +defaults:
   +  sender: 'php_mail'
   +  formatter: 'php_mail'
   

   We're missing config schema I guess, not failing because we have no simpletest tests.

3. +++ b/config/install/mailsystem.settings.yml
   index 0857622..805bfff
   --- a/html_to_text.inc
   
   --- a/html_to_text.inc
   +++ b/html_to_text.inc
   

   We should probably move this into a class, similar to what core did, but we can do that later.

4. +++ b/mailsystem.info.yml
   @@ -0,0 +1,9 @@
   +php: 5.3
   

   Can be dropped, core needs 5.4 now.

5. +++ b/mailsystem.links.menu.yml
   @@ -0,0 +1,6 @@
   +  title: Mailsystem
   

   Possibly change to Mail System and also avoid a single word or mail_system in other descriptions.

6. +++ b/mailsystem.module
   @@ -6,321 +6,24 @@
   +function mailsystem_menu_link_defaults() {
   +  $items['mailsystem.settings'] = array(
   +    'link_title' => 'Mail System',
   +    'route_name' => 'mailsystem.settings',
   

   Left-over, doesn't exist anymore.

7. +++ b/mailsystem.module
   @@ -6,321 +6,24 @@
    */
    function mailsystem_theme_registry_alter(&$theme_registry) {
      module_load_include('inc', 'mailsystem', 'mailsystem.theme');
   -  return mailsystem_theme_theme_registry_alter($theme_registry);
   +  //return mailsystem_theme_theme_registry_alter($theme_registry);
    }
   

   Yes, confirmed it's not doing anything yet, then. Probably doesn't need to block merging this?

8. +++ b/src/Adapter.php
   @@ -0,0 +1,52 @@
   +  }
   +}
   \ No newline at end of file
   diff --git a/src/AdminForm.php b/src/AdminForm.php
   

   .

9. +++ b/src/AdminForm.php
   @@ -0,0 +1,409 @@
   +/**
   + * @todo What is the "theme" for?
   + *       I don't get the use of this from the old code.
   + */
   

   No need for this @todo I think, so drop and then we'll make sure in a follow-up that the mail theme stuff is working correctly, including test coverage.

10. +++ b/src/AdminForm.php
    @@ -0,0 +1,409 @@
    +    $modules = $config->get(MailsystemManager::MAILSYSTEM_MODULES_CONFIG);
    +    $modules = is_null($modules) ? array() : $modules;
    

    Can be inlined by adding ?: [] after the get()

11. +++ b/src/Plugin/Mail/Dummy.php
    @@ -0,0 +1,47 @@
    + *
    + * @Mail(
    + *   id = "mailsystem_dummy",
    + *   label = @Translation("Dummy Mail-Plugin"),
    + *   description = @Translation("Dummy Plugin to debug the complete email on formatting and sending.")
    + * )
    + */
    +class Dummy implements MailInterface {
    

    Should be moved to a test module but that might not work with the unit test?

12. +++ b/src/Plugin/Mail/DummyFormatter.php
    @@ -0,0 +1,41 @@
    + * @Mail(
    + *   id = "mailsystem_dummyformatter",
    + *   label = @Translation("Dummy Mailsystem formatter Plugin"),
    + *   description = @Translation("Dummy Plugin to debug the email on formatting ,does not sending anything.")
    + * )
    

    Same.

13. +++ b/tests/Drupal/mailsystem/Plugin/Mail/Test.php
    @@ -0,0 +1,44 @@
    +class Test implements MailInterface {
    

    Another test?

14. +++ b/tests/Drupal/mailsystem/Tests/AdapterTest.php
    @@ -0,0 +1,86 @@
    + * @file
    + * Contains \Drupal\mailsystem\Tests\AdapterTest.
    + */
    +
    +namespace Drupal\mailsystem\Tests;
    

    Namespace should be Drupal\Tests\mailsystem\Unit in folder tests/src/Unit/

    For all tests.

Made the changes from the comment above and created a pull request on GitHub: https://github.com/LukyLuke/mailsystem/pull/11

Created the follow-ups:

1. #2498261: Convert html_to_text.inc to a class
2. #2498265: Reimplement mail theme functionality
3. #2498271: Add web tests for the settings form

Updated patch, checking if this triggers the d.o testbot now.

Discussed this with @Les_Lim, we agreed to merge and release this as a new 8.x-4.x branch.

Yay, one more project is home!

Nice cleanup. Happy to see this merged and ready for regular issue queue based workflows!