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

Added a data loader for PHPCR #134

Merged
merged 1 commit into from
Feb 5, 2013
Merged

Added a data loader for PHPCR #134

merged 1 commit into from
Feb 5, 2013

Conversation

Burgov
Copy link
Contributor

@Burgov Burgov commented Feb 2, 2013

No description provided.

@lsmith77
Copy link
Contributor

lsmith77 commented Feb 2, 2013

this could actually be a generic Doctrine ObjectManager loader ..

@Burgov
Copy link
Contributor Author

Burgov commented Feb 3, 2013

While the classes seem similar, I think the way the Managers store files are too different

e.g. this one requires a stream_get_contents call on the getContent() method, while Mongo just returns the getBytes() method. And ORM (blob) is a different story completely (if one were to be built).

@lsmith77
Copy link
Contributor

lsmith77 commented Feb 3, 2013

but wouldn't the difference be the responsibility of the given Image::getContent() method?

@lsmith77
Copy link
Contributor

lsmith77 commented Feb 4, 2013

we could also define an interface which adds methods like Image::getContentResource() or something like that

@Burgov
Copy link
Contributor Author

Burgov commented Feb 5, 2013

@lsmith77 the interface sounds like a bad idea, as it makes ImagineBundle a requirement rather than optional for any library implementing the interface. The image class I'm using, for example, comes directly from the SymfonyCMF CreateBundle. I could extend the image class and configure CreateBundle to use that, but it doesn't make things much easier for the end user...

I agree though that the current implementation is rather weak. Even for the GridFS one. Using the "getBytes" method is merely the suggestion from the docs, but by no means is a "getBytes" method something you can assume. The same goes for the "getContent" method used in this PR...

This could be solved by using the Symfony 2.2+ PropertyAccess component and making the method to be used configurable.

@lsmith77
Copy link
Contributor

lsmith77 commented Feb 5, 2013

so yeah .. its a "duct typing" interface anyway .. so i think we can then just assume that users that want to use this loader also implement a getContentResource() method in their entity or document .. as such i would prefer to make this loader accept any ObjectManager implementation .. is that ok for you?

however to make it easier to handle differences, it might be good to move the fetching of the stream to a protected method. that way people could override this class and simply adjust that single method to use getBytes() or whatever else ..

@lsmith77
Copy link
Contributor

lsmith77 commented Feb 5, 2013

hmm i guess there is another issue that ORM users as well as users of another ODM need to figure out and that is the translation of the "id" into something they can use. I guess for PHPCR ODM it would make sense to support the rootPath option we already support for the FileSystemLoader ..

so i guess maybe we should provide an abstract base class that support ObjectManager with 2 abstract methods getImageId($id) which accepts the $id and converts it to something useable within the given ObjectManager and another one getImageResourceStream($image) which accepts the image instance and returns a resource stream. Then we can have an implementation for PHPCR that extends from this abstract class.

lsmith77 added a commit that referenced this pull request Feb 5, 2013
Added a data loader for PHPCR
@lsmith77 lsmith77 merged commit 7a29c08 into liip:master Feb 5, 2013
@lsmith77
Copy link
Contributor

lsmith77 commented Feb 5, 2013

i will take care of the refactoring myself ASAP

@lsmith77
Copy link
Contributor

lsmith77 commented Feb 5, 2013

done in 41a6308

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Level: New Feature 🆕 This item involves the introduction of new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants