Split DefaultContentManager into Exporter and Importer

+1

+1 with more tests. :)

Shouldn't the import process be handled as a normal migration, as we already have migrate as a core functionality?

Some PROS for #4:

• Code reuse.
• Use of an official API for importing entities.
• The import can be done anytime, not only on module enabling. This is very useful when having sites on production to whom we still need to add new content or update existing content. Old items (already imported) can be refreshed too, as we do in Migrate.

CONS:

• New dependency on 'migrate'.

Looks great, only one nit that could be fixed on commit

+++ b/src/Tests/DefaultContentManagerIntegrationTest.php
@@ -29,9 +29,9 @@ class DefaultContentManagerIntegrationTest extends KernelTestBase {
    * The tested default content manager.
    *
-   * @var \Drupal\default_content\DefaultContentManager
+   * @var \Drupal\default_content\ExportManagerInterface

nit, comment needs fix also

Hi guys,
Reroll is ready (rerolled & fixed), check install/uninstall only because I am not sure about how to test this module properly

Updated with "git diff -M25%" to reduce size of patch

that looks great but breaks a lot of patches in queue

@Larowlan please decide, this suppose to be in alpha 4 release, but maybe better to wait

+++ b/default_content.services.yml
@@ -1,10 +1,13 @@
\ No newline at end of file

minor nit to fix on commit

@larowlan do you mind to keep BC shim for old manager?

And names should be default_content.importer & default_content.exporter

@andypost - given its still an alpha, I'm happy if we break BC, provided we have a change notice.

proper title

#13 is old enough that i'm gonna have to re-roll from scratch.
significant divergences in the past 12 months

In this patch:
- Minimal changeset (in terms of class methods) to split import and export methods into two different classes
- Elimination of ununsed dependencies from constructors, use statements, and service definitions
- Elimination of unused setScanner() method

I don't think an interdiff will be helpful, because of the significant delta in codebase.

- Removes export methods from import interface

- Fix service arguments to match constructors

git is creating a difficult to read patch for this, with a copy and a couple rename operations.
Makes it somewhat hard to read.

OK, looks like testbot is choking on the patch.
I'll have to hand-roll something to appease testbot.

OK one more try with services DIC before i do a hand roll

Here's a reroll after #2857589: Remove the run-as option in drush commands

Also I cleaned-up code a bit.

After that I think it makes sense
- to get rid of "DefaultContent" in interfaces and classes because they are namespaced already
- move dumplication of LINK_DOMAIN const somewhere, maybe container param?!

So, rename classes to "Importer", "Exporter", "ImporterInterface" and "ExporterInterface"?
And a container param goes where? services.yml?

A bit more clean-up

@aaronbauman yes, exactly, I think that will be more readable and while we do so destructive change better to architect it right

1. +++ b/src/DefaultContentExporter.php
   @@ -0,0 +1,225 @@
   +      file_prepare_directory($entity_type_folder, FILE_CREATE_DIRECTORY);
   ...
   +        file_put_contents($entity_type_folder . '/' . $uuid . '.json', $serialized_entity);
   

   Future follow up would be to put this behind a service too, so we could unit test

2. +++ b/src/DefaultContentImporter.php
   @@ -0,0 +1,243 @@
   +  const LINK_DOMAIN = 'http://drupal.org';
   

   We should just refer to the constant on the exporter, no need to define twice

3. +++ b/src/DefaultContentImporter.php
   @@ -0,0 +1,243 @@
   +    $folder = drupal_get_path('module', $module) . "/content";
   ...
   +        if (!file_exists($folder . '/' . $entity_type_id)) {
   ...
   +    return file_get_contents($file->uri);
   

   Follow up would be to put this behind another service

4. +++ b/src/DefaultContentScanner.php
   @@ -16,7 +16,7 @@ class DefaultContentScanner {
   -  public function scan($directory) {
   +  public static function scan($directory) {
   

   Why this change - we can't mock static methods

Also, still need a change notice

Here's a new change record: https://www.drupal.org/node/2862246

working on a re-roll now.

wrt the file_* operations, i'm looking for examples from core. Looks like the preferred way to do this with a service is to create StreamWrapperInterfaces, extending LocalStream (for export) and LocalReadOnlyStream (for import). The stream wrapper would replace the existing Scanner altogether. The Gettext / PO components have a robust example.

To me, this seems like overkill to implement a new stream service just to wrap a handful of PHP functions, especially when we don't really have any special reading or parsing considerations like PO. Importer::parseFile() already provides a level indirection which we can mock for testing. Refactoring Exporter::writeDefaultContent to add indirection around function calls should, I think, be sufficient for unit testing. There are dozens of examples of core classes calling file_get_contents directly within a protected utility method, for example to read YAML files.

What do you think?

In this patch:
- parameterizes link_domain in services.yml and updates relevant classes (Maybe this makes more sense as an install config or setting?)
- splits DefaultContentManager and its interface into Importer and Exporter
- rename DefaultContentScanner to Scanner
- Add ScannerInterface
- Expose Scanner as a service and inject dependency into Importer (rolls in #2861315)
- Add wrapper methods in Exporter for file_prepare_directory and file_put_contents

I had to fudge the interdiff a bit because i couldn't git wouldn't grok the file moves, should be readable though.

Yeah the file_ changes are 100% out of scope here - but something we should create follow ups for.

If we had them behind a service/interface, we could mock or use vfsStream - leading to the ability to use pure unit tests.

But 100% follow ups.

Added some code samples to the change record.

This is ready in my book.

@Berdir please confirm that

+++ b/drush/default_content.drush.inc
@@ -55,9 +54,9 @@ function default_content_drush_command() {
+  /** @var \Drupal\default_content\DefaultContentExporterInterface $exporter */

@@ -76,8 +75,8 @@ function drush_default_content_export($entity_type_id, $entity_id) {
-  /** @var \Drupal\default_content\DefaultContentManagerInterface $manager */

+++ b/src/Exporter.php
@@ -0,0 +1,254 @@
+class Exporter implements ExporterInterface {

+++ b/src/ExporterInterface.php
@@ -0,0 +1,57 @@
+interface ExporterInterface {

+++ b/src/Importer.php
@@ -0,0 +1,258 @@
+class Importer implements ImporterInterface {

+++ b/src/ImporterInterface.php
@@ -0,0 +1,21 @@
+interface ImporterInterface {

could be fixed on commit

+++ b/src/Scanner.php
@@ -0,0 +1,43 @@
+class Scanner implements ScannerInterface {
...
+  public function scan($directory) {

I still see this as Component and static implementation

directory totally injectable with vfs

I'm going to commit this soon and continue with #2867579: Core 8.3 compatibility

+1

Now a lot of patches will need re-roll

PS: CR published https://www.drupal.org/node/2862246

I commited follow-up

http://drupalcode.org/default_content/commit/?id=2393ca1
http://drupalcode.org/default_content/commit/?id=8ce6a8f