Update documentation and make file relating to Spyc requirement and Libraries integration + make libraries optional

This is not a bug. The code in question is a third party library that should never have been in the repo and was infact breaking the contributor agreement so it had to be removed.

Although not a bug, I would argue that the current alert is not clear. It took a web search for me to find it (and this issue). Might I suggest:

[error]
A library is missing for rest_server to work (Currently using Missing library Please download spyc and copy the file called spyc.php into the sites/all/modules/services/servers/rest_server/lib/ directory.)                                  [error]

Of course an even better solution would be to read it from the sites/all/libraries location.

Agreed with the above and renaming this issue. I couldn't find any documentation other than failing the installation and then downloading the recommended library. Additionally, the error message isn't clear that the REST Server module wasn't in fact enabled due to the error.

I recommend changing this:

Please download <a href="@download">spyc</a> and create a file called spyc.php in rest_server/lib

To this:

The REST Server module did not install because you are missing the <a href="@download">spyc</a> library. Download that library and move its spyc.php file into services/servers/rest_server/lib.

However, as mcfilms stated, it would be much better to depend on http://drupal.org/project/libraries and allow spyc to be placed in its entirety into sites/all/libraries/spyc instead of moving a single file into the module's subdirectory.

I believe this issue is irrelevant now as integration with libraries and drush has been done. Closing this issue.

The docs did not get updated as part of #1325572: Integrate services with the libraries module to improve handling of third party libraries., and this seemed like the best place for this patch.

• spyc changed to Spyc where appropriate - that seems to be the correct capitalization of the project.
• It looks like Spyc moved to GitHub so I updated URLs where appropriate, including services.make.
• Updated README.txt to indicate Libraries module can be used.
• Updated text in rest_server.install.

#5: services-1349588-5.patch queued for re-testing.

Nothing here applies and its part of a dependency now.

Currently updating the module seems to break services if libaries 2.x is not installed:
[Sat Dec 01 20:21:53 2012] [warn] [client ....4] mod_fcgid: stderr: PHP Fatal error: Call to undefined function libraries_load() in /var/www/vhosts/a../httpdocs/sites/all/modules/services/servers/rest_server/rest_server.install on line 18, referer: http://.../admin/reports/updates/update

Also for me it wasn't clear what to do once services was installed. "...or use Libraries to retrieve spyc." doesn't really tell you what to do. I copied the file into half a dozen places untill it worked so I'm not sure which one was to correct one and therefor can't make a suggestion how the text should read. I have to get going but I wanted to share: The current documentation is not clear about what to do. People will come and ask again if this is left as won't fix.

+1 on #9.

Status Report shows me this: "To enable YAML HTTP requets/responses, please download spyc and create a file called spyc.php in rest_server/lib or use Libraries to retrieve spyc."

This is despite the fact that spyc.php is in rest_server/lib.

I've also put it in sites/all/libraries and sites/all/libraries/spyc.

The status report message remains, and my Services installation doesn't seem to work, so I have no idea which moving part is to blame.

AFAIK, you can't 'use' Libraries to install anything.

AFAIK, you can't 'use' Libraries to install anything.

Think so too and the documentation doesn't rely tell something different obviously. However services ships with a make file that can be used with drush make...

A quick search for the file turns up:

Searching 67 files for "spyc"

.\services\README.txt:
   13  Installation
   14  ============
   15: If you are using the rest server you will need to download the latest version of SPYC:
   16: wget https://raw.github.com/mustangostang/spyc/master/Spyc.php -O  servers/rest_server/lib/spyc.php
   17  
   18: Once downloaded you need to add spyc.php to the rest_server/lib folder which exists under
   19  the location you have installed services in.
   20  

