Gfrktyl

Using Laravel, I need to export some value from the DB in a CSV and then upload the CSV via sFTP or return it in a Response.

I'm trying to be use S.O.L.I.D. principles but with this scenario, I'm not sure how to proceed. As I understand I should have one class that handles the CSV, one that handles the sFTP, one for the Response, and one to handle the logic of the model (the mapping in my case). But I don't understand how I can separate them.

Later one, I will have more Model data to export.

<?php



namespace AppServices;



use AppLine;

use SymfonyComponentHttpFoundationStreamedResponse;

use Storage;



class LinesCsv

{

    const DOCUMENT_TYPE = 20;

    const DELIMITER = ';';



    public function exportCSVFileToSftp($filename = 'export.csv')

    {

        $handle = fopen('php://temp', 'w');

        $handle = $this->buildCsv($handle);

        return Storage::disk('sftp')->put($filename, $handle);

    }



    public function exportCSVFileToResponse($filename = 'export.csv')

    {

        return new StreamedResponse(function () use ($filename) {

            $handle = fopen('php://output', 'w');

            $handle = $this->buildCsv($handle);

            fclose($handle);

        }, 200, [

            'Content-Type'        => 'text/csv',

            'Content-Disposition' => 'attachment; filename="' . $filename . '"',

        ]);

    }





    public function buildCsv($handle, $header = false)

    {

        if ($header) {

            fputcsv(

                $handle,

                array_keys($this->lineMapping(Line::first())),

                self::DELIMITER

            );

        }



        Line::with(['invoice', 'invoice.customer', 'item'])

            ->whereHas('invoice', function ($query) {

                $query->where('is_exportable', 1);

            })

            ->chunk(200, function ($lines) use ($handle) {

                foreach ($lines as $line) {

                    fputcsv(

                        $handle,

                        $this->lineMapping($line),

                        self::DELIMITER

                    );

                }

            });



        return $handle;

    }



    protected function lineMapping(Line $line)

    {

        return [

            'Invoice number'            => $line->invoice->id,

            'Document type'             => self::DOCUMENT_TYPE,

            'Date'                      => $line->invoice->date,

        ];

    }

}

Try to think about this in a more generic way, you basically have:

1. Select data from DB


2. Put that data in a CSV (will you in the future want to use other formats like XLSX, etc? If so, we should abstract this step too, but I wont do this right now)


3. Return that CSV file (doesn't matter to who you're returning)

So we can create a few classes from those steps

1. Any class that can query the DB, your app probably has a bunch of them


2. AbstractWriter - this class is an abstraction/interface to write anywhere, you'll need to implement it for specific formats, like Writer/Csv, Writer/Xlsx, Writer/JSON, etc.


3. AbstractFileReturner - this class is an abstraction/interface to return it to anyone, you'll need to implement it for specific cases, AbstractFileReturner/like Sftp, AbstractFileReturner/HttpResponse, etc.

Any time you need more File formats or Returner, you just implement the AbstractWriter or FileReturner respectively one more time.

Doing all these, you'll be using Single Responsability Principle (each class do only one thing) and Open/Closed (the abstractions/interfaces are closed for modification, but open for extension).

Real code

Ok so I came up with a little bit of code having SOLID in mind and to make it easy to add more file formats and and way outs (Sftp, Response, etc).

This is a photo of the layers I tried to create, it might make the code more understandable.

Now embrace yourself for there are coming a few classes.

I didn't test this code and doens't know much of Laravel, so there might be a few bugs, but you'll get the big picture.

First, a sample controller and model which I thought was your Line.

class SomeModel

{

    // the chunk does not go here, it will go inside who's putting the data into the csv

    public static function someQuery()

    {

        return Line::with(['invoice', 'invoice.customer', 'item'])

            ->whereHas('invoice', function ($query) {

                $query->where('is_exportable', 1);

            });

    }



    public static function lineMapping(Line $line)

    {

        return [

            'Invoice number'            => $line->invoice->id,

            'Document type'             => self::DOCUMENT_TYPE,

            'Date'                      => $line->invoice->date,

        ];

    }

}



class SomeController

{

    public function exportCsvSftpAction()

    {

        $dataCollection = SomeModel::someQuery();

        $filename       = 'output.csv';



        $exporter = new SftpCsvExporter($dataCollection, $filename);

        $exporter->export();

    }



    public function exportCsvResponseAction()

    {

        $dataCollection = SomeModel::someQuery();

        $filename       = 'output2.csv';



        $exporter = new ResponseCsvExporter($dataCollection, $filename);

        $exporter->export();

    }

}

Than there's the first layer: Export, I created an interface to say what it should do (don't mind about how now) and a abstract class that implements some parts of it.

