Reworked building path with PathBuilder

[repher]

more usage of PathBuilderTrait
simplified path generation for local files
created LocalPathBuilder for generating full filesystem paths and adding options for compatibility reasons (pathPrefix, randomPath)
corrected test in AbstractStorageEventListener (was accepting path without prefix even if file is 'Local')

[repher]

The remaining error was there before :-(

[burzum]

This is not going to work because the "adapter" (which should have been named config in the past) is a configuration name that can be anything. For example you could have "Local1" and "Local2" to store files into different base path locations (/var/files1 & /var/files2/ for example). It was never thought to be used as a convention. Also the idea is in fact that the path builder doesn't have to match the adapter name to allow people to customize the path building. I won't accept this part of the changes.

[repher]

Mmh. I think you lost me there … But why having two locations where paths are being built? On the one hand there is a path builder that is used everywhere (LocalListener, FileStorageTable, AbstractListener, …). Also in the comments of Storageutils:random() you state " * @deprecated Use the randomPath() method from the BasePathBuilder instead.” Hence the switch to path builder …

Bernhard

On 12.08.2015, at 10:01, Florian Krämer notifications@github.com wrote:

In src/Event/AbstractStorageEventListener.php #77 (comment):

@@ -137,16 +139,9 @@ public function buildFilename($table, $entity) {

• @return string
  */
  public function buildPath($table, $entity) {

  •   $path = '';
    

  •   if ($this->_config['tableFolder']) {
    

  •       $path .= $table->table() . DS;
    

  •   }
    

  •   if ($this->_config['randomPath'] === true) {
    

  •       $path .= StorageUtils::randomPath($entity[$table->primaryKey()]);
    

  •   }
    

  •   if ($this->_config['uuidFolder'] === true) {
    

  •       $path .= $this->stripDashes($entity[$table->primaryKey()]) . DS;
    

  •   }
    

  •   $pathBuilder = $this->createPathBuilder($entity['adapter']);
    

    This is not going to work because the "adapter" (which should have been named config in the past) is a configuration name that can be anything. It was never thought to be used as a convention. Also the idea is in fact that the path builder doesn't have to match the adapter name to allow people to customize the path building. I won't accept this part of the changes.

—
Reply to this email directly or view it on GitHub https://github.com/burzum/cakephp-file-storage/pull/77/files#r36834857.

[robertpustulka]

@burzum

because the "adapter" (which should have been named config in the past) is a configuration name that can be anything. For example you could have "Local1" and "Local2" to store files into different base path locations (/var/files1 & /var/files2/ for example). It was never thought to be used as a convention.

I'm afraid I can't agree with that. adapter property has to be a valid storage adapter name. New listeners depend on that. See this and this

Nevermind, it all goes through StorageManager eventually :) sorry for that

[burzum]

Thank you for your contribution, but I don't think I'll accept it for the reasons I've named in the other comments. Please let me know if I've missed something, I'll reopen it then.

If you want you can contact me via Github or Skype before spending time on something that might be already done. Thanks again for the effort.

[repher]

Ok. But since I had many problems getting this plugin to work I thought I would take it into my own hands to get it running. I need this very urgent for a client …

In the end all pre 3.0 compatibility stuff should be removed from the new plugin an only be available with the legacy files. Right? But what about the thing with the PathBuilder? Why don't you want AbstractStorageEventListener to keep using the path builder? I thought that was the whole plan?

Bernhard

On 12.08.2015, at 10:20, Florian Krämer notifications@github.com wrote:

Thank you for your contribution, but I don't think I'll accept it for the reasons I've named in the other comments. Please let me know if I've missed something.

If you want you can contact me before via Github or Skype before spending time on something that might be already done. Thanks again for the effort.

—
Reply to this email directly or view it on GitHub #77 (comment).

[repher]

Oh... I think I now where the problems startet. I used LocalFileStorageListener instead of LocalListener in my bootstrap.php. Now everythin works.

I need just a little direction please: I created a DocumentsController to download files that lie outside of WWW_ROOT (for security reasons). Whats the best and most beautiful way to get the whole filesystempath?

I have this:

public function download($id) {
    $file = $this->Documents->get($id);
    $path = $this->Documents->fullFilePath($file);
    $this->response->file($path, ['download' => true, 'name' => $file['filename']]);
    return $this->response;
}

but this obviously does not work since FileStorageTable::fullFilePath() return a relative path to the storage folder. Also the pathbuilder used in this function is created without any default options from LocalListener. I suppose this function should therefore not be used.

I think that something like this should work, but i cannot get it to download the file:

use StorageTrait;

public function download($id) {
    $file = $this->Documents->get($id);
    $Storage = $this->storageAdapter($file['adapter']);
    $content = $Storage->read($file['path']);

    // ????

    return $this->response;
}

Thanks!

SOLVED

I had a typo ... This works:

use StorageTrait;

public function download($id) {
    $file = $this->Documents->get($id);
    $Storage = $this->storageAdapter($file['adapter']);
    $content = $Storage->read($file['path']);

    $this->response->body( $content );
    $this->response->type($file['mime_type']);
    $this->response->download($file['filename']);

    return $this->response;
}

[burzum]

But since I had many problems getting this plugin to work

What problems exactly? There is a ton of documentation.

Why don't you want AbstractStorageEventListener to keep using the path builder? I thought that was the whole plan?

Again, this is already done. Please see my other comment on the other issue.

• This is legacy code https://github.com/burzum/cakephp-file-storage/tree/3.0-event-overhaul/src/Event
• Replaces the legacy https://github.com/burzum/cakephp-file-storage/tree/3.0-event-overhaul/src/Storage

[repher]

In the end i was confused by the documentation :-/ It all started with the bootstrap.php Listeners…

Now it works. I use my own DocumentsTable extending FileStorage and let the magic work :-) Also I use a DocumentsController to upload/download files.

DocumentsTable

<?php
namespace App\Model\Table;

use App\Model\Entity\News;
use Cake\ORM\Query;
use Cake\ORM\RulesChecker;
use Cake\ORM\Table;
use Cake\Validation\Validator;
use Burzum\FileStorage\Model\Table\FileStorageTable;

class DocumentsTable extends FileStorageTable
{
    public function initialize(array $config) {
        parent::initialize($config);
        $this->addBehavior('Tree');
        $this->addBehavior('Burzum/FileStorage.UploadValidator', [
            'allowedExtensions' => [ 'pdf' ],
            // 'validateFilesize' => true,
        ]);
    }
}

DocumentsController:

<?php
namespace App\Controller;

use App\Controller\AppController;
use Burzum\FileStorage\Storage\StorageTrait;

class DocumentsController extends AppController
{
    use StorageTrait;

    public function download($id = null)
    {
        $file = $this->Documents->get($id);
        $Storage = $this->storageAdapter($file['adapter']);
        $content = $Storage->read($file['path']);

        $this->response->body( $content );
        $this->response->type($file['mime_type']);
        $this->response->download($file['filename']);

        return $this->response;
    }

    public function delete($id = null)
    {
        $this->request->allowMethod(['post', 'delete']);
        $document = $this->Documents->get($id);
        if ($this->Documents->delete($document)) {
            $this->Flash->success(__('The document "{0}" has been deleted.', $document->filename));
        } else {
            $this->Flash->error(__('The document "{0}" could not be deleted. Please, try again.', $document->filename));
        }
        return $this->redirect($this->referer());
    }

    public function move( $id = null, $direction = null )
    {
        $this->request->allowMethod(['post', 'put']);

        if ($direction != 'up' && $direction != 'down' ) {
            throw new IllegalArgumentException(__('No valid direction'));
        }

        $document = $this->Documents->get($id);

        // setting scope to foreign_key since we want to sort only files belonging to a single news entry
        $this->Documents->behaviors()->Tree->config('scope', ['foreign_key' => $document->foreign_key]);

        switch ( $direction ) {
        case 'up':
            $this->Documents->moveUp($document);
            break;

        case 'down':
            $this->Documents->moveDown($document);
            break;
        }

        return $this->redirect($this->referer());
    }
}

Thanks,
Bernhard

[burzum]

In the end i was confused by the documentation :-/ It all started with the bootstrap.php Listeners…

What exactly is confusing there? I don't mind improving it if you could be more specific please.

Also please be aware that the overhaul branch is still in development, there is a HUGE PR outstanding that should be fully BC compatible, but it might break things. If it does it needs to be fixed. It should provide 100% BC.

[repher]

I set it up like in the documentation but in the end the following line is wrong

$listener = new LocalFileStorageListener();

and has to be replaced by

$listener = new LocalListener();