Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Media model api #110

Merged
merged 95 commits into from
Oct 18, 2013
Merged

Media model api #110

merged 95 commits into from
Oct 18, 2013

Conversation

rjmackay
Copy link
Contributor

@rjmackay rjmackay commented Oct 7, 2013

Initial implementation and getting myself up to speed with the current code base. So far:

  • Added media table with schema defined in the migration file 20131007103144.php
  • Added a media model.
  • Added media controller application/classes/Controller/Api/Media.php with some initial implementation for the various endpoints
  • Added test for media model

Next

  • Implement the actual file upload so it uploads the images to application/media/uploads
  • Look into adding migration script for images.
  • Write behat test to test the endpoints.
  • Look into wiring the media endpoints when creating a post.
  • Review feature

@rjmackay rjmackay mentioned this pull request Oct 7, 2013
@rjmackay
Copy link
Contributor Author

rjmackay commented Oct 7, 2013

@eyedol I think this is mostly on the right track..

  • for thumbnails, etc.. I suggest we only store the original in the DB, and abstract the thumbnail/resize handling out to another library. I'm thinking we do something like the Drupal imagecache module, where you can request any image size you like and it generates + caches it.

    This means themers can change thumbnail sizes etc as needed. You can happily delete all cached thumbnails at any time and they just get rebuilt when needed.

    Could use something like: http://kohanaframework.org/3.3/guide/image/examples/dynamic or there might be something integrated with kohana-media already.

  • Whats the logic with storing the base image/media url in the db? are you just thinking ahead on CDN media hosting?

@rjmackay
Copy link
Contributor Author

rjmackay commented Oct 7, 2013

In fact this might work for serving image thumbnails etc: https://github.com/Bodom78/kohana-imagefly

@eyedol
Copy link
Contributor

eyedol commented Oct 8, 2013

@rjmackay Imagefly seems like a great library though I can't figure out how I can, in code, fetch the various sizes that I want. As an API consumer, when I hit the media endpoint, I expect some re-resized images for my consumption, that was why I had the different sizes defined in the table.

Would be great if imagefly could resize images in code without url params, that way, when I fetch the original image from the db, I could use it for resizing to the different sizes for the API response. I'll play around with it and see.

The file_url was to accommodate media hosted on CDN and also to make it easier to get the location of the media. I think it makes more sense to just maintain the file_url and drop the filename field. So instead of the file_url being http://domain.com/media/upload/photos it becomes http://domain.com/media/upload/photos/51f75f03cb6fe1.02579520_o.jpg

@brianherbert
Copy link

If you guys end up doing dynamic image resizing on request, proceed with caution. It's very expensive to do in real time and also very difficult if you are serving media off a CDN.

@rjmackay
Copy link
Contributor Author

rjmackay commented Oct 8, 2013

@brianherbert Good point. Doing it in real-time can mostly be avoided.. but also if the resized image is cached it only has to happens once anyway. CDN is a more serious issue though.

@kamaulynder
Copy link
Contributor

There was an email/post from Evan on this, as I scramble to find it. It was quite detailed.
On Oct 8, 2013, at 11:21 AM, Robbie MacKay wrote:

@brianherbert Good point. Doing it in real-time can mostly be avoided.. but also if the resized image is cached it only has to happens once anyway. CDN is a more serious issue though.


Reply to this email directly or view it on GitHub.

@eyedol
Copy link
Contributor

eyedol commented Oct 9, 2013

Progress so far:

  • Implemented image upload to DOCROOT/uploads
  • Added basic validation when uploading an image. Images shouldn't be more than 1MB and they should be any of these types gif,jpg,jpeg,png
  • Added endpoints for uploading, fetching and deleting images.
  • Added initial documentation on the wiki

Next

  • Write Behat test for the endpoints.
  • Look into migration script for media.


protected $upload_dir;

public function before()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we overriding before() ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to initialize $this->upload_dir. I have removed this. The global scope of it is no longer needed as I'm moving the unlink call to the model class.

@rjmackay
Copy link
Contributor Author

rjmackay commented Oct 9, 2013

@Henry RE: "As an API consumer, when I hit the media endpoint, I expect some re-resized images for my consumption, that was why I had the different sizes defined in the table."
I agree.. I wasn't thinking about like an API function. Definitely provide a few sizes in the API.
The ideal would be

  • provide small, medium, large (and maybe original?) size in the API..
  • make these configurable through a config files.
  • Have some ability to generate them on the fly, in case sizes are changed after the fact.
  • Cache/Store any auto generated images to avoid doing that twice.
  • I'd prefer not to store resized images in the DB, just find them by naming convention, but think thats just my personal preference..

However do what you think makes sense given the CDN and API limitations.. we probably can't get all of these sorted.

Also agree on just maintaining file_url

@@ -2,7 +2,7 @@

/**
* Ushahidi API Posts Controller
*
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to revert all the random whitespace changes in Posts and Sets before merge..

@@ -108,7 +108,7 @@ git submodule update --init
6. Edit ```application/config/environments/development/init.php``` and change base_url to point the the httpdocs directory in your deployment
7. Copy ```httpdocs/template.htaccess``` to ```httpdocs/.htaccess```
8. Edit ```httpdocs/.htaccess``` and change the RewriteBase value to match your deployment url
9. Create directories ```application/cache``` and ```application/logs``` and make sure they're writeable by your webserver
9. Create directories ```application/cache```, ```application/media/uploads``` and ```application/logs``` and make sure they're writeable by your webserver
```
mkdir application/cache application/logs
chown www-data application/cache application/logs
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the example commands too?

/**
* Path to the image cache directory you would like to use, don't forget the trailing slash!
*/
'cache_dir' => APPPATH.'cache/imagefly/',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any idea if this should be using DIRECTORY_SEPARATOR here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DIRECTORY_SEPARATOR is not really necessary but a good practice. I also think it's future safe. Maybe not. I'll change the / to that instead for the sake of 'good practice'

@rjmackay
Copy link
Contributor Author

I think this is done bar minor things.. fix the docs. make a call on the config. Merge it! 👍

eyedol added a commit that referenced this pull request Oct 18, 2013
@eyedol eyedol merged commit 12717e1 into master Oct 18, 2013
@rjmackay
Copy link
Contributor Author

@eyedol is this: https://wiki.ushahidi.com/display/WIKI/Ushahidi+3.x+API+Media+Resource all up to date with the changes?

@eyedol
Copy link
Contributor

eyedol commented Oct 19, 2013

Yes. Updated it with the changes. Mind a review?

@rjmackay rjmackay deleted the media-model-api branch November 4, 2013 22:21
nimishmedatwal pushed a commit to nimishmedatwal/platform that referenced this pull request Mar 11, 2024
* add toggle button to sidebar at data view page

* fix angular json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants