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

[WIP] Elfinder #46

Merged
merged 12 commits into from Sep 4, 2013
Merged

[WIP] Elfinder #46

merged 12 commits into from Sep 4, 2013

Conversation

rmsint
Copy link
Contributor

@rmsint rmsint commented Aug 26, 2013

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #28
License MIT
Doc PR symfony-cmf/symfony-cmf-docs#251

TODO to merge

  • code review / cleanup
  • check root creating (see PHPCRDriver:: _stat)
  • check rename error (DocumentManager::move results in an error)
  • add installation documentation
  • recheck image preview with imagine thumbnail - elFinder uses the same url for full screen display
  • check if it is possible to return a placeholder for the url and use a filter or something when rendering content (use a dataFilter rule for ckeditor)

//cc @sjopet

@rmsint rmsint mentioned this pull request Aug 26, 2013
}

if ($doc instanceof ImageInterface) {
$url = $this->mediaHelper->displayUrl($doc);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing this to $this->mediaHelper->displayUrl($doc, array('imagine_filter' => $this->imagineFilter)); will use imagine for the preview if it is enabled, otherwise it will automatically use the default display route.

Copy link
Member

Choose a reason for hiding this comment

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

should we inject a imagine filter configuration then to activate this when imagine is available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@rmsint
Copy link
Contributor Author

rmsint commented Aug 27, 2013

Using the imagine filter does not work as expected. When selecting an image using ckeditor the imagine_filter is also applied and the imagine url is returned. Therefore the change is undone.

Furthermore a media browser abstraction is added, maybe only for the browse url too much atm. On the other side it is consistent with the upload helper. The media browser is implemented in symfony-cmf/create-bundle#75 for Create and adds automatically elfinder to ckeditor if elfinder is installed.

@rmsint
Copy link
Contributor Author

rmsint commented Aug 27, 2013

@helios-ag fyi, this will provide integration between the Symfony CMF and elFinder using the FMElfinderBundle. Do you have time to help us with a quick check of the driver: Adapter/ElFinder/PhpcrDriver, do you see anything faulty or missing? Maybe you find something we missed.

@dbu
Copy link
Member

dbu commented Aug 27, 2013

@helios-ag oh indeed that would be helpful. and maybe you can also help us with the imagine question above: i guess the question boils down to if there is a way to have a separate preview image url and display url. and as a bonus, if there can be some way to select from a list of variants of the image (imagine allows us to define named filters that can scale or otherwise process the image when downloading. while still having only one version of the uploaded image so different variants can be used in different places)

@rmsint
Copy link
Contributor Author

rmsint commented Aug 27, 2013

Rebased to include merged #48


if ($sourceDir === $targetDir) {
// rename
// TODO rename file causes error: Detached document or new document with already existing id passed to persist()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible to solve this by fe. first removing the source and then create a new target object?

Copy link
Member

Choose a reason for hiding this comment

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

this sounds like a bug in phpcr-odm. ideally we would have a PR with a failing test demonstrating the problem.

we noticed this: doctrine/phpcr-odm#262 but thats only when replacing a child, it happened when we uploaded a new image somewhere.

simply moving really should not cause issues.

Copy link
Member

Choose a reason for hiding this comment

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

btw, in phpcr-odm the move always is both path and name, there is no distinction between rename and move. what about simply using move unless $source === $targetDir?

Copy link
Member

Choose a reason for hiding this comment

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

The current code means you cannot rename files or folders in the same directory. If anyone has any additional information about what this bug in phpcr is I'd be willing to look into it. Right now the elfinder rename function using the phpcr driver seems to be completely broken.

Copy link
Member

Choose a reason for hiding this comment

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

the bug is tracked here: #52

if ($doc instanceof ImageInterface) {
$url = $this->mediaHelper->displayUrl($doc);
if ($this->imagineFilter) {
$tmbUrl = $this->mediaHelper->displayUrl($doc, array( 'imagine_filter' => $this->imagineFilter ));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thumbnails are used in the "Icons view" and "List view"

@rmsint
Copy link
Contributor Author

rmsint commented Aug 28, 2013

Not sure if we want to fix the last comments (after #46 (commits)) before merge. If so, someone has to take over, I will be packing my bags for holiday.

@@ -13,9 +13,10 @@
<xsd:element name="extra-filter" type="xsd:string" minOccurs="0" maxOccurs="unbounded" />
</xsd:sequence>
<xsd:attribute name="use-imagine" type="enable" />
<xsd:attribute name="imagine-filter" type="xsd:string" />
<xsd:attribute name="imagine-filter" type="imagine-filter" />
Copy link
Member

Choose a reason for hiding this comment

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

this can't be right. if i read the configuration class correctly, this must be an element.

@lsmith77 lsmith77 deleted the elfinder branch September 4, 2013 06:53
@lsmith77
Copy link
Member

lsmith77 commented Sep 4, 2013

i have created followup tickets

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

4 participants