.\services\servers\rest_server\includes\rest_server.views.inc:
   59  
   60    private function render_yaml($data) {
   61:     if (($library = libraries_load('spyc')) && !empty($library['loaded'])) {
   62:       return Spyc::YAMLDump($data, 4, 60);
   63      }
   64      else {
   65:       watchdog('REST Server', 'Spyc library not found!', array(), WATCHDOG_ERROR);
   66        return array();
   67      }

.\services\servers\rest_server\includes\RESTServer.inc:
  556  
  557    public static function parseYAML($handle) {
  558:     if (($library = libraries_load('spyc')) && !empty($library['loaded'])) {
  559:       return Spyc::YAMLLoadString(self::contentFromStream($handle));
  560      }
  561      else {
  562:       watchdog('REST Server', 'Spyc library not found!', array(), WATCHDOG_ERROR);
  563        return array();
  564      }

.\services\servers\rest_server\rest_server.api.php:
   43  
   44    // Add a Yaml response format conditionally.
   45:   if (($library = libraries_load('spyc')) && !empty($library['loaded'])) {
   46      $formatters['yaml'] = array(
   47        'mime types' => array('text/plain', 'application/x-yaml', 'text/yaml'),

.\services\servers\rest_server\rest_server.install:
   16      module_load_include('module', 'rest_server');
   17      module_load_include('module', 'libraries');
   18:     if (($library = libraries_load('spyc')) && empty($library['loaded'])) {
   19        $requirements['rest_server'] = array(
   20:         'description' => 'The spyc library is missing, thus YAML is disabled',
   21          'severity' => REQUIREMENT_INFO,
   22:         'value' => $t('To enable YAML HTTP requets/responses, please download <a href="@download">spyc</a> and create a file called spyc.php in rest_server/lib or use Libraries to retrieve spyc.', 
   23:                       array('@download' =>  'http://code.google.com/p/spyc/downloads/list')),
   24:         'title' => t('Missing spyc library'),
   25        );
   26      }

.\services\servers\rest_server\rest_server.module:
   57      );
   58  
   59:     if (($library = libraries_load('spyc')) && !empty($library['loaded'])) {
   60        $parsers['application/x-yaml'] = 'RESTServer::parseYAML';
   61      }
   ..
  113      );
  114  
  115:     if(($library = libraries_load('spyc')) && !empty($library['loaded'])) {
  116        $formatters['yaml'] = array(
  117          'mime types' => array('text/plain', 'application/x-yaml', 'text/yaml'),
  ...
  222   */
  223  function rest_server_libraries_info() {
  224:   $libraries['spyc'] = array(
  225:     'name' => 'Spyc (A simple YAML loader/dumper class for PHP)',
  226:     'vendor url' => 'https://code.google.com/p/spyc/',
  227:     'download url' => 'https://code.google.com/p/spyc/downloads/list',
  228      'version arguments' => array(
  229:       'file' => 'spyc.php',
  230        'pattern' => '@version\s+([0-9a-zA-Z\.-]+)@',
  231        'lines' => 4
  232      ),
  233      'files' => array(
  234:       'php' => array('spyc.php'),
  235      ),
  236    );

.\services\services.make:
    3  core = 7.x
    4  
    5: libraries[spyc][download][type] = "get"
    6: libraries[spyc][download][url] = "https://raw.github.com/mustangostang/spyc/master/Spyc.php"
    7: libraries[spyc][filename] = "../spyc.php"
    8: libraries[spyc][directory_name] = "lib"
    9: libraries[spyc][destination] = "modules/contrib/services/servers/rest_server"
   10  

35 matches across 7 files

There are several interesting bits in there:

• Some help messages refer to google code, some to github. Google Code is old so this should probably be changed.
• The make file refers to "modules/contrib/services/servers/rest_server" - the contrib folder is not used often and it should be "sites/all/libraries/spyc" anyway since the other folder is not checked anymore.
• spyc.php is the old file name and it has been renamed to Spyc.php on github. The make file tries to rename the file back. This doesn't work on my machine (drush version 5.8, drush make API version 2) and I think we shouldn't change external file names anyway unless we have a very good reason for it.

I fixed the above mentioned items and updated the documentation. Please see and test the attacked patch.

Title change.

I think

Currently sypc has an upstream bug please the version of the code on this issue until the upstream patch has been committed if you reference any error messages

on the modules page shoud be replaced by

In version 3 REST Server module requires the libraries api 2.x+ module. To add YAML support, the Spyc.php file has to be placed at sites/all/libraries/spyc/Spyc.php - See README.txt for more details.

"If you are using the rest server you will need to download the latest version of SPYC."

This isnt true, this is only true if you want to enable YAML.

Thank you for your feedback.
I didn't actually touch that part in the patch. Anyway your right, here is a new patch. I updated my last comment for the modules page as well.

btw: Should we have dependencies[] = libraries (>=2.x) in the rest_server.info? Doesn't hurt and will be needed in most cases at some point but then I guess it's not a hard requirement for the module. Checking for libraries module would be a different issue though...

Well, its not required.

Its only required if you want YAML, so I think we need to figure out why its crashing when someone doesnt have libraries installed.

It's crashing if someone doesn't have Libraries b/c you put it in rest_service.info. This means a minor update from the version before that until now will screw your site up unless you get Libraries, whether you're using YAML or not. That's why s.Daniel suggested taking it out and just documenting for YAML users that it's needed along with Spyc.

Actually it crashes also because there are calls to libraries_load.
All right, then lets check if libraries exists before we load the lib and make libraries optional. I'll post a patch later.

Now with version check of libraries.

Needs version checking at other places as well...

The checking is now bundled in a spyc_loaded() function. Please review in detail since im a PHP novice and don't use YAML (so I cant test this part).
One thing that should be considered here is the performance indications of the checkings since I don't know if the call to system_get_info is cached etc.

Thanks, s.Daniel!

Also, some link to a page about how to use drush make to install only Services (or any module) would be great.

#22 breaks test :/

#22: services-rest_server-Spyc-make_documentation_check_libraries-1349588-22.patch queued for re-testing.

Number of issues

* Missing empty lines at the end of certain files causes

-}
+}
\ No newline at end of file

Also lets name space the new function so services_spyc_loaded.

The new function has a lot of white space issues and appears to use tabs rather than spaces. And why are we checking version information on every single request this is what requirments info in the info file is for or hook requirements in .install.

Dont rly get ad hoc why some services would be affected and some not by this patch. However I don't have access to my dev machine today so here is a patch only to the README.txt to ensure that the failure is actually related to this patch.

Sry not used to simpletest yet...

@marcingy: Just read your message. Thanks for the review. I'll check the whitespace issues when I have access to my dev machine which should be tomorrow.

And why are we checking version information on every single request this is what requirments info in the info file is for or hook requirements in .install.

This is what I ment in regards of performance.
As kylebrowning pointed out Spyc is only required if you want to use YAML and services is only required if you want to load Spyc. Therefore I removed dependencies[] = libraries (>=2.x). If you all feel fine with making libraries a requirement (I am) then I'll be happy to change the patch. If we don't want that we need to check for libraries & spyc in the code and then we should probably think of a better way to do this. If someone can give me a pointer how it should be done I'll do it.

I am more concerned about the version checking on each ocassion why not just do

if (module_exists('libraries') && ($library = libraries_load('spyc')) && $library['loaded']) {

There really is no need for the $info = system_get_info('module', 'libraries'); hunk, if libraries exists as a module we use it.

Only libraries version 2 has the libraries_load function. If someone has libraries 1 installed the code would break. We could do:

if (module_exists('libraries') && function_exists('libraries_load') && ($library = libraries_load('spyc')) && $library['loaded']) {

Attached a simple patch without changes to validate the tests. Actual patch follows.

OK seems like the errors where from previous recent commits.

Changes: Removed the extra function. Check for function_exists. Added a link to drush_make project page.

Thinking some more about this and can't we just do

if (function_exists('libraries_load')  && ($library = libraries_load('spyc')) && $library['loaded']))

Ie if we do want to determine if the version of libraries is correct just check the function?

Few other comments

+    if(!((module_exists('libraries') && function_exists('libraries_load') 
+        && ($library = libraries_load('spyc')) && $library['loaded']))) {

Lets have this on one line and also missing space after if.

There are tab issues isn the array hunk following this.

What does this mean
Normal REST calls will still work.

Why not say 'If you require YAML HTTP requets/responses, please....'

There is a missing new line at the end of rest_server.module.

And in the readme.txt why are you wrapping at column 65 and 66 please extend to 80 characters and oly wrap if the line is going to be over 80 characters.

So mostky nitpicks otherwise the path is looking good :)

I am having the same issue. The file is in both the rest_server/lib folder and the libraries folder. Yet, I'm still getting the error that YAML is disabled. Can someone please tell me how to fix this? Thanks!

same as #39

Same here too :(

Finally, I've moved spyc.php from services/servers/rest_server/lib/spyc.php to sites/all/libraries/spyc/spyc.php, cleared the cache twice (!)... and it's working well.

spyc.php needs to go into a spyc folder, then do the ctfc routine ..

This still needs to be fixed. The error message is not only confusing, it is pointing at spyc.php to be put in the services module folder, which is incorrect.

I give a simplified implementation for this patch, as reference from following:

• services.make: see drustack_extra.make
• rest_server_libraries_info(): see twbs_less_libraries_info()
• rest_server.install: see twbs_less_requirements()

Patch from #44 now applied to drustack_extra.make

Heres an updated patch, it should be green now as the tests have been updated.

I should remove the debug statements, but ill do that tomorrow and commit.

I am using regular JSON for my API, and after noticing on status page that Sypc lib is missing, I have installed it. Unfortunately it converted my API responses from JSON to YAML automatically.

This was confusing.

Next I figured out that if I just leave empty "spyc" directory in sites/all/library status message still shows spyc version 0.5.1 even though there is no Spyc.php.

This was even more confusing.

Am I doing something wrong, on this needs to be fixed, at very least not to show spyc requirement on status page if you do not use yaml?

Please don't open old issues for support requests