New submission for a client library in php for openSRF

Bug #1109301 reported by Pranjal Prabhash on 2013-01-29
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenSRF
Wishlist
Jeff Godin

Bug Description

This is my code submission for Google Summer of Code 2012. The Evergreen Git repo for this is http://git.evergreen-ils.org/?p=working/OpenSRF.git;a=shortlog;h=refs/heads/user/pranjal/new-php-opensrf

Changed in opensrf:
assignee: nobody → Pranjal Prabhash (pranjal-prabhash)

I will sign off on this myself, but am seeking at least one more opinion from the community about merging this, since I have an obvious bias as Pranjal's GSOC mentor here.

tags: added: pullrequest
Jason Stephenson (jstephenson) wrote :

I'll take a look at it and give it a spin.

Changed in opensrf:
assignee: Pranjal Prabhash (pranjal-prabhash) → Jason Stephenson (jstephenson)
Changed in opensrf:
assignee: Jason Stephenson (jstephenson) → nobody
importance: Undecided → Wishlist
status: New → Triaged
Jason Stephenson (jstephenson) wrote :

My opinion after looking at this again:

I think this is a great start but needs more work before going into master. I have the following reasons for my opinion:

1. The Fieldmapper and OpenIlsSimpleRequest objects should be moved from OpenSRF to Evergreen. The Fieldmapper is definitely an Evergreen construct. It looks like OpenIlsSimpleRequest could be renamed to OpenSRFSimpleRequest and stay where it is.

2. Compared to the Perl, Python, and Java implementations of OpenSRF, this one is very simple and very small. Simple is good, but it would seem that the code would fit in better alongside the other implementations if it were more fleshed out and followed their patterns a bit more closely.

3. OsrfSession depends too much on the Fieldmapper and Evergreen being installed. I know that OpenSRF is typically only used with Evergreen, but it seems that OpenSRF is intended to stand as its own project separate from Evergreen. I'd like to maintain that separation. OpenSRF is an interesting project in its own right and should not be relegated to second class status behind Evergreen.

4. I see some troublesome code that follows a pattern found in Evergreen but not so much in OpenSRF. OpenIlsSimpleRequest returns the results of OsrfResponse->parse. This can be a scalar or an array value. Functions that return different types depending on the runtime circumstances of their execution (i.e. what happens internally while they run and not the way that they are called externally) are in my opinion dangerous. They lead to error prone situations when the programmer is lazy and doesn't check return types and just assumes that he will always get the same type of object that he got on his one or two test invocations. Many (Booch, et al.) say that languages that allow this should not be used for large projects. (Yes, Perl, I'm giving you the hairy eyeball.) It demands a great deal of extra discipline on the part of the programmers when such situations arise.

I could go on, but I'll stop there.

I am not knocking Pranjal's effort. I think this is a very good first start at implementing OpenSRF calls in PHP. My overall impression, though, is that this code would belong more in Evergreen and not in OpenSRF since there are dependencies on things that are not installed until Evergreen is installed.

I would not sign off on this code for OpenSRF as is.

I agree with you on #1 and #3. I'll revisit this later and separate it into parts that should be in Evergreen and parts that should be in OpenSRF.

With #2, sure I'd like to see more flesh on this too, but we don't want waiting for a full-featured sleek thing to forestall the inclusion of a reasonably serviceable thing for now, do we?

And on #4, I think it's pretty obvious the project as a whole takes a non-Boochian direction (where *did* we hide those flowcharts that make everything clear?) but since we do this sort of thing in Evergreen anyway, and the function in your example is OpenIlsSimpleRequest(), sure I'd be happy to move that into the EG, not OpenSRF section.

If anyone is reading this and it's already well into summer 2013 or later, and I haven't taken action on it yet, please nag me.

Thanks Jason.

Jeff Godin (jgodin) on 2014-04-08
Changed in opensrf:
assignee: nobody → Jeff Godin (jgodin)
Galen Charlton (gmc) on 2016-02-04
tags: removed: pullrequest
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers