Integrate services with the libraries module to improve handling of third party libraries.

I don't like the idea of removing these libraries. It will break the bot that we really don't like to do. In the next version of services we hopefully would be able to avoid these libraries by using Symfony components.

Moving the issue to postponed.

Im kind of the same opinion. We just got our bot working and now its going to break again plus add an extra step to the install process.

I really don't mind continuing to apply patches for updates to these libraries, but I understand its a drupal rule to not put them in there.

If we decide to remove libraries we should have proper documentation in README.txt file.

@marcingy could you please add chapter for installation notes there.

Looks great!

FWIW, I'd suggest that those libraries be installed (and looked for) under sites/all/libraries rather than in a directory in the module. This avoids the hassle of having to replace the libraries every time a module update is installed. ...Maybe even directly integrate with the LibrariesAPI (http://drupal.org/project/libraries).

I noticed this today as well, and I tend to agree.

Patches welcome!

Here is a patch that adds Libraries API support for the YAML parser. Patch applies to D6 services 3.1.

Users will need to download spyc from http://code.google.com/p/spyc/downloads/list and put it in their sites/all/libraries/spyc folder.

Does libraries give the ability to require a dependency when certain conditions are met?

Do we need to remove the hook_requirments for the spyc library?

Also of note, the spyc is only needed for YAML so Id like to see this stuff only required when application/yaml response format is enabled in the Server Settings of the endpoint.

I think the best approach is probably to use libraries api if it exists for all 3rd party libraries -- falling back to a hard coded or configurable location if it does not, and to keep all dependency checks in hook_requirements.

Attached is a patch that decouples spyc (and, for that matter, YAML) from rest_server. The patch looks in both the libraries location and the old location (lib/spyc); if it detects spyc in either location, it uses that. If it doesn't detect spyc, it removes YAML from the rest_server options cleanly.

This introduces a new dependency on libraries to rest_server. I think this is a preferable trade-off since we are no longer hard-wiring a dependency on spyc.

I'm attaching one patch that is git apply-able. The other ("spyc-decoupling-makeable-1325572-22.patch") works with Drush make. I have tested this patch for all cases described above.

omg jobeirne++++

It would be nice if this came with tests that verify things work with YAML

And after that id say commit and port to 6.x

Will do. Can I get commit access to add tests/update README, etc? I have plans to library-ize the rest of the dependencies as well.

You shouldn't need commit access to provide a patch just as you have done.

No problem, so long as the commits are attributed properly.

> multi-line string
Drupal standards indicate 80col should be respected. The string was broken up to accomodate this.

> extra space
Oops.

> insufficient doxygen
The @return line pretty clearly indicates exactly what the function does. I'm not sure what else to tell you. As a side note, you should probably look into semi-colons and other forms of punctuation.

> "drupal coding standards"
I'm not sure how the body of that function "doesn't meet Drupal coding standards." Could you elaborate?

From the link you posted above:

In general, all lines of code should not be longer than 80 chars.

There are no exceptions listed for strings or function calls that I have found.

As far as the control structures qualm, the standard does not mandate but only "strongly recommend[s]" braces.

Enjoy the patch.

Alright, your changes have been made. Again, it'd be really nice if a non-patched Services could be used with programmatic builds. This patch allows that to happen.

Just a note that lots has been going on in the Libraries API 2.x branch. Might be good to expose the yaml formatter in a separate module via rest_server_response_formatters_alter().

getting this error with the patch installed when turning on REST server.

Fatal error: Call to undefined function _rest_server_get_spyc_location() in /var/www/v4docs-dev.apigee.com/webapp/sites/all/modules/contrib/services/servers/rest_server/rest_server.install on line 15

If the rest server is not activated, you need to do a "module_load_include" or something to make sure the .module file is available to provide the function.

Ah, that's a big one. :)

I'll fix and re-roll.

Fixed. I tested an install on a clean instance of 7.

Im fine with the tests as a later patch. this patch looks good to me and can be attributed to jobeirne with --author="jobeirne "

I dont understand the differences between the patches though, which one would you like committed, also can you provide a 6.x patch as well?

Good news. One patch is apply-able with drush make, one is apply-able with git's apply. I don't know 6.x very well --- all of my dev experience is in 7; if lacking a 6.x patch is a deal-breaker, I can look into it.

Although I don't want to bike-shed the issue, I just wanted to point out that Drupal 8 is adopting some parts of Symfony. Considering that, it might be a good idea to switch over to Symfony YAML sometime. I've been putting some time into the Symfony module if you want to add some support for it in there itself.

