Make components more independent by getting rid of assumptions on directory structure

[thomheymann]

[enhancement/bug/1.3x]

the plugin fails if you play around with the default directory structure a little.. for instance i've used the following:

---

MEDIA:
*/app/media/*
MEDIA_TRANSFER:
*/app/media/transfer/*
MEDIA_FILTER:
*/app/webroot/files*
MEDIA_STATIC:
*/app/webroot/static*

var $actsAs = array(
    'Media.Transfer' => array(
        'trustClient' => false, *// default*
        'baseDirectory' => MEDIA_TRANSFER, *// default*
        'createDirectory' => true, *// default*
        'overwrite' => true *// default*
        ),
    'Media.Generator' => array(
        'baseDirectory' => MEDIA, *// default*
        'filterDirectory' => MEDIA_FILTER, *// default*
        'createDirectory' => true *// default*
        ),
    'Media.Coupler' => array(
        'baseDirectory' => MEDIA *// default*
        ),
    'Media.Meta' => array(
        'level'  => 2 *// default*
        )
    );

---

now when saving a file and generating versions the following happens:
the file gets saved in:
/app/media/transfer/ img/somePicture.jpg (as expected)
but versions get saved in:
/app/webroot/files/ version/ transfer/img/somePicture.jpg (!?)

---

the reason this happens is that Generator's baseDirectory defaults to MEDIA (which in this case is /app/media/, this baseDirectory then get's removed from the absolute path [/app/media/transfer/ img/somePicture.jpg] to generate the relative path to the file which becomes transfer/ img/somePicture.jpg [oups - it should be img/somePicture.jpg])
so let's manually set Generator's baseDirectory:

