Skip to content
This repository has been archived by the owner on Jan 6, 2023. It is now read-only.

[thumbnailer] Support for files in subdirectories #856

Merged
merged 1 commit into from
May 3, 2019
Merged

[thumbnailer] Support for files in subdirectories #856

merged 1 commit into from
May 3, 2019

Conversation

trivoallan
Copy link
Contributor

Many websites store images in a complex directory structure. This PR makes it possible to use thumbnailer in such cases.

For instance : /thumbnail/_/100/100/crop/good/complex/path/to/some-image.jpg

Many websites store images in a complex directory structure. This PR
makes it possible to use thumbnailer in such cases.

For instance :
`/thumbnail/_/100/100/crop/good/complex/path/to/some-image.jpg
@benhaynes
Copy link
Member

Thank you @trivoallan! We'll review this asap! :)

@theharshin
Copy link
Contributor

@trivoallan Thanks for the PR.
I've gone through the PR and what I can see is, you're extracting the thumbnail params from the path and treating rest of the string as a thumbnail URL. While this works, I would like to know how this change will help in improving the functionality of Directus. I would like to know the use case of this change 🙂
Right now Directus is not creating Thumbs in subdirectories & this PR does not add functionality to create subdirectories either. So merging this PR won't make any difference for now!

@trivoallan
Copy link
Contributor Author

@theharshin i'm using the functionnality on this website : https://www.daheardit-records.net/fr/discography/dhr-41#release where the images are created and organized by a legacy backoffice.

For several reasons, we do not plan to use Directus to handle the images anytime soon, so this PR makes it possible to use Directus Thumbnailer.

@benhaynes
Copy link
Member

Interesting — so it's: This change doesn't hurt Directus, so why not merge it... versus... this change doesn't help Directus, so why merge it?

We do have an 80/20 rule that dictates we only want to include functionality in the core codebase that at least 80% of our users will want/need. Any thoughts on this?

If it's a minor change that allows some new options without making the codebase unnecessarily complex then I'd say we can merge. However, if it's just going to add more complexity and not be used by many I'd say let's avoid it.

@theharshin — do you have an opinion on if this added code is "worth" adding to the codebase?

@trivoallan
Copy link
Contributor Author

trivoallan commented Apr 29, 2019

@benhaynes in my context it really helps (even is mandatory) Directus adoption : my organisation is planning to migrate all backoffices to Directus.

We have dozens of websites with various directory layouts for storing images. Having Directus Thumbnailer being able to support those various layouts is really important for us to migrate incrementally.

@benhaynes
Copy link
Member

Hey @trivoallan — I completely understand, and that's why we're discussing it. :)

However many of our million+ users have specific requirements they want... even need. Many even might be cool/useful features to have for certain users — but if we indiscriminately add all of those features into the core codebase then we end up with a large piecemeal platform like WordPress (added complexity, more docs, more to maintain, etc).

Our primary goal is to avoid this by keeping the core code as simple as possible, and giving many options for extending the platform.

You've made a good case for your needs, but we need to be judicious with these merges... so let's take a moment to discuss and see what @theharshin @BJGajjar @hemratna think.

@binal-7span binal-7span merged commit d902945 into directus:master May 3, 2019
@theharshin
Copy link
Contributor

@benhaynes @trivoallan The PR looks good after testing and hence merged. Not a major change in terms of operations. Also, @trivoallan has made a good code refactor in the code. Thanks for the PR 🙂

@trivoallan trivoallan deleted the thumbnailer-sub-directories branch May 3, 2019 10:00
@benhaynes
Copy link
Member

Excellent. Thanks @theharshin.

Is there anything we should update in the docs to reflect this change?

@theharshin
Copy link
Contributor

@benhaynes Not required as of now because this is not a big change with perspective of core directus users. It's useful when the user wants to import images from 3rd party websites and wants to generate thumbs on Directus.

@benhaynes
Copy link
Member

Gotcha. Still seems like it might be nice to mention in the Docs as a note/tip. Something like:

:::tip
You can generate thumbnails for images from 3rd party websites by...
:::

Maybe not, it's up to you! I just want to make sure we keep all this info int he docs so we don't forget it's an option (and so our users know it's an option).

@rijkvanzanten
Copy link
Member

We can say that the path of the file matches the path of the filesystem and leave it at that. This feature doesn't let you generate thumbnails for images from 3rd party websites

hemratna added a commit that referenced this pull request May 7, 2019
* Return object in delete after hook instead of onli ID (#882)

* Add fix for big file sizes

Closes #750

* Add migrations for hash and single-file

* Show correct fields in roles.users

For some reason the database column for options was empty

Closes https://github.com/directus/app/issues/1471

* Delete ISSUE_TEMPLATE.md

* security notice

* Add check for mod_php before setting php_value for upload size

This will prevent errors on systems that don't allow overriding the php
values from within the .htaccess files. This will only check for php 7+
though, as the mod_php directive is version specific. This is okay for
now, as we officially only support PHP 7.1+

* Change field width from integer to string

This will allow the app to render the fields in the correct widths
starting with v7.2.

* Issue fix #854 (#896)

* Add migrations for setting field notes and widths

Lays out the settings a bit nicer and adds setting descriptions.

Fixes https://github.com/directus/app/issues/1379

* Fix sort order of fields on install

* Increase specificity of migrations so it doesn't target non-settings

* Move collection notes to the DB

I'm aware that this makes them english only for the time being.
Once we implement the using the translation column in the app, we
can make them properly translatable.

* Fix abstraction name

* Add migrations for misc fields

Sorting of files, making a couple interfaces required, etc

* Bump version

* Fix: Wrong MIME for extentions in uppercase (#895)

* FEAT more events that invalidate the cache (#892)

* Allowing string relations (#800)

* emoji support for comments and bookmark names

* Use JSON interface for system collections

* Fixing custom primary key primary key column name (#881)

Swapping this variable seems to resolve the issue.

* Bump version

* Issue #885 (#898)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* Issue#885 - Done

* #885 Removed Test cases

* Issue #886 (#899)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* Issue#885 - Done

* Issue#886 - Done

* #886 Reverted unwanted code

* Issue #884 (#901)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* #884 Done

* #884 Removed Test cases

* Issue #884 - Change (#907)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* #884 Done

* #884 Removed Test cases

* #884 change

* Fix#810 (#908)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* #810 done

* #810 Reverting Test Cases

* Issue Fix #902 (#909)

* Issue fix #902

* Add migration for allow value nullable in settings table

* Set texttype for value field

* Doc issue fix #84 (#910)

* Issue fix #841 (#911)

* Increase expiry time of tokens from 5 to 20 minutes (#913)

It should still be pretty secure. This allows the app to go easier on
the refreshing, and it makes sure that you can upload large files
without having the token expire halfway through.

* Fix missing ref to 5 min exp

* Issue Fix #863 (#916)

* Issue fix #853 (#918)

* Issue Fix #920 (#922)

* Issue Fix #920

* Issue Fix #920

* Issue fix #879 (#924)

* [thumbnailer] Support for files in subdirectories (#856)

Many websites store images in a complex directory structure. This PR
makes it possible to use thumbnailer in such cases.

For instance :
`/thumbnail/_/100/100/crop/good/complex/path/to/some-image.jpg

