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

Add jackalope.factory to the DI extension #27

Merged
merged 1 commit into from Dec 12, 2012
Merged

Add jackalope.factory to the DI extension #27

merged 1 commit into from Dec 12, 2012

Conversation

aabdou
Copy link

@aabdou aabdou commented Dec 11, 2012

The Jackalope/Jackrabbit repository factory supports the jackalope.factory factory that could be used to override the factory class the Jackrabbit transport uses. A valid class name or an instance of a class that implements the FactoryInterface are two valid options for the jackalope.factory parameter.

@@ -162,7 +174,7 @@ private function loadJackalopeSession(array $session, ContainerBuilder $containe
}
if (isset($session['backend']['disable_transactions'])) {
$parameters['jackalope.disable_transactions'] = $session['backend']['disable_transactions'];
}
}
Copy link
Member

Choose a reason for hiding this comment

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

please revert

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I didn't clearly describe my use case. What I need to override is the factory NOT the RepositoryFactory. The line you highlighted is the one that creates the RepositoryFactory factory service (which could be RepositoryFactoryJackrabbit or the doctrinedbal option). The pull request I submitted overrides the "factory" that the RepositoryFactory uses to build the transport.

If you open the Jackalope\RepositoryFactoryJackrabbit class then navigate to the getRepository method, it takes the following parameters:

  • jackalope.factory
  • jackalope.default_header
  • jackalope.jackrabbit_expect
  • jackalope.check_login_on_server
  • jackalope.disable_stream_wrapper

The DI extension of the bundle has support for some of the parameters, but the "jackalope.factory" is not there.

Copy link
Member

Choose a reason for hiding this comment

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

i do understand your use case and i am very much looking forward to what you will enable with this change.
i was asking you to revert the whitespace change.

@lsmith77
Copy link
Member

interesting PR .. aside from the whitespace change it looks good to merge to me. may i ask about your use case?

@aabdou
Copy link
Author

aabdou commented Dec 11, 2012

I'm trying to switch the Http layer of the Jackalope/Jackrabbit transport to use the Guzzle Http framework. The factory is the one that provides all the Jackalope entities including the transport client and its request. By injecting a custom factory into the RepositoryFatcory through its parameters, the transport changes will be totally transparent to Jacakalope.

@lsmith77
Copy link
Member

ah very cool.

there is a fairly old ticket for this: jackalope/jackalope#33
unfortunately the FIG group has not yet defined an interface for http clients.

if (class_exists($session['backend']['factory'])) {
$parameters['jackalope.factory'] = $session['backend']['factory'];
} else {
$parameters['jackalope.factory'] = new Reference($session['backend']['factory']);
Copy link
Member

Choose a reason for hiding this comment

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

does it make sense to create a service implicitly? just asking because i never saw this.

Copy link
Member

Choose a reason for hiding this comment

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

it seems OK to me. the error message should be clear enough in case of a misconfigiration. obviously this needs some docs too

@dbu
Copy link
Member

dbu commented Dec 12, 2012

definitely +1 on the PR, makes sense.

@aabdou: is the factory itself documented well enough to implement your own? i don't know if we have assumptions somewhere or the interface could be incomplete. but if it works for you, then its already a good sign :-)

@aabdou
Copy link
Author

aabdou commented Dec 12, 2012

@lsmith77: I reverted the whitespace

@dbu: well, sort of :D You can at least tell that you need to implement a FactoryInterface that provides one method to create an instance of an object and set some params on it.

dbu added a commit that referenced this pull request Dec 12, 2012
Add jackalope.factory to the DI extension
@dbu dbu merged commit 72b15a7 into doctrine:master Dec 12, 2012
@dbu
Copy link
Member

dbu commented Dec 12, 2012

thanks for this contribution!

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

3 participants