var $actsAs = array(
    'Media.Generator' => array(
        'baseDirectory' => MEDIA_TRANSFER
        )

and see what happens..

---

Warning (2): imagecreatefromjpeg(/app/media/transfer/transfer/img/somePicture.jpg) [function.imagecreatefromjpeg]: failed to open stream: No such file or directory [APP/plugins/media/vendors/media/adapter/gd.php, line 82]
Warning (512): Media::make - Instruction ImageMedia::convert() failed. [APP/plugins/media/vendors/media/media.php, line 319]
Warning (512): GeneratorBehavior::make - Failed to make version version of file /app/media/transfer/transfer/img/somePicture.jpg. [APP/plugins/media/models/behaviors/generator.php, line 133]

---

ok.. so the path to the file is obviously broken with the double /transfer/ in there but how comes? well long story short - versions are made afterSave() and by then Coupler has stepped in and added its dirname of transfer/img (again, because it uses MEDIA, not MEDIA_TRANSFER)
so let's change Coupler's baseDirectory to MEDIA_TRANSFER as well:

var $actsAs = array(
    'Media.Coupler' => array(
        'baseDirectory' => MEDIA_TRANSFER
        )

---

Warning (2): imagecreatefromjpeg(/app/media/transfer/transfer/img/somePicture.jpg) [function.imagecreatefromjpeg]: failed to open stream: No such file or directory [APP/plugins/media/vendors/media/adapter/gd.php, line 82]
Warning (512): Media::make - Instruction ImageMedia::convert() failed. [APP/plugins/media/vendors/media/media.php, line 319]
Warning (512): GeneratorBehavior::make - Failed to make version version of file /app/media/transfer/transfer/img/somePicture.jpg. [APP/plugins/media/models/behaviors/generator.php, line 133]

---

this doesn't work either - same error message, why? well Coupler is smart and ignores baseDirectory settings if the model has also been bound to Transfer which it is in this case, but then it does something stupid:
it assumes that Transfer's baseDirectory (MEDIA_TRANSFER) has to be under MEDIA (which is Coupler's default baseDirectory), so it simply goes one directory up of Transfer's baseDirectory by turning:
/app/media/transfer/
into:
/app/media/

and that then fails since it gives us a dirname for each record of transfer/img which is appended to MEDIA_TRANSFER (/app/media/transfer/) which gives us the final /app/media/transfer/transfer/img/somePicture.jpg

so this can't be fixed by a simple config change anymore so let's change coupler.php line 59:

$settings['baseDirectory'] = dirname($transferSettings['baseDirectory']) . DS;

into:

$settings['baseDirectory'] = $transferSettings['baseDirectory'];

voila! - working..

---

now i thought about simply pushing this as a fix but i think this really requires a general design change.. (shell scripts and helper would need some changing too) at the moment the media plugin is designed in a paradox way.. it splits up file uploads into the following tasks: transfer, versioning and storing a file in a database.. this is brilliant, however, even though these parts are meant to work independent from each other they are bound together by a fixed file structure convention which assumes that all those different directories (/transfer/, /static/, /filter/) are encapsulated in one single directory (/media/).. i propose a design that stops assuming this and gets rid of the MEDIA constant altogether..
this would mean that Transfer defines the directory where all files are located using its baseDirectory setting..
Generator and Coupler inherit this setting from Transfer.. however Generator keeps its own filterDirectory setting which is used as the 'base directory' of versioned files..
it would give more flexibility yet still allow to build directories the current way (encapsulating everything in /media/)

any thoughts? i would be happy to push a few fixes for this myself.. in fact maybe we could get rid of the constants altogether but let me know what you think.. but maybe im overlooking something..

[mariuswilms]

Wow thanks for investigating, this is a really nice report! Going to look into that bug very soon. There's been already some discussion regarding removal of the constants. I must think about that problem again and see if I can come up with an easier and better solution which works throughout the whole plugin.

[thomheymann]

i think removing the constants would have a lot of advantages even allowing multiple different transfer and filter directories for different models.. however as mentioned i think that if we go one step further and remove the paradigm of having a /media/ directory and use /transfer/ /filter/ and /static/ only it could be implemented in a flexible, clean and intuitive way..

/transfer/ could be set using baseDirectory of Transfer.

/filter/ could be set using filterDirectory of Generator (which also inherits baseDirectory from Transfer if bound to the same model, otherwise default value of /webroot/media/transfer is used).

/static/ is irrelevant for all Behaviors anyway (Transfer, Generator, Coupler) - we only need it in the MediaHelper.

MediaHelper has a baseDirectory, filterDirectory and staticDirectory setting in this scenario

all those settings default to the current /webroot/media/transfer/, /webroot/media/filter/, /webroot/media/static/ scheme

example model

var $actsAs = array(
    'Media.Transfer' => array(
        'baseDirectory' => WEB_ROOT.'media'.DS.'transfer'
        ),
    'Media.Generator' => array(
        'baseDirectory' => WEB_ROOT.'media'.DS.'transfer' *// inherited from Transfer*
        'filterDirectory' => WEB_ROOT.'media'.DS.'filter'
        ),
    'Media.Coupler' => array(
        'baseDirectory' => WEB_ROOT.'media'.DS.'transfer' *// inherited from Transfer*
        )
    );

you know what, in fact i think the term baseDirectory is where it becomes confusing as there shouldn't really be a 'base directory'.. the dirname field should always get its relative path value from the transferDirectory so a unified 'base directory' along all behaviors suggests something untrue (e.g. if a file is uploaded to /app/transfer/img/checksum/file.jpg and the transferDirectory is /app/transfer/ the dirname value should always be img/checksum - it doesnt have anything to do with Generator or Coupler at all)

so how about something like this:

example model

var $actsAs = array(
    'Media.Transfer' => array(
        'directory' => WEB_ROOT.'media'.DS.'transfer'
        ),
    'Media.Generator' => array(
        'directory' => WEB_ROOT.'media'.DS.'filter'
        'transferDirectory' => WEB_ROOT.'media'.DS.'transfer' *// inherited from Transfer*
        ),
    'Media.Coupler' => array(
        'transferDirectory' => WEB_ROOT.'media'.DS.'transfer' *// inherited from Transfer*
        )
    );

(the logic of the settings stay exactly the same but i've swapped the perspective making clear which setting belongs to whom)

the MediaHelper could be built like this

example controller

var $helpers = array(
    'Media.Media' => array(
        'transferDirectory' => WEB_ROOT.'media'.DS.'transfer' *// maybe inherited from Transfer the same way FormHelper guesses the main Model*
        'filterDirectory' => WEB_ROOT.'media'.DS.'filter' *// maybe inherited from Transfer the same way FormHelper guesses the main Model*
        'staticDirectory' => WEB_ROOT.'media'.DS.'static' *// maybe inherited from Transfer the same way FormHelper guesses the main Model*
        )
    );

i think this would be a lot more consistent.. but still allow to use e.g. Transform or Generator standalone

[iamcam]

I agree - get rid of the constants. I would love to use this plugin on a current project, but the directory structure is far too rigid for our needs. I like the idea of setting defaults at$actsAs, but I can also see the usefulness of using Configure::write() because maybe I need either a dynamic path, or at least a more flexible one. Here's a short example to illustrate what I'm thinking (take it with whatever permutations and grains of salt you wish):

Configure::write('Media.path', WWW_ROOT.'media'.DS);
Configure::write('Media.static.path', WWW_ROOT . 'media' . DS . 'static' . DS);
Configure::write('Media.static.url', 'media/'. 'static/' );
// --or -- //
Configure::write('Media.static.url', 'media/'. $dynamic_path_component. 'static/' );
// etc

[mariuswilms]

Rename Transfer Behavior baseDirectory http://github.com/davidpersson/media/commit/9e4cc3deb28704bf95379539a04f72e9c13e909c