* defaults cors.max-age to 600 (#921)

* Bump version

* Generate GraphQL schema file which support primary-key, text-input and numeric interfaces.

* Implement graphql-php server.

* GraphQL type for DirectUs files

* Code cleanup.

* Adding custom scalar support for Date, Datetime, JSON

* Adding support for the m2o type in schema generation.

* Adding support for m2o type.

* Support for O2M.
GraphQL type for Directus Role.
Rename GraphQL types for Directus user, files.

* Adding pagination support.

* Code cleanup.

* Adding time scalar type.

* Adding meta support.

* Search filter approach 1.

* Search result.

* Adding support for AND and OR logical filter.

* Rebase with master.

* Adding support for Activity, Collection Preset, Collection, Field type. Change the naming convention. Adding pascal case function in string utils.

* Adding support for setting collection.

* Adding support for Folder, Permission, Relation, Revision collection.

* Adding README.md

* Update README.md

* Change naming convention to snake_case.

* Change in naming convention. Merge list and single query into list query by adding additional arg `id`.
Lapsus pushed a commit to Lapsus/api that referenced this pull request May 8, 2019
Many websites store images in a complex directory structure. This PR
makes it possible to use thumbnailer in such cases.

For instance :
`/thumbnail/_/100/100/crop/good/complex/path/to/some-image.jpg
binal-7span pushed a commit that referenced this pull request Jun 17, 2019
* pgsql 10 initial support

* email_notification column must be set as a boolean

* Handle unique column collisions

* BUG delta in revisions can be null

* BUG transformed the remaining lastInsertValue into getLastGeneratedId()

* Pass new item flag to o2m new items

Closes https://github.com/directus/app/issues/1418

* Don't show popover for 0 items / no template

Closes https://github.com/directus/app/issues/1397

* Bug fix (#848)

* Merge conflict resolve

* Handle item not found exception in collection detail API

* Extended the list of safe tags (#849)

As described in issue #832

* Issue fix #819 (#851)

* Mark adding new item as new in m2m

* Bump version

* Revert composer changes

* Issue fix #843 (#852)

* BUG searches with LIKE on non-textual columns

* Remove the extensions from the API

* Issue fix #847 (#857)

* Issue fix #833 (#859)

* Initial commit for documentation (#844)

* Revert "Initial commit for documentation (#844)" (#868)

This reverts commit 6e85d59.

* BUG Bypass Zend-db choice not to allow nullable boolean fields

* BUG field length were not taken into account

* CHORE dupliacted line

* BUG o2m working + post-alter table event dispatching

* Return object in delete after hook instead of onli ID (#882)

* Add fix for big file sizes

Closes #750

* Add migrations for hash and single-file

* Show correct fields in roles.users

For some reason the database column for options was empty

Closes https://github.com/directus/app/issues/1471

* Delete ISSUE_TEMPLATE.md

* security notice

* Add check for mod_php before setting php_value for upload size

This will prevent errors on systems that don't allow overriding the php
values from within the .htaccess files. This will only check for php 7+
though, as the mod_php directive is version specific. This is okay for
now, as we officially only support PHP 7.1+

* Change field width from integer to string

This will allow the app to render the fields in the correct widths
starting with v7.2.

* Issue fix #854 (#896)

* Add migrations for setting field notes and widths

Lays out the settings a bit nicer and adds setting descriptions.

Fixes https://github.com/directus/app/issues/1379

* Fix sort order of fields on install

* Increase specificity of migrations so it doesn't target non-settings

* Move collection notes to the DB

I'm aware that this makes them english only for the time being.
Once we implement the using the translation column in the app, we
can make them properly translatable.

* Fix abstraction name

* Add migrations for misc fields

Sorting of files, making a couple interfaces required, etc

* Bump version

* Fix: Wrong MIME for extentions in uppercase (#895)

* FEAT more events that invalidate the cache (#892)

* Allowing string relations (#800)

* emoji support for comments and bookmark names

* Use JSON interface for system collections

* Fixing custom primary key primary key column name (#881)

Swapping this variable seems to resolve the issue.

* Bump version

* Issue #885 (#898)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* Issue#885 - Done

* #885 Removed Test cases

* Issue #886 (#899)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* Issue#885 - Done

* Issue#886 - Done

* #886 Reverted unwanted code

* Issue #884 (#901)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* #884 Done

* #884 Removed Test cases

* Issue #884 - Change (#907)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* #884 Done

* #884 Removed Test cases

* #884 change

* Fix#810 (#908)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* #810 done

* #810 Reverting Test Cases

* Issue Fix #902 (#909)

* Issue fix #902

* Add migration for allow value nullable in settings table

* Set texttype for value field

* Doc issue fix #84 (#910)

* Issue fix #841 (#911)

* Increase expiry time of tokens from 5 to 20 minutes (#913)

It should still be pretty secure. This allows the app to go easier on
the refreshing, and it makes sure that you can upload large files
without having the token expire halfway through.

* Fix missing ref to 5 min exp

* Issue Fix #863 (#916)

* Issue fix #853 (#918)

* Issue Fix #920 (#922)

* Issue Fix #920

* Issue Fix #920

* Issue fix #879 (#924)

* [thumbnailer] Support for files in subdirectories (#856)

Many websites store images in a complex directory structure. This PR
makes it possible to use thumbnailer in such cases.

For instance :
`/thumbnail/_/100/100/crop/good/complex/path/to/some-image.jpg

* defaults cors.max-age to 600 (#921)

* Bump version

* Fix 943 (#947)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* #943

* Fix 717 (#944)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* #717

* Fix 576 (#926)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* #576 - In progress

* #576 O2M and M20 nested filters

* #576 Fix O2M and M2O nested filters

* get proper string length (#933)

Not tested... I only based this PR on:

Ref: 0fce6a4#commitcomment-33408113

* fixed settings logo (#940)

* added collection/table to InvalidFieldException (#956)

* Fix 931 (#936)

* Test cases : AUthentication - Auth, Forgot Password, Collections - Create, Delete

* #931

* #931

* Issue fix #917 (#960)

* reuse item service instead of using a new instance (#959)

* Issue fix 762 (#961)

* Plain text mail issu resolve (#966)

* Bump version
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants