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

Allows multiple derivative types. #638

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Daniel-KM
Copy link
Contributor

Hi,

For a project, I need other derivative types than "fullsize", "thumbnail" and "square_thumbnail". In particular, I need mini square thumbnails to build a pretty view and another type between "fullsize" and "original" for a site with big images.

So this little patch allows that. A plugin example is available at https://gist.github.com/Daniel-KM/b91f7f94182e50538149.

Sincerely,

Daniel Berthereau
Infodoc & Knowledge management

@Daniel-KM
Copy link
Contributor Author

Hi,

The Travis test succeeds on php 5.2 but not above because there is a new version of PHPUnit that made deprecated some old code. Nevertheless, the patch works fine (like for other pull requests).

Sincerely,

Daniel Berthereau
Infodoc & Knowledge management

@Daniel-KM
Copy link
Contributor Author

Hi,

This structure is not common, but this is allowed since switch does a loose comparison, so if $property is "my_derivative_uri", case (substr($property, -4) == '_uri'): will check "my_derivative_uri" == true, that is true. Conversely, if $property is "other_value", case (substr($property, -4) == '_uri'): will check "other_value" == false, that is false, so switch goes to next case.

As it's not a common way, I replace the case by:

        // Manage uri for derivatives.
        if (substr($property, -4) == '_uri') {
            $type = substr($property, 0, -4);
            if (isset($this->_pathsByType[$type])) {
                return $this->getWebPath($type);
            }
        }

I rebased it too in order to check the Travis test.

Sincerely,

Daniel Berthereau
Infodoc & Knowledge management

Daniel-KM referenced this pull request in Daniel-KM/Omeka Sep 11, 2014
@Daniel-KM
Copy link
Contributor Author

Hi,

Travis test is ok!

Sincerely,

Daniel Berthereau
Infodoc & Knowledge management

@Daniel-KM
Copy link
Contributor Author

Hi,

The patch has been fixed, so do you think it's a good idea to allow multiple derivatives?

Sincerely,

Daniel Berthereau
Infodoc & Knowledge management

@zerocrates
Copy link
Member

I think the idea of adding extra derviatives is fine, and most of how this accomplishes that looks pretty good.

What I'd like to see though, is the configuration file settings only adding to the set of derivatives we have now. I foresee a lot of potential trouble when users mix this with the existing themes and plugins (and pieces of the core) that assume that "thumbnail", "square_thumbnail", and "fullsize" are available. I see the intent is to let them be moved around, but I think as-is this setup makes it too easy for a mistaken edit to the config file to break things.

@Daniel-KM
Copy link
Contributor Author

Hi,

I commit a patch to set default derivatives types automatically.

Sincerely,

Daniel Berthereau
Infodoc & Knowledge management

@Daniel-KM
Copy link
Contributor Author

Hi,

The branch has been rebased on v2.5.

Sincerely,

Daniel Berthereau
Infodoc & Knowledge management

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

2 participants