interface Exportable

{

    protected function createFile();



    protected function write($line);



    protected function deliver($filename);

}



abstract class Export implements Exportable

{

    protected $fileHandle = null;

    private   $dataCollection;

    private   $filename;



    public function __construct($dataCollection, $filename)

    {

        $this->createFile();



        // @TODO You should add validation for $dataCollection like

        // checking if it has data, if it really is a Collection so we can call chunk, etc

        // And check $filename for invalid characters   

        $this->dataCollection = $dataCollection;

        $this->filename       = $filename;

    }



    protected abstract function createFile();



    protected abstract function write($line);



    protected abstract function deliver($filename);



    // this method is who makes it all work together 

    // and let the controller be so simple 

    public function export()

    {

        $this->dataCollection->chunk(200, function ($lines) {

            foreach ($lines as $line) {

                $this->write($line);

            }

        });



        $this->deliver($this->filename);

    }

}

Now there's the second layer: Csv, in this layer you will be able to add more file formats in the future if needed.

This layer knows how to write, but doesn't know how to open nor deliver the file.

abstract class CsvExporter extends Export

{

    const DELIMITER = ';';



    private $headerWritten = false;



    protected function write($line)

    {

        if (!$this->headerWritten) {

            fputcsv(

                $this->fileHandle,

                array_keys(SomeModel::lineMapping(SomeModel::first())),

                self::DELIMITER

            );



            $this->headerWritten = true;

        }



        fputcsv(

            $this->fileHandle,

            SomeModel::lineMapping($line),

            self::DELIMITER

        );

    }

}

And finally the third and last layer: this is responsible for opening and delivering the file. So we have Sftp and Response in this layer.

class SftpCsvExporter extends CsvExporter

{

    protected function createFile()

    {

        if ($this->fileHandle !== null) {

            throw new LogicException('Handle already initilized');

        }



        $this->fileHandle = fopen('php://temp', 'w');

    }



    protected function deliver($filename)

    {

        return Storage::disk('sftp')->put($filename, $this->fileHandle);

    }

}



class ResponseCsvExporter extends CsvExporter

{

    protected function createFile()

    {

        if ($this->fileHandle !== null) {

            throw new LogicException('Handle already initilized');

        }



        $this->fileHandle = fopen('php://output', 'w');

    }



    protected function deliver($filename)

    {

        $handle = $this->fileHandle;



        return new StreamedResponse(function () use ($handle, $filename) {

            // I don't know this StreamedResponse so you might want to correct this

            // the only thing left is to close the handle before send the file

            fclose($handle);

        }, 200, [

            'Content-Type'        => 'text/csv',

            'Content-Disposition' => 'attachment; filename="' . $filename . '"',

        ]);

    }

}

Responding to your statement

As I understand I should have one class that handle the CSV, one that handle the sFTP, one for the Response, and one to handle the logic of the model (the mapping in my case).

Perhaps by the word class you mean method, as it seems you have the following methods:

• exportCSVFileToSftp


• exportCSVFileToResponse


• buildCsv


• lineMapping

And those appear to line up with what you outlined in that statement, with the exception of the CSV building abstracted out to be called by the two methods to export the data.

I have read about the S.O.L.I.D. principles a few times and tried to keep them in mind when developing new code. However, I also stumbled upon Tony Marson's Blog post: Not-so-SOLID OO principles from 2011. Basically he refutes the abstract examples used when explaining the S.O.L.I.D. principles and questions whether the implementation tracks he uses are incorrect. He does support the MVC pattern as well as three tier architecture, which overlap but are not the same thing.

General review comments

I see lines like this:

$handle = fopen('php://temp', 'w');

$handle = $this->buildCsv($handle);

and also

$handle = fopen('php://output', 'w');

$handle = $this->buildCsv($handle);

Initially my thought was that $handle was being re-assigned to the value from buildCsv() which seemed like it might return a value of a different type. Then I looked at the implementation of that method and noticed that it merely returns $handle. There doesn't really seem like much advantage to re-assign the value after the CSV data is sent to the output resource...

While the method names are quite descriptive, it would be helpful for anyone reading your code to have docblocks above each method, describing the parameters, return values, etc. While there isn't one single standard for the format, there are very popular formats like phpDocumenter.

And if you are using PHP 7, you could utilize Return type declarations in order to also improve readability.

The method exportCSVFileToResponse() appears to call fclose() on the resource but exportCSVFileToSftp() does not call fclose(). As answers to Why do I need fclose after writing to a file in PHP? explain it is a good habit to always call fclose() manually for reasons of security, memory usage, knowledge about write failure, etc.