davidyell / cakephp-proffer Goto Github PK

The event listener event takes an Imagine interface as a param which means that swapping out Imagine will not work because the event params will not match.

Warning (2): Illegal string offset 'error' [ROOT/plugins/Proffer/src/Model/Behavior/ProfferBehavior.php, line 52]
Code Context
    public function beforeValidate(Event $event, Entity $entity, ArrayObject $options) {
        foreach ($this->config() as $field => $settings) {
            if ($this->_table->validator()->isEmptyAllowed($field, false) && $entity->get($field)['error'] === UPLOAD_ERR_NO_FILE) {
Proffer\Model\Behavior\ProfferBehavior::beforeValidate() - ROOT/plugins/Proffer/src/Model/Behavior/ProfferBehavior.php, line 52
Cake\Event\EventManager::_callListener() - CORE/src/Event/EventManager.php, line 269
Cake\Event\EventManager::dispatch() - CORE/src/Event/EventManager.php, line 234
Cake\ORM\Table::dispatchEvent() - CORE/src/Event/EventManagerTrait.php, line 75
Cake\ORM\EntityValidator::_processValidation() - CORE/src/ORM/EntityValidator.php, line 161
Cake\ORM\EntityValidator::one() - CORE/src/ORM/EntityValidator.php, line 121
Cake\ORM\Table::validate() - CORE/src/ORM/Table.php, line 1834
Cake\ORM\Table::_processSave() - CORE/src/ORM/Table.php, line 1224
Cake\ORM\Table::Cake\ORM\{closure}() - CORE/src/ORM/Table.php, line 1192
Cake\Database\Connection::transactional() - CORE/src/Database/Connection.php, line 533
Cake\ORM\Table::save() - CORE/src/ORM/Table.php, line 1193
App\Controller\Admin\ClubsController::edit() - APP/Controller/Admin/clubsController.php, line 71
Cake\Controller\Controller::invokeAction() - CORE/src/Controller/Controller.php, line 414
Cake\Routing\Dispatcher::_invoke() - CORE/src/Routing/Dispatcher.php, line 111
Cake\Routing\Dispatcher::dispatch() - CORE/src/Routing/Dispatcher.php, line 85
[main] - ROOT/webroot/index.php, line 37

So it looks like the beforeValidate method is having trouble picking up the field when an allowEmpty() rule is configured.

When new images are uploaded the old ones need to be managed so that the server won't just fill up with useless images.

tried to upload an image that was smaller than the thumbnail sizes and got an error. I was expecting the image to stay the same size.

Issue submitted via email

You have this line under ProfferPath.php:

$this->setSeed($this->generateSeed($entity->get($settings['dir'])));

With this setup I kept getting null as the $seed value. After debugging a few variables I found that my setting were under the $settings variable for the method _construct in ProfferPath.php.

I ended up using:

$this->setSeed($this->generateSeed($settings['dir']));

This setup worked.

Followed your project description for the plugin. I have the "mimetype" validator rule set for the UsersTable. When using a test form (type=file) with input (type=file) and fieldname "image", I get the following warning:

Warning (2): finfo::file(): Empty filename or path [ROOT\vendor\davidyell\proffer\src\Model\Validation\ProfferRules.php, line 70]

which boils down to the following code:

$type = $finfo->file($value['tmp_name'], FILEINFO_MIME_TYPE);

in the mimetype() method of the class.

I think there should be an isset() and empty() just above it. If empty/null/not exists, then return false as well.

p.s. Thanks for the plugin! Great work

The events are currently to do with generating thumbnails, but the event listener might do other things when a file is uploaded.

Renaming the events will make them more generic and give users a better more generic sounding hook into the upload process.

Relates to #14

Make sure that the pathing used for tests is consistent and doesn't use webroot as it should be using tmp.

This will mean that the behaviour will need a mock of the ProfferPath class injected into it during testing. Which means updating the behaviour to allow DI.

When you upload new images a new path is generated which causes extra folders to be created rather than the files just being placed in the existing folder.

buildPath() needs to be allowed to override the seed generation.

Blocked by #10

Once the tests are written, setup Travis to keep an eye on build quality.

Hello,
I found a problem in thumbnailCropScale() method

Original code:

protected function thumbnailCropScale(ImageInterface $image, $targetWidth, $targetHeight)
{
    $target = new Box($targetWidth, $targetHeight);
    $sourceSize = $image->getSize();
    if ($sourceSize->getWidth() > $sourceSize->getHeight()) {
        $width = $sourceSize->getWidth() * ($target->getHeight() / $sourceSize->getHeight());
        $cropPoint = new Point((int)(max($width - $target->getWidth(), 0) / 2), 0);
    } else {
        $height = $sourceSize->getHeight() * ($target->getWidth() / $sourceSize->getWidth());
        $cropPoint = new Point(0, (int)(max($height - $target->getHeight(), 0) / 2));
    }
    $box = new Box($targetWidth, $targetHeight);
    return $image->thumbnail($box, ImageInterface::THUMBNAIL_OUTBOUND)
    ->crop($cropPoint, $target);
}

According to that code, an image of 3008 x 2000 (http://i.imgur.com/WwG9fGt.jpg) will be resize to 500 x 332.
Then when we ask Imagine to crop it to 500 x 500 for example, it will extend the width of 332 with a black border (http://i.imgur.com/MeeXNxY.jpg).

The solution is to ask Imagine to resize the image based on targetHeight for a landscape image and on targetWidth for a portrait image.

That way, Imagine will resize it to 752 x 500, which can be cropped correctly to 500 x 500 (http://i.imgur.com/rLg4s5s.jpg)

Fixed code:

protected function thumbnailCropScale(ImageInterface $image, $targetWidth, $targetHeight)
{
    $target = new Box($targetWidth, $targetHeight);
    $sourceSize = $image->getSize();
    if ($sourceSize->getWidth() > $sourceSize->getHeight()) {
        $width = $sourceSize->getWidth() * ($target->getHeight() / $sourceSize->getHeight());
        $height = $targetHeight;
        $cropPoint = new Point((int)(max($width - $target->getWidth(), 0) / 2), 0);
    } else {
        $height = $sourceSize->getHeight() * ($target->getWidth() / $sourceSize->getWidth());
        $width = $targetWidth;
        $cropPoint = new Point(0, (int)(max($height - $target->getHeight(), 0) / 2));
    }
    $box = new Box($width, $height);
    return $image->thumbnail($box, ImageInterface::THUMBNAIL_OUTBOUND)
    ->crop($cropPoint, $target);
}

Regards,

The cleanup task in the shell will always look in the 'default' path for files. It should probably look up the configuration from the Table.

https://github.com/davidyell/CakePHP3-Proffer/blob/master/src/Shell/ProfferShell.php#L116

If the path has been customised using an event listener, then I'm not too sure how to read that properly. Perhaps the shell can prompt for the path?

Follows on from #8

The shell should probably delete images which are on the filesystem but which are not configured in the behaviour config.

Try and find a way to remove the stub classes, perhaps using more intelligent mocks or similar.

Need to add some tests which cover the validation rules class.

It would be worth updating the documentation to outline the scope of the plugin. What functionality it will provide and which functionality it will not provide.

Hi,
I think we should allow more costomization of the file path.
Following the doc (https://github.com/davidyell/CakePHP3-Proffer/blob/master/docs/examples/UploadFilenameListener.md) we only can change filename and seed.

What about if i do not have to deal with the table name?

setTable (https://github.com/davidyell/CakePHP3-Proffer/blob/master/src/Lib/ProfferPath.php#L91) is a protected method, we should set it to public and change fullPath (https://github.com/davidyell/CakePHP3-Proffer/blob/master/src/Lib/ProfferPath.php#L208) method to manage empty '' table.

The validation needs to be read in order to find out if the image needs to be processed or not.

$table->validator() will get the 'default' validation group. Need to think about custom validation groups and also custom validator objects.

Need to use a field in the entity which won't be cast by the marshaller. So it'll need to be a field in the data which is different to the one stored in the database.

A few ideas to try

1. Try just storing the file upload array in the entity using a different name to the saved db field. Then when the data has been saved I can query the entity to process the data. Relates to #46
2. Might be possible to use an entity virtual field

Then I can populate the actual entity field name in the entity after processing the

I need to provide and example of replacing the listener, or at least not running it.

Need to write a shell so that thumbnails can be automatically regenerated should the model config change.

Executing the shell task is creating some errors .

bin/cake bake proffer.proffer generate posts

Produces, Exception: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'posts.image_dir' in 'field list' in [/var/www/domain.com/dev/website/vendor/cakephp/cakephp/src/Database/Statement/MysqlStatement.php, line 36]

Relates to #79

Would it be worth building functionality to allow an image to be uploaded and saved as a base64 string so that all the images could be stored in the db?

I can't think of a use case for this immediately off-hand, but I'm sure it would be handy to some?

I have been testing this plugin with an application I am creating. For some time everything was working great, until a few days ago I noticed that my tests were saving good, except there was not images being uploaded and no errors at all. After debugging, I find out I am getting the following error:

Column not found: 1054 Unknown column 'posts.image_dir' in 'field list

After running

bin/cake proffer.proffer generate posts

image_dir is in fact a columns in the posts database table.

Since the only column not saving are the ones that Proffer needs, is there any chance there is an issue within your plugin or should I look somewhere else for the problem.

Thaks,

Need something to deal with bad filenames, optionally.

So that if someone wants to fix their broken filenames it can do that, otherwise they'll need to deal with it in their own front-end.

Need to add documentation for the db configuration. Including the fields and how to configure them.

Use the event system to generate thumbnails so devs can overload the thumbnail generation totally.

It might be worth collating the various common errors that people might experience when using the plugin so that solutions to them are readily available.

Eg,
Unknown type "proffer.file"
Would be a missing 'bootstrap' => true in the loading of the plugin.

Follows on from #7

I think it's a good idea to try and think of a way to move Imagine out into the event listener. Uploading a file shouldn't rely on an image transformation library.

This would allow developers to implement their own library in their own event listener if they wanted to.

When I create an upload template do I have to include the dir form field in my form for it to work? 😄

Need to check the test coverage and increate it to cover the various scenarios.

bin/phpunit --coverage-html proffer_coverage.html plugins/Proffer/

Need some method which will return,

• A full path name to the folder
• A complete filename with and without prefix
• Component parts of the path

Need to create some tests to actually generate thumbnails and then test the resulting images match the right dimensions etc.

Need to improve the thumbnail quality as it's generating pretty poor quality, grainy images at the moment.

I've encountered this warning when removing files via normal delete() in controller/model:

Warning:

Warning (2): rmdir(...\webroot\files\projectfiles\file_name\d84df8b9-800f-4951-9ff3-781017f23971\): Directory not empty [ROOT\vendor\davidyell\proffer\src\Model\Behavior\ProfferBehavior.php, line 132]

Trace:

rmdir - [internal], line ??
Proffer\Model\Behavior\ProfferBehavior::afterDelete() - ROOT\vendor\davidyell\proffer\src\Model\Behavior\ProfferBehavior.php, line 132
Cake\Event\EventManager::_callListener() - CORE\src\Event\EventManager.php, line 385
Cake\Event\EventManager::dispatch() - CORE\src\Event\EventManager.php, line 351
Cake\ORM\Table::dispatchEvent() - CORE\src\Event\EventManagerTrait.php, line 78
Cake\ORM\Table::_processDelete() - CORE\src\ORM\Table.php, line 1696
Cake\ORM\Table::Cake\ORM\{closure}() - CORE\src\ORM\Table.php, line 1618
Cake\Database\Connection::transactional() - CORE\src\Database\Connection.php, line 561
Cake\ORM\Table::delete() - CORE\src\ORM\Table.php, line 1623
App\Controller\ProjectFilesController::delete() - APP/Controller\ProjectFilesController.php, line 138
Cake\Controller\Controller::invokeAction() - CORE\src\Controller\Controller.php, line 405
Cake\Routing\Dispatcher::_invoke() - CORE\src\Routing\Dispatcher.php, line 114
Cake\Routing\Dispatcher::dispatch() - CORE\src\Routing\Dispatcher.php, line 87
[main] - ROOT\webroot\index.php, line 37

Need to update the site so that if a file which isn't an image is uploaded, then no thumbnail is generated.

It would be nice to allow users to customise the seed which is used to create the folder. Such as using the date or similar, or perhaps filetype?

Need to ensure that there is no filename collision in unrelated records.

It might be helpful to add some event emits to the plugin around the beforeSave and beforeMarshal so that userland devs can hook the data being passed around the plugin.

Such a use-case might be wanting to maintain the users uploaded file between form submissions if the form fails submission the user should not have to resubmit their file.

Need to consider the impact of allowing this level of control, as it could create more bugs with people changing data in unintended ways.

Since the save might still fail after the beforeSave callback, it would be better to handle the processing of the uploaded file in the afterSave callback.

Before I used Proffer to upload images, but today I had to implement a file browser and used it to upload other types of files. I noticed that the config key thumbnailSizes is required ( it does also state this in the documentation ), even when uploading non-image files. When I set thumbnailSizes with an empty array, everything works out fine.

Maybe make it optional to supply thumbnailSizes, since it can be empty already at the moment?

Thanks!

https://github.com/davidyell/CakePHP3-Proffer/blob/master/.travis.yml#L35-L36

This needs to be replaced with a composer dependency.

The behaviour needs automated tests.

While implementing the Proffer code, PHP threw me an error telling me that:

Fatal error: Class 'finfo' not found in /home/...../...../finfo.php on line 4

I was able to resolve the issue enabling the fileinfo extension in php.ini . If people have the same problem, read this post: http://stackoverflow.com/questions/3579072/php-fileinfo-is-undefined-function

To resolve the issue I suggest to detect if the module is loaded yes/no and maybe create/use a fallback for it. Otherwise, maybe add it to the requirements of the plugin.

p.s. Even though the requirements say PHP5.4+ ( fileinfo was introduced in PHP5.3 i think ), it could be disabled in the php.ini of the server.

Thanks for the plugin!

The new events which the plugin emits are not tested anywhere. It would be wise to add some tests which just ensure that the events are bound properly and get fired when the code is executed.

Otherwise there is no way to guarantee that the events exist for when people want to use them to customise aspects of the plugin.

I was testing around with Proffer and had an invalid file_name and/or file_dir field content in the database. Up on deleting the model, Proffer notified me that unlink() does not work on a non-existing file ( obviously ). I can also see this happening when the physical file has been deleted [for some reason unknown] and the database still references it [and the user has not run a cleanup].

Solution: warp file_exists() around unlink()

Thx again for this great plugin.

There needs to be a way to wrap the Imagine library functionality without having to code a method for each piece of functionality provided by the library.

The updated README.md contains a syntax/typing error:
Current:
Plugin::load('Proffer', ['bootstrap' => true);

Should be:
Plugin::load('Proffer', ['bootstrap' => true]);

The readme file is starting to get very large with examples and verbose descriptions about the plugin and it's configuration.

I think it's time to create a docs folder and break up the readme into smaller more relevant files for each aspect.

The advantage of having a docs folder is that the documentation will travel with the plugin, giving developers instant access to the docs and also not requiring an internet connection, which could be handy.

• Be sure to copy the gist examples inline.

When I refactored the pathing in #14 I created a regression in the shell as it wasn't updated the use the new path class.

Need to be able to delete an image and remove the matching record in the database.