Yeah Drupal 8 services is going to be an entirely different thing with the wscci and all most of this probably won't matter to us at that point and services will just be a REST provider of some sort.

Hey Guys,

Not sure what the current status is here. Currently the 7.x-3.1 tag does not include the spyc.php library, and downloading it into libraries does nothing and still breaks the install. The patches seem to fail too.
Thanks!!

Oh wait never mind, saw the message on the project page. Sorry about that :)

If this module is being installed before the libraries modules has actually been enabled (such as from an installation profile) then libraries_get_path() will not be available when rest_server_requirements() is actually installed. I added a module_load_include() to rest_server_requirements() to ensure the function is available.

The commit is appreciated, but in the future it'd be nice if you used proper attribution for the commit ("--author=...").

As an aside, i always apply git format-patch patches which include the author http://drupal.org/node/1146430

This patch seemed to apply to Git and tarball, so I skipped the makeable trend going in this thread.

looks good.

Fixed thanks guys!!!

Here is some recommended way to use Libraries module for Services module. This is against 7.x-3.x branch.

The way it #58 currently implemented is the standard usage of the Libraries module, it should implement the hook_libraries_info() to compatible with the other library functionality like drush libraries and all the other. This patch may does the same but in proper way with hook_libraries_info(). Worth reviewing so changing to need review.

Styles issues has been fixed.

With do not test flag, since MySQL failed.

Are tests currently failing? Why is this patch failing?

The patch fails due to undefined function call because it was removed. It looks like the calls to the function should be removed and are not used anymore.

Let's see how it likes this.

Do we have another test that can't pass pifr-only? :(

I still only get 6 fails in another issue. Hmm...

#69: 1325572-services-with-the-libraries-69.patch queued for re-testing.

#69: 1325572-services-with-the-libraries-69.patch queued for re-testing.

#63: 1325572-services-with-the-libraries-63.diff queued for re-testing.

ITs failing because it doesnt apply cleanly.

Here is the new patch.

Think there's a typo in rest_server.install.

change

if (($library = libraries_load('spyc')) && !empty($library['loaded'])) {
...
}

to

if (($library = libraries_load('spyc')) && empty($library['loaded'])) {
...
}

so that the error message is shown only when spyc is not loaded?

Good call.

Is it just me or was this change never actually documented? I can't see any mention of it in the README.txt, docs, or project page.

It was in the 3.1 release notes, but it was hard to notice.

I just reviewed the release notes again, and I still don't see it though I'm sure it's in an issue title or something.

For such a huge change, it would be good to provide information at the top of the release notes above the changelog indicating an API change has taken place. See for example how I outline major changes in Commerce release notes: http://drupal.org/node/1819278 (including the note about the Drupal >= 7.15 dependency)

I really think the project page should be updated to include mention of the dependency on Libraries. And anywhere that you talk about downloading Spyc.php to the REST server module's lib folder, should that not be replaced with a mention of the sites/all/libraries/spyc directory? Or perhaps I don't even know what Spyc.php is for, because I put it in the libraries directory and haven't had any problems yet.

Additionally, the project page should be updated to remove the outdated reference to http://drupal.org/node/1313976.

I'd really like to standardize on the Services module for sites needing to build Drupal Commerce REST web services, but I'm wondering if I should be more involved to ensure the product documentation and maintenance is more complete / stable for our end users. It would be disappointing to recommend a solution that might sneak API changes / dependencies into minor releases every now and then. : P

Would you like me to reopen this issue or perhaps post a follow-up for documentation?

Another big API change came in #1154420: Readd support for rich options for resources - needed by auth mechanism that I completely missed, with the addition of an "operations" array as a wrapper around resource callbacks that wasn't there before. Ostensibly this is backwards compatible, because my test server never broke, but something like this should be declared in big letters at the top of a release. (Or better yet, relegated to a new branch of development.)

Id open a new task for documentation.

Ill make betty release notes in the future documenting changes like this.

THe operations array will fallback on the original api version and that documentation exists here http://drupalcode.org/project/services.git/blob/refs/heads/7.x-3.x:/serv...

Libraries is only required for YAML response format to work.

lol Ok, then I've been worried about it for nothing. Doing all my work with JSON. : P

Ok, I see now the #api_version key, but I don't see anywhere where the differences in API versions are documented. Additionally, the docs do simply state that an operations array is required without any indication that the default API version in the absence of an #api_version parameter means that the operations array is not required.

I don't really think it's a great idea to have one hook with multiple response formats, but if you're going to do that, perhaps you should include two different doc blocks representing the format for the different versions of the API? Or at least some sort of inline notation about the differences.